Add PreserveDependency to StackFrameHelper #32023
Conversation
|
FYI @marek-safar: From a correctness perspective this is a good change, but it may come with non-trivial size cost. |
|
I am not concerned as this is coreclr specific part but could https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.StackTrace/src/ILLinkTrim.xml be now removed? Alternatively, think about folding System.Diagnostics.StackTrace into System.Private.CoreLib to avoid all complexity caused by external assembly with 2 types. |
There was a problem hiding this comment.
Is the per-assembly linking used by CoreFX build going to keep the type around without this?
There was a problem hiding this comment.
I think it will as libraries are built with coreclr by default for packaging but this raises good point about handling runtime specific dependencies during libraries illinker optimization pass.
There was a problem hiding this comment.
The failures in System.Tests.ExceptionTests.VerifyCallStack tests that are popping up in the CI suggest that the type is not kept around.
There was a problem hiding this comment.
good catch - I reverted the deletion of ILLinkTrim.xml.
There was a problem hiding this comment.
I may be missing something, but I don't see how this is going to solve the reported issue. It looks to me like MarkUserDependency in the linker's MarkStep will also only look in the context's assembly cache, not add new assemblies to the closure. Did you confirm whether this works?
edit: Never mind, I forgot about the earlier step that adds assemblies referenced from these attributes to the closure. Thanks!
While linker can recognize the hardcoded type name in the InitializeSoruceInfo it sometimes fails to resolve the type as the System.Diagnostics.StackTrace is not directly referenced by most assemblies. This is partially an internal linker limitation and the attribute is a workaround. That said linker would not be able to recognize the GetMethod or CreateInstance calls which follow and in member-level trimming mode would not correctly keep the GetSourceLineInfo nor the .ctor of the type.
This reverts commit c554f2c392a46b5d65f49a304e1ed15e1b91b852.
vitek-karas
force-pushed
the
StackFrameHelperPreserveDep
branch
from
c554f2c to
98b8015
Compare
|
The test failure is infrastructure issue: The test succeeded, but reporting the results back through helix failed. |
ghost
locked as resolved and limited conversation to collaborators
4 participants
While linker can recognize the hardcoded type name in the
InitializeSoruceInfoit sometimes fails to resolve the type as theSystem.Diagnostics.StackTraceis not directly referenced by most assemblies. This is partially an internal linker limitation and the attribute is a workaround.That said linker would not be able to recognize the
GetMethodorCreateInstancecalls which follow and in member-level trimming mode would not correctly keep theGetSourceLineInfonor the.ctorof the type.This comes with a tradeoff for small apps. Console Hello World gains almost 200K in size when trimmed aggressively (member-level trimming), because the
System.Diagnostics.StackTracehas dependencies onSystem.IOandSystem.Reflection.Metadata. But without this change linker effectively breaks the functionality of showing source file information in stack traces.See dotnet/sdk#4017 for customer issue.