Make ParameterDefaultValue.TryGetDefaultValue bitcode compliant #47197
Conversation
When using this code in Xamarin apps targeting iOS or watchOS, the code failed when bitcode was enabled since a catch exception with a when block is not allowed/bitcode compliant. In order to get this code working on bitcode enabled apps (all apps running in watchOS must have bitcode enabled), this PR removes the when and instead moves the condition into the catch block
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
|
|
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue DetailsContextWhen using this code in Xamarin apps targeting iOS or watchOS, the code failed when bitcode was enabled since a catch exception with a when block is not allowed/bitcode compliant. The same scenario also happened in other parts of the codebase. See mono/mono#19451 for example. Example Stacktrace
|
|
This is valid C# code. Shouldn’t we be working on making this work in AOT? Other libraries are going to use this same pattern/feature. Do we expect them to change as well? |
|
The extra This specific filter should probably be addressed as part of whatever is the solution to #46927 for platforms where a (performant) two pass exception handling semantic cannot be implemented because the underlying platform doesn't support it. (Looks like the underlying issue that required this specific filter was fixed around .NET Core 3.1. Maybe the filter can be ifdeffed to only when compiling for netstandard 2.0?) |
That wouldn't solve the issue for existing Xamarin apps though since they'll still use the netstandard2.0 build. |
|
@akoeplinger @marek-safar @vargaz - is this getting fixed in the runtime? It doesn't make sense to me to fix this assembly when any other assembly can use this same C# feature. Or some new change can add back a |
|
Right now on wasm we support this in aot+interpreter mode by falling back to the interpreter, but can't really support it in aot only mode. |
If we want that, we should add a rule/analyzer to ensure people aren't using them. |
|
The C# "when" keyword was specifically introduced to improve post-mortem diagnostic. The exception filters do not unwind the stack and so it is possible to get more information from the crash-dumps for unhandled exceptions that way. I believe that there was even a rule in some version of fxcop that was looking for places that should use exception filters, but are not. Undoing use of exception filters sounds like a step backward to me. I understand that filters are hard to support with the correct semantics in current Mono AOT. I do not think it is impossible, it is just a lot of work that we do not want to do right now. I think falling back for interpreter for methods with filters is fine workaround for now. |
+1 |
|
We should avoid using exceptions for control flow (it has a bad perf impact even without exception filters) and this one should be very easy to fix. The whole try/catch can be removed for netcore build because the workaround is needed for .net framework only. |
|
The two assemblies which compile this file don't build for netcore. They only build for netstandard and net461. To remove the workaround we will have to add a TFM target for these assemblies. |
|
@eerhardt is it worth adding an extra configuration for fixing this? In my opinion it does not meet the bar to get extra asset in package and ApiCompat risk for the new added TFM. But since this try/catch block is redundant for .net core, we could either wait until Xamarin also has pattern matching and then remove the block all together. Or we could add a TFM target to fix for .net framework, I would go with the former approach. |
|
We are very likely to add configurations that target newer netcoreapp versions for most assemblies, to take advatange of new APIs in the core platform. If there is a reason to add target for newer runtime, just add it. I do not think it is worth it to try to be selective about it. Optimize for future, not for the past. |
|
Would we also need to add a newer netcoreapp version when we enable nullable annotations in these assemblies? |
|
Closing this PR as per above discussion above. What we want is to add The try-catch block will remain for older frameworks (i.e. netstandard). Filed issue: #50439 |
ghost
locked as resolved and limited conversation to collaborators
Context
When using this code in Xamarin apps targeting iOS or watchOS, the code failed when bitcode was enabled since a catch exception with a when block is not allowed/bitcode compliant.
In order to get this code working on bitcode enabled apps (all apps running in watchOS must have bitcode enabled), this PR removes the when and instead moves the condition into the catch block.
The same scenario also happened in other parts of the codebase. See mono/mono#19451 for example.
Example Stacktrace