Closed tamlin-mike closed 6 years ago
I tried putting together a full class definition in IL, but ilasm always reports:
Invalid Add method of event ...
Would it be possible for you to post the full class, event and backing field definition employing the failing pattern? Also, I guess the original assembly references mscorlib v2, because a reference to System.Core is generated when referencing System.Action<T1, T2>... am I right?
Sorry about the incomplete report. I stripped it down too much. Here's a version that ilasm can consume, while displaying both the problem and crash.
// ilasm.exe /nologo /dll whatever.ilasm
.class public auto ansi beforefieldinit Index`2<TK, class .ctor TR>
{
// Fields
.field private class [System.Core]System.Action`2<!TK, !TR> Getting
.field private static class [System.Core]System.Action`2<!TK, !TR> '<>f__am$cache0'
.custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
// Methods
.method public hidebysig specialname rtspecialname instance void .ctor () cil managed
{
.maxstack 8
ldarg.0
ldsfld class [System.Core]System.Action`2<!0, !1> class Index`2<!TK, !TR>::'<>f__am$cache0'
brtrue.s IL_0019
ldnull
ldftn void class Index`2<!TK, !TR>::'<Getting>m__0'(!0, !1)
newobj instance void class [System.Core]System.Action`2<!TK, !TR>::.ctor(object, native int)
stsfld class [System.Core]System.Action`2<!0, !1> class Index`2<!TK, !TR>::'<>f__am$cache0'
IL_0019: ldsfld class [System.Core]System.Action`2<!0, !1> class Index`2<!TK, !TR>::'<>f__am$cache0'
stfld class [System.Core]System.Action`2<!0, !1> class Index`2<!TK, !TR>::Getting
ret
}
.method public hidebysig specialname instance void add_Getting (class [System.Core]System.Action`2<!TK, !TR> 'value') cil managed
{
.maxstack 3
.locals init (
[0] class [System.Core]System.Action`2<!TK, !TR>,
[1] class [System.Core]System.Action`2<!TK, !TR>
)
ldarg.0
ldfld class [System.Core]System.Action`2<!0, !1> class Index`2<!TK, !TR>::Getting
stloc.0
IL_0007: ldloc.0
stloc.1
ldarg.0
ldflda class [System.Core]System.Action`2<!0, !1> class Index`2<!TK, !TR>::Getting
ldloc.1
ldarg.1
call class [mscorlib]System.Delegate [mscorlib]System.Delegate::Combine(class [mscorlib]System.Delegate, class [mscorlib]System.Delegate)
castclass class [System.Core]System.Action`2<!TK, !TR>
ldloc.0
call !!0 [mscorlib]System.Threading.Interlocked::CompareExchange<class [System.Core]System.Action`2<!TK, !TR>>(!!0&, !!0, !!0)
stloc.0
ldloc.0
ldloc.1
bne.un IL_0007
ret
}
.method public hidebysig specialname instance void remove_Getting (class [System.Core]System.Action`2<!TK, !TR> 'value') cil managed
{
.maxstack 3
.locals init (
[0] class [System.Core]System.Action`2<!TK, !TR>,
[1] class [System.Core]System.Action`2<!TK, !TR>
)
ldarg.0
ldfld class [System.Core]System.Action`2<!0, !1> class Index`2<!TK, !TR>::Getting
stloc.0
IL_0007: ldloc.0
stloc.1
ldarg.0
ldflda class [System.Core]System.Action`2<!0, !1> class Index`2<!TK, !TR>::Getting
ldloc.1
ldarg.1
call class [mscorlib]System.Delegate [mscorlib]System.Delegate::Remove(class [mscorlib]System.Delegate, class [mscorlib]System.Delegate)
castclass class [System.Core]System.Action`2<!TK, !TR>
ldloc.0
call !!0 [mscorlib]System.Threading.Interlocked::CompareExchange<class [System.Core]System.Action`2<!TK, !TR>>(!!0&, !!0, !!0)
stloc.0
ldloc.0
ldloc.1
bne.un IL_0007
ret
}
.method private hidebysig static void '<Getting>m__0' (!TK '', !TR '') cil managed
{
.custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
.maxstack 8
ret
}
// Events
.event class [System.Core]System.Action`2<!TK, !TR> Getting
{
.addon instance void Index`2::add_Getting(class [System.Core]System.Action`2<!0, !1>)
.removeon instance void Index`2::remove_Getting(class [System.Core]System.Action`2<!0, !1>)
}
}
Thank you for posting the working sample. It helps a lot!
After fixing the crash in ConvertConstructorCallIntoInitializer
, it looks like this is the same as #1036.
I was not yet able to find out which compiler is generating this pattern. Any hints?
I totally missed #1036. Yes, this looks exactly like it.
The compiler is in all likelyhood from MONO, or a modified version of it. The environment producing the code is with 99% certainty UNITY.
I have implemented a matcher for MCS event
that partially works. The strange thing is, when I look at the class containing the event I get:
public event Action<TK, TR> Getting = delegate
{
};
but when I drill down to the event itself (select it in the tree-view), it displays properly:
public event Action<TK, TR> Getting;
I suspect there's an attribute that needs to be removed from it, to suppress the faulty addition of = delegate {};
, but I don't know where or what.
Anyway, please consider the following as a partial fix.
--- a/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs
+++ b/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs
@@ -631,6 +631,44 @@ Identifier ReplaceEventFieldAnnotation(Identifier identifier)
}
}
};
+
+ // MONO MCS pattern
+ static readonly Accessor automaticEventPatternV4MCS = new Accessor {
+ Attributes = { new Repeat(new AnyNode()) },
+ Body = new BlockStatement {
+ new AssignmentExpression {
+ Left = new NamedNode("var1", new IdentifierExpression(Pattern.AnyString)),
+ Operator = AssignmentOperatorType.Assign,
+ Right = new NamedNode(
+ "field",
+ new MemberReferenceExpression {
+ Target = new Choice { new ThisReferenceExpression(), new TypeReferenceExpression { Type = new AnyNode() } },
+ MemberName = Pattern.AnyString
+ })
+ },
+ new DoWhileStatement {
+ EmbeddedStatement = new BlockStatement {
+ new AssignmentExpression(new NamedNode("var2", new IdentifierExpression(Pattern.AnyString)), new IdentifierExpressionBackreference("var1")),
+ new AssignmentExpression {
+ Left = new IdentifierExpressionBackreference("var1"),
+ Right = new InvocationExpression(new MemberReferenceExpression(new TypeReferenceExpression(new TypePattern(typeof(System.Threading.Interlocked)).ToType()),
+ "CompareExchange",
+ new AstType[] { new AnyNode("type") }), // type argument+ new Expression[] { // arguments
+ new DirectionExpression { FieldDirection = FieldDirection.Ref, Expression = new Backreference("field") },
+ new CastExpression(new Backreference("type"), new InvocationExpression(new AnyNode("delegateCombine").ToExpression(), new IdentifierExpressionBackreference("var2"), new IdentifierExpression("value"))),
+ new IdentifierExpressionBackreference("var1")
+ }
+ )}
+ },
+ Condition = new BinaryOperatorExpression {
+ Left = new CastExpression(new TypePattern(typeof(object)), new IdentifierExpressionBackreference("var1")),
+ Operator = BinaryOperatorType.InEquality,
+ Right = new IdentifierExpressionBackreference("var2")
+ },
+ }
+ }
+ };
bool CheckAutomaticEventMatch(Match m, CustomEventDeclaration ev, bool isAddAccessor)
{
@@ -676,10 +714,22 @@ bool CheckAutomaticEventV2(CustomEventDeclaration ev, out Match addMatch, out Ma
return true;
}
+ bool CheckAutomaticEventV4MCS(CustomEventDeclaration ev, out Match addMatch, out Match removeMatch)
+ {
+ addMatch = removeMatch = default(Match);
+ addMatch = automaticEventPatternV4MCS.Match(ev.AddAccessor);
+ if (!CheckAutomaticEventMatch(addMatch, ev, true))
+ return false;
+ removeMatch = automaticEventPatternV4MCS.Match(ev.RemoveAccessor);
+ if (!CheckAutomaticEventMatch(removeMatch, ev, false))
+ return false;
+ return true;
+ }
+
EventDeclaration TransformAutomaticEvents(CustomEventDeclaration ev)
{
Match m1, m2;
- if (!CheckAutomaticEventV4(ev, out m1, out m2) && !CheckAutomaticEventV2(ev, out m1, out m2))
+ if (!CheckAutomaticEventV4(ev, out m1, out m2) && !CheckAutomaticEventV2(ev, out m1, out m2) && !CheckAutomaticEventV4MCS(ev, out m1, out m2))
return null;
RemoveCompilerGeneratedAttribute(ev.AddAccessor.Attributes, attributeTypesToRemoveFromAutoEvents);
EventDeclaration ed = new EventDeclaration();
It seems like mcs is generating code in the ctor that initializes the event with an empty delegate. I think we will have to adjust the test to match the output. Not sure if it is worth the effort to add an additional transform that removes these initializations.
Could you try to find out what mcs generates when using plain event syntax without initialization? (it might be that the original code in your example did indeed initialize the event...)
Could you try to find out what mcs generates when using plain event syntax without initialization? (it might be that the original code in your example did indeed initialize the event...)"
Actually, the class I'm looking at has two events, and the other one does NOT get initialized by an empty delegate. That either suggests a pretty blatant compiler bug, or more likely that it is indeed a user-specified construct.
I'll try to find an mcs to see what it does, but meanwhile I'm leaning towards (to the point that I'm assuming) "user-specified". Provided that assumption is correct, the patch I provided could be considered both complete and final.
Another event pattern I have encountered (I should really open a new issue for this, but since it's somewhat related I'll piggyback on this one) is the following:
private SomeType BackingField;
public event SomeType OnSomething
{
[MethodImpl(MethodImplOptions.Synchronized)]
add
{
this.BackingField = (SomeType)Delegate.Combine(this.BackingField, value);
}
[MethodImpl(MethodImplOptions.Synchronized)]
remove
{
this.BackingField = (SomeType)Delegate.Remove(this.BackingField, value);
}
}
While pointless in light of event's being automatically thread-safe (at least nowadays), it's a pattern that is used and at least those method bodies should probably be recognized to not use the explicit call-sequence, but +=
and -=
. It would be a stepping stone to correctness.
Comments?
I believe the "probably wont fix" label is in error, since from my testing the patch I provided is solving it.
Comments?
Thank you! I implemented your pattern in 659b12bdb82e8fa80ae7577cf67430df36fbe390.
I think the other pattern you came across is similar to the one described in automaticEventPatternV2
. Can you try to debug why it does not match?
Also, if you have larger patches, please think about making a pull request instead of pasting the patch as comment. This would make my life a little bit easier, thanks! ;-)
@siegfriedpammer re. automaticEventPatternV2
, I think I found the source of the problem.
This is what the compiler produces. See anything unusual? Like, castclass
?
No wonder the matcher didn't grok it.
If we take the add
handler as an example, the AST becomes
stfld bkFld(ldloc this, castclass TEvent(call Combine(ldfld bkFld(ldloc this), ldloc value)))
.
Is it possible to add an optional qualifier to an added castclass
in the matcher? If not, this would become yet another complete matching-rule, and the potential for a future explosion worries me a bit.
Ultimately, a compiler could chose to interleave other instructions without side-effects (while unlikely for MSIL, just see what f.ex. C compilers do with x86 code), leaving us in an unmanageable mess (again).
I think what I'm getting at is that doing this parsing at this low level might eventually make the code unmanageable, and a higher-level approach would become needed. Instead of looking at explicit ldfld
, stfld
and so on, a more DFA-like approach could become neccessary.
We're not there just yet, but since I think it's an important consideration, and my idea here could have some merit, I felt it worth spending the space and time to share it.
Sidenote: Do you still think the mcs - probably wont fix
tag applies? :-)
.event TEvent foo
{
.addon instance void T::add_foo(class TEvent)
.removeon instance void T::remove_foo(class TEvent)
}
.method public hidebysig specialname
instance void add_foo (
class TEvent 'value'
) cil managed synchronized
{
.maxstack 8
IL_0000: ldarg.0
IL_0001: dup
IL_0002: ldfld class TEvent T::eventBackingField
IL_0007: ldarg.1
IL_0008: call class System.Delegate::Combine(Delegate, Delegate)
IL_000d: castclass TEvent // <-
IL_0012: stfld class TEvent T::eventBackingField
IL_0017: ret
}
.method public hidebysig specialname
instance void remove_foo (
class TEvent 'value'
) cil managed synchronized
{
.maxstack 8
IL_0000: ldarg.0
IL_0001: dup
IL_0002: ldfld class TEvent T::eventBackingField
IL_0007: ldarg.1
IL_0008: call class System.Delegate::Remove(Delegate, Delegate)
IL_000d: castclass TEvent // <-
IL_0012: stfld class TEvent T::eventBackingField
IL_0017: ret
}
So in order to reproduce this, I downloaded myself a copy of old old Mono 2.0 (from 2008) and compiled this code:
using System;
class Test {
public event EventHandler TestEvent;
}
using mcs /target:library test.cs
This is the pattern generated:
.class private auto ansi beforefieldinit Test
extends [mscorlib]System.Object
{
// Fields
.field private class [mscorlib]System.EventHandler TestEvent
// Methods
.method public hidebysig specialname rtspecialname
instance void .ctor () cil managed
{
// Method begins at RVA 0x20ec
// Code size 7 (0x7)
.maxstack 8
IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: ret
} // end of method Test::.ctor
.method public hidebysig specialname
instance void add_TestEvent (
class [mscorlib]System.EventHandler 'value'
) cil managed synchronized
{
// Method begins at RVA 0x20f4
// Code size 24 (0x18)
.maxstack 8
IL_0000: ldarg.0
IL_0001: ldarg.0
IL_0002: ldfld class [mscorlib]System.EventHandler Test::TestEvent
IL_0007: ldarg.1
IL_0008: call class [mscorlib]System.Delegate [mscorlib]System.Delegate::Combine(class [mscorlib]System.Delegate, class [mscorlib]System.Delegate)
IL_000d: castclass [mscorlib]System.EventHandler
IL_0012: stfld class [mscorlib]System.EventHandler Test::TestEvent
IL_0017: ret
} // end of method Test::add_TestEvent
.method public hidebysig specialname
instance void remove_TestEvent (
class [mscorlib]System.EventHandler 'value'
) cil managed synchronized
{
// Method begins at RVA 0x2110
// Code size 24 (0x18)
.maxstack 8
IL_0000: ldarg.0
IL_0001: ldarg.0
IL_0002: ldfld class [mscorlib]System.EventHandler Test::TestEvent
IL_0007: ldarg.1
IL_0008: call class [mscorlib]System.Delegate [mscorlib]System.Delegate::Remove(class [mscorlib]System.Delegate, class [mscorlib]System.Delegate)
IL_000d: castclass [mscorlib]System.EventHandler
IL_0012: stfld class [mscorlib]System.EventHandler Test::TestEvent
IL_0017: ret
} // end of method Test::remove_TestEvent
// Events
.event [mscorlib]System.EventHandler TestEvent
{
.addon instance void Test::add_TestEvent(class [mscorlib]System.EventHandler)
.removeon instance void Test::remove_TestEvent(class [mscorlib]System.EventHandler)
}
} // end of class Test
The only difference is that there is no dup
used, but an equivalent instruction is used instead, which leads to the following raw C# ('no transforms' step):
public event EventHandler TestEvent
{
[MethodImpl(MethodImplOptions.Synchronized)]
add
{
this.TestEvent = (EventHandler)Delegate.Combine(this.TestEvent, value);
}
[MethodImpl(MethodImplOptions.Synchronized)]
remove
{
this.TestEvent = (EventHandler)Delegate.Remove(this.TestEvent, value);
}
}
And PatternStatementTransform is perfectly able to recognize that pattern.
Please be aware that we match C# AST when trying to detect auto events. So castclass
is already transformed to a C# cast expression.
See https://github.com/icsharpcode/ILSpy/blob/b2d30dc6153aebd6bd1be4e011a4c1da95cb0f30/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs#L576-L593
The cast expression on the right side of the assignment is already there (line 587).
@tamlin-mike Not sure what's going on on your side... maybe you are missing some references? Could you provide a private link to a binary via private gitter? Would love to see the actual binary, because IL snippets are often difficult to get accepted by ilasm. Thanks!
@siegfriedpammer I have found the problem.
It's an obfuscated binary, where private members have 2-3 letter names, so the backing field has a different name than the accessor. This makes PatternStatementTransform.CheckAutomaticEventMatch
fail, due to name mismatch.
sigh I have been barking up the wrong tree the whole time. As a test, I relaxed that name check, and... it should come as no surprise it works.
Sorry for all the confusion.
Provided that name check is useful, should I whip up a patch with a decompiler-option "Don't require event's backing name to match"?
Well... given that it's an obfuscated assembly and ILSpy by definition will not support obfuscated assemblies out of the box, I don't know if that's a useful option. If it's obfuscated, you should use a deobfuscator first. The decompiler is already quite complex as it is now, I don't want to add too many more options...
@tamlin-mike if you are interested in helping us out a bit more (than you already do... thank you!), providing unit tests is always welcome (preferably as a PR). Currently C# 7.0+ ref locals, ref returns and condition ref expressions are implemented, but not yet extensively tested...
@siegfriedpammer
If it's obfuscated, you should use a deobfuscator first.
Already done that, but the deobfuscator didn't rename the event backing fields to match the public field names. Maybe it should, or at least attempt to.
But this got me thinking: Is there really any promise that the backing field will always have the same name as the public event name? It would really surprise me if C#, or even CLR, held such a promise. I suspect it's more of a compiler implementation detail, where current compilers follow this convention. If that suspicion is correct, perhaps that name-check should simply not be performed? Could it lead to false positives if disabled/removed?
It would really surprise me if C#, or even CLR, held such a promise. I suspect it's more of a compiler implementation detail, where current compilers follow this convention.
Most of the patterns the decompiler can detect are based on implementation details of the compiler, so that is not really an argument. But yes, we could do a better job and use the field name detected in the event accessor implementation.
But that leaves us with the problem that we would no longer be able to hide event backing fields, because these are not marked CompilerGenerated
and we do not want to decompile each event, when just displaying the members in the tree-view.
But that leaves us with the problem that we would not longer be able to hide event backing fields
Good point.
and we do not want to decompile each event, when just displaying the members in the tree-view.
If applying some careful heuristics, would that really induce a noticable delay?
Im thinking of something like
private
delegate
event
of the same typeevent
's accessor, to see if that is its backing field.Coming to think of it, the "decompile event accessors" step might even be redundant, and only slow it down.
Another option could be to just display them [the backing fields] as it works today, but once the decompilation kicks in it could apply these heuristics and do "the right thing" for a decompiled type.
I don't know, maybe I over-think or over-engineer a potential solution to a problem not many have. If that's the case, I have no problem keeping local modifications.
Inside
ConvertConstructorCallIntoInitializerVisitor.HandleInstanceFieldInitializers
, the path wherefieldOrPropertyOrEventDecl
is not aPropertyDeclaration
,GetChildrenByRole(Roles.Variable)
can returnnull
, resulting in a crash.I have traced it back to what I believe to be the error of a missing recognizer in PatternStatementTransform, where
event
'sadd
/remove
compiler-generated methods have a slightly different layout than what is currently recognized.I tried to repro with MS' csc.exe (2.6.0.62329 (5429b35d)) but failed. This version of add/remove is recognized.
The csc-produced version looks like:
The failing version looks like:
The same difference applies for the
remove
member.Still, even with added add/remove recognition
HandleInstanceFieldInitializers
could probably benefit from an assert, or even greater resilience.