Proxy creation fails where method parameters carry certain modopt combinations #277
Comments
|
Interesting that |
|
When I compiled mvdtom's original interface using MSVC and then inspected the IL, there was no As to the rest of my IL types, I can't say whether there should be marshalling instructions. But I guess C# should be able to deal with |
|
Only see |
|
That's exactly why the modopts are necessary: to prevent a loss of precision in the type mapping. If C++/CLI simply translated I've posted repro instructions above. I guess you want something you can just run as is, without much copying & pasting? I should be able to get something ready tomorrow. |
|
No problems. I checked our With a repro, I can debug the chain and determine whether we got it wrong or recommend the next team. Thanks for being patient. |
|
My ideal goal, is that we end up with a test(which I am willing to contribute) that guards you from this down there which we quite frankly could push up for you. This is TL;DR; but check this issue out :) |
We definitely support modopts on the .NET Framework, it's the reason we've got A failing unit test would be great, if there is an API in the Rhino DLL you can use to reproduce it that would be better than adding another one. |
|
@jonorossi: Unfortunately, the Rhino DLL contains only a single interface with a single method having a single parameter with a single modopt ( I am a bit at a loss how to best provide a repro. @Fir3pho3nixx, why did you link to the other issue above? To show the template I should use to summarise this problem? If that's required, I'll oblige, of course. I'm also uncertain about...
Given that the only C++/CLI compiler is MSVC, which is tied to the Windows platform, I'd say it would be better to work with (more cross-platform) ILASM source code. I'll be happy to post a full repro once I understand what exactly you're after. In the meantime, if you just want some quick code that executes without much setting up and reproduces the reported issue, here you go: using System;
using System.Reflection;
using System.Reflection.Emit;
using System.Runtime.CompilerServices;
using Castle.DynamicProxy;
class Program
{
public static void Main()
{
var assemblyBuilder = AppDomain.CurrentDomain.DefineDynamicAssembly(
new AssemblyName("ModoptAssembly"),
AssemblyBuilderAccess.Run);
var moduleBuilder = assemblyBuilder.DefineDynamicModule("ModoptAssembly");
TypeBuilder typeBuilder = moduleBuilder.DefineType(
"IFoo",
TypeAttributes.Class | TypeAttributes.Interface | TypeAttributes.Public | TypeAttributes.Abstract | TypeAttributes.AutoLayout | TypeAttributes.AnsiClass | TypeAttributes.BeforeFieldInit);
MethodBuilder methodBuilder = typeBuilder.DefineMethod(
"DoWork",
MethodAttributes.Public | MethodAttributes.NewSlot | MethodAttributes.HideBySig | MethodAttributes.Abstract | MethodAttributes.Virtual, typeof(void), new[] { typeof(sbyte) });
methodBuilder.SetSignature(
returnType: typeof(void),
returnTypeRequiredCustomModifiers: null,
returnTypeOptionalCustomModifiers: null,
parameterTypes: new[]
{
typeof(sbyte)
},
parameterTypeRequiredCustomModifiers: null,
parameterTypeOptionalCustomModifiers: new[]
{
// play around with those..
new Type[] { typeof(IsConst), typeof(IsSignUnspecifiedByte) }
});
Type type = typeBuilder.CreateType();
// ...to make this fail or succeed:
var proxy = new ProxyGenerator().CreateClassProxy(typeof(object), new[] { type });
}
}(The above code produces the same type as the C++/CLI compiler would for the interface initially shown. I've also saved the generated assembly to disk and verified it using peverify.) |
|
@stakx I'd love it if you were able to replace the existing unit tests currently using Then obviously turn your code into a unit test. Any idea where the problem might be in the DynamicProxy code? |
|
@jonorossi: I can attempt this. I also need to find out whether the tests can / should work for .NET Core as well. If you guys already know exactly how to do all of this safely, I'd gladly accept some guidance hints. Otherwise, I just see how it goes and report back with a PR. Regarding your last question, I have no idea what the error cause might be, but while I am at it, I can do some investigating. |
.NET Core doesn't have the APIs to emit custom modifiers, you'll see the conditional compilation in MethodEmitter, so none of these unit tests can pass there, maybe with .NET Standard 2.0 we'll get it back though, but don't worry about that. Maybe just start with a pull request to replace the existing modopt unit tests using the Rhino DLL and we can work through exactly how the assembly generation will be impacted, not sure without actually looking at the code what will happen. Once we get that sorted and merged, move on to this defect. Good plan? |
|
Sounds good, will do. :) |
|
I have managed to replicate the issue. Created a C++/CLI interface and class over here: https://github.com/fir3pho3nixx/Core/blob/modopts-repro/src/Castle.Core.Tests.Cpp/Castle.Core.Tests.Cpp.h With the corresponding implementation here: https://github.com/fir3pho3nixx/Core/blob/modopts-repro/src/Castle.Core.Tests.Cpp/Castle.Core.Tests.Cpp.cpp Then wrote 2 tests... One that proxies the concrete type: https://github.com/fir3pho3nixx/Core/blob/modopts-repro/src/Castle.Core.Tests/RhinoMocksTestCase.cs#L382 And another that proxies the interface: https://github.com/fir3pho3nixx/Core/blob/modopts-repro/src/Castle.Core.Tests/RhinoMocksTestCase.cs#L398 The class passes, but the interface fails. The IL Method Signature for both the interface and the class is the same except for one being abstract. By dropping the const modifier on I could also verify the modifiers being passed through using this diagnostic code: https://github.com/fir3pho3nixx/Core/blob/modopts-repro/src/Castle.Core/DynamicProxy/Generators/Emitters/MethodEmitter.cs#L174 When both Looks like |
|
@Fir3pho3nixx: I was also just going to submit a second PR containing some failing tests (but might wait a little longer now). I've chosen a slightly different approach, namely to generate the test classes dynamically instead of using the C++/CLI compiler. So I'm essentially using It would probably be worth trying out a few additional things:
|
|
@stakx - Your approach makes sense from a maintainability approach. I have done one better, I found out what the problem is. Check this commit https://github.com/fir3pho3nixx/Core/commit/a5ff169964546cc632eaeeb78de01715efdf0c58 Looks like the parameters are being reversed! :) My C++ CLI test passes with this change. |
|
@Fir3pho3nixx: Awesome find! I noticed the same thing in my unit tests when I had to replace I think I am going to change the tests back to using |
|
@stakx - Look forward to the PR :) |
|
Can do. Also, it probably wouldn't hurt to add more tests for required modifiers (modreq) and for modifiers as applied to the return type of a method. |
|
Yes, please. Make sure you use more than one modifier, this is what caught us out. |
|
Will do! 👍 (And before you ask, I'll also make sure not to use palindrome sequences. :-D) Btw.: Do you think it would make sense to combine my two PRs into one, or should I keep them separate for now? |
|
Merge them. We know what the issue was. |
|
OK, all done. Let me know if the PR needs any modifications. |
|
Sorry for the silence guys, it's just been one of those weeks. Great work on the getting a fix in with a solid set of unit tests. Do we want to release this as 4.1.1? |
|
stakx@d5c29bf would possibly fix the failing tests on Mono, but I haven't actually verified the claims made in those comments, so no PR just yet. |
@stakx we can't (and don't want to) use |
|
@jonorossi - understood. Running some quick experiments using the Mono compiler suggests that Mono actually reverses the modopts, too, so my above commit is invalid, anyway. I'll get back on this later. |
|
@stakx does order even matter? I can't find any mention of order in ECMA-335. This is the main guts about custom modifiers:
|
In theory, perhaps not, since it doesn't explicitly say that ordering matters. In practice, at least for the CLR, apparently yes; otherwise we would never have had a bug to fix. Judging from the CLI standard's perspective, a test checking for a specific modopt / modreq order is perhaps invalid, meaning it should removed. On the other hand, that test documents why we saw the need to reverse the custom modifier order upon method emission. If we just remove the test, how can we be sure whether we need the |
I should have been clearer, I was referring to runtime operation, does order matter which is seems like it doesn't, custom modifiers are very much like custom attributes. Thinking about the exception message from the CLR ("Signature of the body and declaration in a method implementation do not match"), it sounds like it is doing a signature object comparison against its parent, and after trying to dig into the coreclr code looks like that is true in MetaSig::CompareMethodSigs and its callees. We obviously need to call Reverse so |
|
@jonorossi +1 It looks to me like the test is having trouble asserting the output.
Wondering if we apply ToArray or something to the reverse whether this would work. I will know more once I check it this weekend. |
|
I apologize in advance for the very long post. I'd like to share what I have found regarding how various subsystems deal with modopt ordering. @Fir3pho3nixx, I hope you'll be able to confirm these findings; I'd hate to add to the confusion instead of helping to clear things up. TL;DR:I am basing the following findings on two assumptions that I haven't yet verified any further:
Here is what I found:
This is then how modopts should be treated in Castle.Core:
What follows below are the details that led me to the above conclusions. Compiling with ILASM:I started by compiling this class with ILASM: An ILDASM dump of the generated DLL reports the following (abbreviated): The signature lists the modopts in reverse order when compared to the original IL source code.
@jonorossi - Let's very briefly consider if the reversal might just be coincidence. Manually decoding the above signature (according to ECMA 335, section II.23.2) shows that the custom modifiers have not been sorted alphabetically, nor according to their index in the
Reflecting over the above DLL with
|
|
@stakx thanks for doing the investigative work and the detailed write up of your findings.
Did you mean to write CLR in the last sentence here, or Mono?
I'm a little lost by your recommendations, on the CLR should I came across a GitHub issue on the .NET Portability Analyzer that also ran into this ordering problem, referring to "IL syntactic order instead of the metadata order" with a Microsoft employee explaining how they are writing the metadata order: microsoft/dotnet-apiport#236 (comment). It appears to indicate that When we do make these changes we should change to using |
|
@stakx - Your write up is much appreciated. @jonorossi - What I am getting is we should reverse the params on windows but not on mono. I have also done some more investigation. I traced out the parameters going into and out of @stakx test and I also found some rather peculiar behaviour on mono. Added some trace methods here and here. When I run it on windows, for the first type I can see the following output: However when I run it on mono(4.x or 5.x) I dont get any modifiers coming back out. This happens for everything generated in the test. There is something rather dodgy about how mono reports these modfiers using reflection(they simply just aren't there!). Busy creating a standalone console app that hopefully will demonstrate the issue. |
|
After running this windows I get ...
But on linux I get:
Notice how the modifiers are missing from the output? |
Sorry, you caught a typo there. I did indeed mean Mono, not CLR.
Basically, my current recommendation boils down to this: The only place where custom modifier order should be reversed is when reading them on the CLR with
I vaguely remember that when I ran my first experiments on Mono with custom modifiers, I came across a situation where not modopts were reported back to me. Then I rewrote my test code, and all of a sudden, the modopts got reported again. I concluded that the error must've been my own, but now that you've noticed the same thing happening, I'm not so sure anymore. (I ran my Mono tests on Windows, btw.) |
|
My recommendation is we create a separate issue for tracking mono problem and change the test fixture to This will bring our travis build back on master, and we can then track the mono issue separately after we have raised my repro as an issue with the right team. |
|
@stakx - You found one crazy issue! :) |
|
@Fir3pho3nixx: I'm actually feeling somewhat shocked at how this has turned into a convoluted knotty mess... :-) |
@stakx great, I thought that is what you meant, your statement "never reverse ordering when emitting via MethodBuilder" was what I kept getting caught on since that is what DP does (use a MethodBuilder), but I think you meant when the unit tests use MethodBuilder.
@stakx yep, this is the world of reflection emit unfortunately, we've had all sorts of painful stuff like this over the years 😄
@stakx @Fir3pho3nixx since people are much less likely to use custom modifiers on Mono since there is no C++/CLI, I think @Fir3pho3nixx is right, let's get the build going again and make sure this works on .NET Framework, and either have different asserts on Mono or exclude the tests completely with |
|
Master is now green, closing. |
|
PS: Perhaps the tests in |
|
Gentlmen, sorry to sound like a grandma that has wet her diaper, after reading #94 I think we missed out on an opportunity. In this comment I talked about making this code change to the test. Instead we ignored the entire fixture using the I liked my suggestion because if mono(massive assumption) ever fixed the problem this test would automagically come alive when we run it on TravisCI after upgrading mono. Excluding it outright is harsh. We reduce it to an NUnit Theory using Assume which exits gracefully and then one days when we get results > 0, tada! the test tells us. Can we change this code? I will submit the PR. Sorry! |
:-D. I guess now that the build is fixed, it would be nice to improve on those tests before we go on and forget about them. 👍 I would also be quite happy to leave the next PRs concerning this up to you, since my Windows-based Mono installation appears to be partially corrupt, which makes reliably building & testing Castle.Core rather difficult. Regarding your suggestion to replace the test fixture attribute
I think it's a great suggestion, but perhaps not sufficient. If Mono does report any custom modifiers, this by itself would make the tests fail again. You might also need to change Perhaps Mono needs a whole different set of tests altogether (or at least a few |
|
I want the test to fail again. I am coding conscious living guards into the tests that remind you. Anybody that cares needs to know something changed. It needs to be re-evaluated at that point in time. |
|
@Fir3pho3nixx - sounds awesome. But if I understood correctly, you want the test to fail at some point in the future (when Mono changes), and not right now? |
|
@stakx - Yes. |
|
I will submit PR tomorrow night. @jonorossi - Cool? |
Not at all, I would have preferred we could have run the test even without decent asserts. Thanks for sending through the pull request. |
…w: Hardware Intrinsics for Intel Description of the proposal contains specifics about implementation details and formal syntax proposed to support Constant Parameters feature. It is accompanied by early prototype which supports const parameters declaration, invocation of methods with constant arguments with value expressed as constant expression, overload resolution with support for 'const' parameters modifiers. Still no support for code generation as there is no final decision how to represent 'const' parameters at the CLI level. No working implementation for Roslyn Constant Parameter feature conditional support, however, everything is stubbed to support it. Very limited tests.
I am forwarding issue devlooped/moq#244 here as this appears to be a problem with DynamicProxy, not with Moq.
@mvdtom noticed that trying to mock the following C++/CLI interface with Moq will trigger a
TypeLoadException:Moq would create a proxy of this interface as follows (simplified):
The exception thrown is:
This probably has to do with C++/CLI's use of optional modifiers (
modopt) in the method signature. Those can be seen in the IL:@mvdtom also noticed that the exception doesn't happen if the C++
constmodifier is omitted, or if he chooses a type other thanchar. Based on this, I ran some additional tests of my own.I defined a handful of types in IL that differ only in the specific combination of
modopts the parameter carries (seeTypesWithModopt.il, which was compiled usingilasmand checked withpeverify). Then I tried to create proxies for all those types (seeProgram.cs). Here's the result:In summary, DynamicProxy...
IsConstmodopts.IsSignUnspecifiedBytemodopt that is not accompanied by any other modopts.Given that the last two bullet points are very similar, I would guess that DynamicProxy really only supports
IsConst, and otherwise tolerates a single modopt. I haven't tried yet what happens if several modopts are spread across more than one parameter.What is DynamicProxy's position and support for modopts? I realise it is probably geared mainly towards C# and VB.NET, but I'm still curious if DynamicProxy wants to support them at all, or not.
The text was updated successfully, but these errors were encountered: