Fix re-entrancy when marking nested functions #2907
Conversation
vitek-karas
left a comment
vitek-karas
left a comment
There was a problem hiding this comment.
I actually coded the simple fix during the day, but didn't manage to send a PR for it :-). Anyway, I added some more tests - maybe you could merge the tests with this change: https://github.com/vitek-karas/linker/tree/FixSOInCompilerGeneratedCodeMarking
Adjust comments, clarify lack of cache
|
I almost made the PR yesterday but wanted to think a bit more about it :). Thanks for the extra tests! |
| // This should only be called behind a check to IsProcessed for the method or corresponding user method, | ||
| // to avoid recursion. |
There was a problem hiding this comment.
Yes - I considered adding an assert originally but didn't want to do a potentially expensive operation with side effects in debug mode. But it's probably worth it to have the assert. We should get test coverage for release mode in the official build so it's probably fine.
There was a problem hiding this comment.
Side-effects? I don't really see anything here which should cause side effects, but I could be wrong.
There was a problem hiding this comment.
Oh sorry - I meant that it could change the order in which we build the compiler-generated code maps.
There was a problem hiding this comment.
True - on the other hand if there's a bug in that.. we have a bigger problem :-)
test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeAccessedViaReflection.cs
Outdated
Show resolved
Hide resolved
- Add asserts - Fix method name
This fixes endless recursion introduced by the change which scans compiler-generated methods as a group. This change fixes the re-entrancy instead of putting a temporary value in the cache, because this will make it easier to fix the behavior of CheckRequiresReflectionMethodBodyScanner to not consider type parameter annotations. Commit migrated from dotnet/linker@5809152
2 participants
Fixes #2903
I tried to actually fix the re-entrancy instead of putting a temporary value in the cache, because this will make it easier to fix the behavior of
CheckRequiresReflectionMethodBodyScannerto not consider type parameter annotations.