Add symbol support helper class for mscorlib's StackTrace. #8670
Conversation
|
@ellismg, @stephentoub @jkotas @noahfalk I addressed all the code review feedback.
|
| @@ -27,6 +32,18 @@ | |||
| <ItemGroup> | |||
| <Compile Include="System\Diagnostics\StackFrameExtensions.cs" /> | |||
| </ItemGroup> | |||
| <ItemGroup Condition="'$(TargetGroup)' == ''"> | |||
| <Compile Include="System\Diagnostics\StackTraceSymbols.CoreCLR.cs" /> | |||
There was a problem hiding this comment.
@weshaggard This project compiles for three target groups. I think we only need this for .NET Core (since CoreCLR will use it) but can you confirm this is the right thing to do?
There was a problem hiding this comment.
I guess I only waiting for feedback from Wes on this issue. I’ve addressed got an ok from the rest of the reviewers.
From: Matt Ellis [mailto:notifications@github.com]
Sent: Wednesday, May 18, 2016 6:26 PM
To: dotnet/corefx corefx@noreply.github.com
Cc: Mike McLaughlin mikem@microsoft.com; Assign assign@noreply.github.com
Subject: Re: [dotnet/corefx] Add symbol support helper class for mscorlib's StackTrace. (#8670)
In src/System.Diagnostics.StackTrace/src/System.Diagnostics.StackTrace.csprojhttps://github.com//pull/8670#discussion_r63810461:
@@ -27,6 +32,18 @@
- -<Compile Include="System\Diagnostics\StackFrameExtensions.cs" />
@weshaggardhttps://github.com/weshaggard This project compiles for three target groups. I think we only need this for .NET Core (since CoreCLR will use it) but can you confirm this is the right thing to do?
—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/8670/files/46ebb77cdef1f2c23a599e5f0dbd2e0cb7fa9d59#r63810461
|
LGTM |
| { | ||
| public class StackTraceSymbols : IDisposable | ||
| { | ||
| Dictionary<IntPtr, Tuple<MetadataReaderProvider, MetadataReader>> _readerCache; |
|
A few nits, otherwise LGTM. |
|
@dotnet-bot test Innerloop Windows_NT Release Build and Test |
|
@dotnet-bot test Innerloop Windows_NT Debug Build and Test |
|
@dotnet-bot test Innerloop CentOS7.1 Debug Build and Test |
|
@dotnet-bot test Innerloop CentOS7.1 Release Build and Test |
|
Windows_NT build failed because you are missing OSGroup from your project reference between System.Diagnostics.StackTrace.csproj and System.Threading.Overlapped.csproj. |
| <ItemGroup Condition="'$(TargetGroup)' == ''"> | ||
| <Compile Include="System\Diagnostics\StackTraceSymbols.CoreCLR.cs" /> | ||
| <ProjectReference Include="..\..\System.Collections\src\System.Collections.csproj" /> | ||
| <ProjectReference Include="..\..\System.Threading.Overlapped\src\System.Threading.Overlapped.csproj" /> |
There was a problem hiding this comment.
Sorry, I don’t understand exactly. Are you saying that I need to exclude building StackTraceSymbols.CoreCLR.cs for the Windows_NT with a conditional like ‘$(OSGroup)’ != ‘Windows_NT’?
We want this code on Windows too.
There was a problem hiding this comment.
Eric, just so I understand as well, the feedback is to guard the P2P reference to System.Threading.Overlapped to happen only when
There was a problem hiding this comment.
If you build locally with build.cmd you'll see the failure.
System.Threading.Overlapped only builds with Windows_NT. You need to add that metadata here to the project reference so that you don't build that project with different global properties. It's more about not introducing a race condition in the build than actually building for Windows. You may not even need the overlapped reference at all, so if you can remove it that'd be better.
|
Mike, can you pick up ellismg/corefx@3fa755c? I worked with Eric to clean up the bin clashes, and we believe this should work. |
| @@ -0,0 +1,34 @@ | |||
| | |||
There was a problem hiding this comment.
Can you just delete this SLN? We usually put slns up a level and they include everything.
There was a problem hiding this comment.
Yes. I didn’t really mean to add it.
From: Matt Ellis [mailto:notifications@github.com]
Sent: Friday, May 20, 2016 11:50 AM
To: dotnet/corefx corefx@noreply.github.com
Cc: Mike McLaughlin mikem@microsoft.com; Assign assign@noreply.github.com
Subject: Re: [dotnet/corefx] Add symbol support helper class for mscorlib's StackTrace. (#8670)
In src/System.Diagnostics.StackTrace/src/System.Diagnostics.StackTrace.slnhttps://github.com//pull/8670#discussion_r64089807:
@@ -0,0 +1,34 @@
+
Can you just delete this SLN? We usually put slns up a level and they include everything.
—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/8670/files/c97f9137ba65fbbaff6850d558df7778d201df70#r64089807
This new helper class uses the portable pdb reader in System.Reflection.Metadata to read the source/line number information for stack frames.
|
Thanks for your time and patience. It works!! From: Matt Ellis [mailto:notifications@github.com] Mike, can you pick up ellismg/corefx@3fa755chttps://github.com/ellismg/corefx/commit/3fa755c1decc94148ee1ab51cfaedeeacf77b05d? I worked with Eric to clean up the bin clashes, and we believe this should work. — |
|
@dotnet-bot test Innerloop OSX Debug Build and Test |
Add symbol support helper class for mscorlib's StackTrace. Commit migrated from dotnet/corefx@77dddc7
This new helper class uses the portable pdb reader in System.Reflection.Metadata
to read the source/line number information for stack frames.