Add ETW enabled checks to ConcurrentBag implementation (case 1230447) #1277
Conversation
This change modifies the `ConcurrentBag` implementation from corefx. Since we cannot update the corefx submodule, make a copy of the ConcurrentBag.cs file, fix the issue, and point the build at this copy. The change is in the `TrySteal` method. It makes calls on the `CDSCollectionETWBCLProvider`, which is removed by the manged linker by default. Those calls should be wrapped in a `CDSCollectionETWBCLProvider.Log.IsEnabled()` if block, so the linker can understand to not make the calls.
|
Wouldn’t it be easier to change the managed linker to make ETW functions no-ops instead of forking class library code like this? It’s likely this isn’t the only place that will fail like this and it’s generally assumed that ETW calls will guard themselves from exceptions inside of them. Before I implemented ETW in class libraries, all those methods were no-ops. After I implemented them, they were still no-ops where not available. Changing that will break any code that accidentally depends on it and there’s a lot of code that calls into ETW in corefx. |
|
This was a the recommended solution from Marek at Microsoft. We may need to audit the corefx code for other places that misbehave by not checking |
Keep in mind that is but one specific ETW provider. There are quite a few in the class libraries. Also, you'll have to do this audit every time we pull from upstream corefx/mono. |
Good point. We've not updated from upstream corefx in two years, I think. So maybe it is not an issue. Also, the upstream linker supports removal of ETW code, so I suspect it will be fixed upstream (Marek said he would fix this specific issue). |
|
FWIW I've looked at the rest of the current corefx code in our fork of Mono. There are four other places where |
|
The support for excluding etw was implemented by Marek in monolinker. I'm not sure why he picked replacing with throws instead of no-ops, but I'm guess he had a reason. If you are curious, I can follow up with him and ask. This is the first bug on it. I'm content to give it a chance before we go investing additional time and energy into this.
Microsoft has really ramped up their investment in monolinker . They are becoming very concerned with the link ability of corefx. I view depending on them to keep excluding etw working as relatively low risk. |
|
Ok, I'll go ahead and merge this change then. |
This change modifies the
ConcurrentBagimplementation from corefx.Since we cannot update the corefx submodule, make a copy of the
ConcurrentBag.cs file, fix the issue, and point the build at this copy.
The change is in the
TryStealmethod. It makes calls on theCDSCollectionETWBCLProvider, which is removed by the manged linkerby default. Those calls should be wrapped in a
CDSCollectionETWBCLProvider.Log.IsEnabled()if block, so the linkercan understand to not make the calls.
Release notes:
Fix case 1230447:
IL2CPP: Correct the implementation of ConcurrentBag so that it works with ETW managed code stripping.
We should back port this change to 2020.1, 2019.3, and 2018.4, since the ETW stripping code is in all of those versions.