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

Hide post exception stack frames #14652

Merged
merged 2 commits into from
Oct 23, 2017
Merged

Hide post exception stack frames #14652

merged 2 commits into from
Oct 23, 2017

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Oct 22, 2017

Follow on from #14564, #14608

Hides from stack trace:

   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() 
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 
   at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task) 
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult() 

/cc @stephentoub @noahfalk @jkotas PTAL

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@tmat
Copy link
Member

tmat commented Oct 22, 2017

Actually, doesn't this make foreach (var frame in new StackTrace(e).GetFrames()) { ... } inconsistent with Exception.ToString()?

@benaadams
Copy link
Member Author

benaadams commented Oct 22, 2017

Yep; you get everything if you are processing the frames; nothing is hidden from you (unless its in the Diagnostics namespace)

@benaadams
Copy link
Member Author

Changing the frames would probably be a horribly breaking change; I wasn't even intending on changing the frames for the prettification of callers - just adding additional detail for the frames; and using it in .ToString

@tmat
Copy link
Member

tmat commented Oct 22, 2017

@benaadams I'm not sure why it would be a breaking change. As @stephentoub pointed out the frames may get inlined already or internal refactoring may change them.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks, @benaadams.

@benaadams
Copy link
Member Author

I'm not sure why it would be a breaking change.

Tools (could be a structured logger or analysis tool) will process the frames to determine what is being executed; and to extract further detail; don't want to lie to them and they can use the same method to hide (or later resolve) if they choose; as they can the the info from reflection.

Processing the textual .ToString() output with a tool is dodgy to begin with; and is normally processed by people (although its currently fairly confusing so people also use tools to make it less so).

The goal I have been aiming for is so someone can call .ToString() on an exception and get an output that they can relate to in terms of the code they think is running; with the names and parameters they understand as a developer (including framework code).

.ToString() doesn't output the asm call stack; or the il callstack; it produces the C# callstack as the user would perceive it from their code until you hit linq (iterators), a lambda, local function, iterator or async when you get a strange halfway house where the call stack is suddenly nothing like the code you wrote or called.

The line numbers via PDB match back(ish) to the line numbers in the source code but the function names are wacky. Its what the actually executing function are called post compile - but its still a compilation detail.

So ideally I want to resolve it back to the source code's function names (including the framework source).

In javascript land they have .map files; so if you write one of the transpiled languages typescript, coffeescript etc you get back stack traces in that rather than the rather weird javascript the transpiling produces - so its a similar goal that other ecosystems already have.

Also to a new user hitting an error using any of these powerful language features; outside the debugger, Exception.ToString() is probably their first point of call and it currently isn't a "pit of success"


namespace System.Diagnostics
{

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Extra new line

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

namespace System.Diagnostics
{

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Constructor | AttributeTargets.Struct, Inherited = false)]
Copy link
Member

Choose a reason for hiding this comment

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

Nothing is looking for this attribute on properties today - it may be better to omit AttributeTargets.Property.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

@tmat
Copy link
Member

tmat commented Oct 23, 2017

Tools (could be a structured logger or analysis tool) will process the frames to determine what is being executed; and to extract further detail; don't want to lie to them and they can use the same method to hide (or later resolve) if they choose; as they can the the info from reflection.

Processing the textual .ToString() output with a tool is dodgy to begin with; and is normally processed by people (although its currently fairly confusing so people also use tools to make it less so).

Fair enough. However, since this change is making .ToString() inconsistent with StackTrace.GetFrames() users might be confused because .ToString() shows them something else than another 3rd party stack formatting library, or logger. Perhaps this is just the first step that makes the default formatting better. I'm just worried about adding more inconsistency to the system - ToString shows you something, StackTrace shows you something else and the VS debugger shows you yet another view of the stack trace. So which one is it?

The goal I have been aiming for is so someone can call .ToString() on an exception and get an output that they can relate to in terms of the code they think is running; with the names and parameters they understand as a developer (including framework code).

.ToString() doesn't output the asm call stack; or the il callstack; it produces the C# callstack as the user would perceive it from their code until you hit linq (iterators), a lambda, local function, iterator or async when you get a strange halfway house where the call stack is suddenly nothing like the code you wrote or called.

.ToString() actually does print out the IL (or rather metadata) call stack. Let's look at a trivial thing like generic type names. Currently the stack trace prints out List`1 as a name of the type. That's not a name of a type in C#. In C# it's List<T>. In VB it's List(Of T).

So which one should it print out? What if there are frames from multiple languages on the stack? Should it show everything in format of the language it was invoked from or should it rather format each frame according to the language the method was written in?

It would be certainly nice to bring ToString closer to the source code, but since the CLR is multi-lingual runtime it needs to work in terms of Common Language Infrastructure entities. It should not assume one specific language or a hard-coded subset of languages. We can introduce some abstractions that compilers can use to add more metadata to the assemblies to help the CLR to display the stack traces better without hard coding implementation details of each language into the runtime. Some of these abstractions are already available -- DebuggerHidden, DebuggerNonUserCode, etc. attributes. Maybe we need a few more. But we need to think carefully about each one.

Another possible approach would be to leave ToString as is -- display the CLR view of the stack trace. And provide a higher-level stack printing library that is extensible and allows any .NET language to contribute their custom stack formatter.

The line numbers via PDB match back(ish) to the line numbers in the source code but the function names are wacky. Its what the actually executing function are called post compile - but its still a compilation detail.

The PDB is not just a pile of line number mappings. It associates the MoveNext method with the kickoff method exactly. See https://github.com/dotnet/corefx/blob/master/src/System.Reflection.Metadata/specs/PortablePdb-Metadata.md#StateMachineMethodTable for Portable PDB and https://github.com/dotnet/symreader/blob/master/src/Microsoft.DiaSymReader/Reader/ISymUnmanagedAsyncMethod.cs#L18 for Windows PDB.

In javascript land they have .map files;

In .NET land we have .pdb files for the same purpose.

@jkotas jkotas merged commit c679f88 into dotnet:master Oct 23, 2017
dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Oct 23, 2017
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Oct 23, 2017
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@benaadams
Copy link
Member Author

However, since this change is making .ToString() inconsistent with StackTrace.GetFrames() ... Perhaps this is just the first step that makes the default formatting better.

Hopefully; maybe it could be better phrased as .ToString is easier to argue that it won't cause compat breaks than StackFrame?

just worried about adding more inconsistency to the system - ToString shows you something, StackTrace shows you something else and the VS debugger shows you yet another view of the stack trace.

Its bringing ToString closer to VS debugger

The PDB is not just a pile of line number mappings. It associates the MoveNext method with the kickoff method exactly. See ...

Whoa... now we are cooking with gas... 😄

A-And pushed a commit to A-And/corert that referenced this pull request Oct 31, 2017
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@aelij
Copy link

aelij commented Dec 22, 2017

@stephentoub Could this be labeled as netfx-port-consider?

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Jan 13, 2018
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Jan 13, 2018
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
safern pushed a commit to dotnet/corefx that referenced this pull request Jan 16, 2018
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
safern pushed a commit to dotnet/corefx that referenced this pull request Jan 16, 2018
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
@benaadams benaadams deleted the part-2 branch April 13, 2019 00:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants