Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix System.Diagnostics.StackTrace version in stack trace helper code #7383

Merged
merged 1 commit into from
Sep 28, 2016
Merged

Fix System.Diagnostics.StackTrace version in stack trace helper code #7383

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/mscorlib/src/System/Diagnostics/Stacktrace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ internal void InitializeSourceInfo(int iSkip, bool fNeedFileInfo, Exception exce
if (s_symbolsMethodInfo == null)
{
s_symbolsType = Type.GetType(
"System.Diagnostics.StackTraceSymbols, System.Diagnostics.StackTrace, Version=4.0.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a",
"System.Diagnostics.StackTraceSymbols, System.Diagnostics.StackTrace, Version=4.0.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right change? I know you are fixing the version to be the one that Roslyn brought in but is using the version of a Roslyn dependency the right thing to do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically we want to probe for the lowest version and let the loader pick a newer version if that's what is available. That's what will happen here, right @gkhanna79?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericstj I am trying to understand what is the right policy/approach here and not dictate one :) Is S.D.ST expected to come only with Roslyn? If so, then this looks like the right approach (assuming we want to keep the lowest version). Otherwise, this needs to be thought through more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as Roslyn isn't removed from the shared framework (though I've heard it has been discussed), it shouldn't matter. With this change coreclr doesn't depend on the version of S.D.ST because even if Roslyn references a newer version, the coreclr support will still work.

/cc: @weshaggard

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ericstj that we should always try to load the lowest version of the assembly that actually contains this type. As for whether or not it is in the shared framework or not that is a different question, and the code here seems to correctly no-op if the assembly isn't found.

throwOnError: false);

if (s_symbolsType == null)
Expand Down