New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for not wrapping exceptions with TargetInvocationException. #13767

Merged
merged 3 commits into from Sep 6, 2017

Conversation

Projects
None yet
4 participants
@AustinWise
Contributor

AustinWise commented Sep 3, 2017

This add supports for the API described in dotnet/corefx#22866.

@AustinWise

This comment has been minimized.

Contributor

AustinWise commented Sep 3, 2017

There is at least one outstanding issue that might make it a bad idea to merge this pull request immediately: dotnet/corert#4437 already added the new BindingFlags value to the shared System.Private.CoreLib code. I'm not sure what effect adding it this pull request will have. If it causes a problem, I will rebase once that change is merged.

Additionally, there are a couple of there things I'm not sure are correct about this pull requests:

  • If it is ok to call CallDescrWorkerWithHandler from RuntimeMethodHandle::InvokeMethod. The only other method in CoreCLR that calls this is also in reflectioninvocation.cpp. Seeing only one other caller makes me a little nervous.
  • If the units tests belong here or in corefx.
  • If I put the units tests in the right place.
  • What is the right CLRTestPriority to assign the unit tests.
  • I deleted some instances of StackWalkMark that were never passed to to a native method and the associated DynamicSecurityAttribute annotations. Is there anything to worry about when removing these inline-supressing attributes?
@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Sep 5, 2017

Thanks for picking this up, @AustinWise! I'll start looking at the change in more detail but some immediate answers to your questions:

dotnet/corert#4437 already added the new BindingFlags value to the shared System.Private.CoreLib code. I'm not sure what effect adding it this pull request will have.

As long as you picked up the exact change (including whitespaces), there shouldn't be any problem. There'll be an automated PR that brings BindingFlags.cs over from corert in the next few days - if the change has already been made in CoreCLR by then, it'll just be a null commit for that PR.

If the units tests belong here or in corefx.

They belong in corefx, but as it happens, I've already written tests for this feature that I was planning to PR this week since I wanted test coverage for corert and didn't know how long the coreclr side would remain up for grabs. (You can see a preview here: https://github.com/AtsushiKan/corefx/blob/sig-wip/src/System.Runtime/tests/System/Reflection/BindingFlagsDoNotPass.netcoreapp.cs). So you needn't worry about that.

I deleted some instances of StackWalkMark that were never passed to to a native method and the associated DynamicSecurityAttribute annotations. Is there anything to worry about when removing these inline-supressing attributes?

I'm not sure if this is safe or not - cc @jkotas, @gkhanna79

@@ -912,7 +912,7 @@ internal static Utf8String GetUtf8Name(RuntimeMethodHandleInternal method)
[DebuggerStepThroughAttribute]
[Diagnostics.DebuggerHidden]
[MethodImplAttribute(MethodImplOptions.InternalCall)]
internal extern static object InvokeMethod(object target, object[] arguments, Signature sig, bool constructor);
internal extern static object InvokeMethod(object target, object[] arguments, Signature sig, bool constructor, bool doNotWrapExceptions);

This comment has been minimized.

@AtsushiKan

AtsushiKan Sep 5, 2017

Contributor

I think it would be good to make the new argument a positive (wrapExceptions) to match the CreateInstance method. It's easier to follow without the negative.

This comment has been minimized.

@AtsushiKan

AtsushiKan Sep 5, 2017

Contributor

Other than the "doNotWrapExceptions" vs. "wrapExceptions" issue, this looks good to me.

@AtsushiKan AtsushiKan requested a review from jkotas Sep 5, 2017

@jkotas

This comment has been minimized.

Member

jkotas commented Sep 5, 2017

deleted some instances of StackWalkMark

Looks good. Thanks for cleaning it up.

AustinWise added some commits Sep 6, 2017

Respond to PR feedback.
Mainly by making "WrapExceptions" consistently positive in FCalls.
@AustinWise

This comment has been minimized.

Contributor

AustinWise commented Sep 6, 2017

I updated this PR in response to the feedback by changing the "wrapExceptions" parameter to be consistently positive. I also removed the units tests in deference to the future unit tests in CoreFX.

@AtsushiKan In looking at your tests, they cover most code paths, but there are a couple Activator.CreateInstance left unexercised. CoreCLR has separate code paths for constructors with and without parameters. Additionally, if a parameterless constructor does not throw when called, it is cached. Subsequent calls to constructor happen through a delegate and the TargetInvoationException wrapping happens in C#. I covered these cases in my tests here: https://github.com/AustinWise/coreclr/blob/692ab2810bb9bd4e81e9187230dd7d0439ef1c61/tests/src/reflection/DoNotWrapExceptions/DoNotWrapExceptions.cs#L127-L137

@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Sep 6, 2017

@dotnet-bot test OSX10.12 x64 Checked Build and Test

@AtsushiKan AtsushiKan merged commit 1f9aeeb into dotnet:master Sep 6, 2017

15 of 16 checks passed

OSX10.12 x64 Checked Build and Test Build finished.
Details
CROSS Check Build finished.
Details
CentOS7.1 x64 Debug Build and Test Build finished.
Details
Tizen armel Cross Debug Build Build finished.
Details
Tizen armel Cross Release Build Build finished.
Details
Ubuntu arm Cross Release Build Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Ubuntu16.04 arm Cross Debug Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details
Windows_NT x86 full_opt legacy_backend CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Sep 6, 2017

@AustinWise - Thanks for your contribution! I've merged your PR and will augment my tests as you've suggested.

@AustinWise AustinWise deleted the AustinWise:austinBinding branch Nov 6, 2017

akoeplinger added a commit to mono/mono that referenced this pull request Mar 29, 2018

Add BindingFlags.DoNotWrapExceptions (#7863)
The original design review is here: dotnet/corefx#22866 . To summarize, this adds a new flag to `BindingFlags` called `DoNotWrapExceptions`. When this flag is used to invoke a method using reflection, any exceptions thrown are not wrapped with a `TargetInvocationException`.

It has already been implemented in some other version of .NET:
- dotnet/corert#4437
- dotnet/coreclr#13767

I would be delighted if this handy feature was also available in Mono.

I have in part based my changes on the CoreCLR implementation. There all already tests for this feature in corefx, so I have added those to the corlib_xtest.dll.sources file.

I have a couple of concerns about my implementation:
- I notice that `DynamicMethod.Invoke` was ignoring the BindingFlags and other arguments. I changed this method to pass along all the arguments. For what it's worth, it appears that [CoreCLR respects](https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Reflection/Emit/DynamicMethod.cs#L488) these arguments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment