StackTraceFormatting{Options} by benaadams · Pull Request #14650 · dotnet/coreclr

benaadams · 2017-10-22T07:35:43Z

Clean follow up to #14608

Adds extra .ctor overload + param to DebuggerNonUserCodeAttribute

[Flags]
internal enum StackTraceFormattingOptions
{
    None                   = 0,
    StackTraceHidden       = 1 << 0
}

public partial class DebuggerNonUserCodeAttribute
{
    /// <summary>Initializes the <see cref="DebuggerNonUserCodeAttribute"/>.</summary>
    /// <param name="stackTraceOptions">The <see cref="StackTraceFormattingOptions"/> hint.</param>
    internal DebuggerNonUserCodeAttribute(StackTraceFormattingOptions stackTraceOptions)
    {
        StackTraceOptions = stackTraceOptions;
    }

    /// <summary>Gets the <see cref="StackTraceFormattingOptions"/>.</summary>
    internal StackTraceFormattingOptions StackTraceOptions { get; }
}

Adds StackTrace configuration overrides (for back-compat/runtime switching)

[Flags]
internal enum StackTraceFormatting
{
    None = 0,
    ExcludeStackTraceHidden   = 1 << 0,
    ExcludeDispatchBoundaries = 1 << 1
}

public partial class StackTrace
{
    internal static StackTraceFormatting FormattingOptions { get; set; }
            = StackTraceFormatting.ExcludeStackTraceHidden |
              StackTraceFormatting.ExcludeDispatchBoundaries;
}

As shown in #14564 similar can be achieved by altering the call chain and the Jit inlines sometimes and doesn't others so what's seen varies - so would only be breaking change if a dependency was taken on the names of private functions in coreclr and the order in which they were called; and then depending on it via Exception.ToString() - at which point pretty much all changes (i.e. internal implementation changes) become breaking?

/cc @tmat @JamesNK @aelij @stephentoub @noahfalk @jkotas PTAL

benaadams · 2017-10-22T07:36:53Z

src/mscorlib/src/System/Diagnostics/DebuggerAttributes.cs

+
+        /// <summary>Initializes the <see cref="DebuggerNonUserCodeAttribute"/>.</summary>
+        /// <param name="stackTraceOptions">The <see cref="StackTraceFormattingOptions"/> hint.</param>
+        internal DebuggerNonUserCodeAttribute(StackTraceFormattingOptions stackTraceOptions)


Would need corresponding change in corert as not in shared partition (or moving to shared partition and type in corert removed)

benaadams · 2017-10-22T08:02:35Z

More terse stack trace output

System.Exception: Inner1 ---> System.Exception: Inner2 ---> System.Exception: Inner3 ---> System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.List`1.Enumerator.MoveNextRare() 
   at System.Collections.Generic.List`1.Enumerator.MoveNext() 
   at Program.FirstException(Int32 value, Int32 index)
   at Program.<>c.<LinqIteration>b__4_0(Int32 x, Int32 i)
   at System.Linq.Enumerable.<SkipWhileIterator>d__179`1.MoveNext()
   at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
   at System.Collections.Generic.EnumerableHelpers.ToArray[T](IEnumerable`1 source)
   at Program.LinqIteration(Int32 count)
   at Program.<Iterator>d__5.MoveNext()
   at Program.InnerException3()
   --- End of inner exception stack trace ---
   at Program.InnerException3()
   at Program.InnerType`1.InnerException2(Int32 count)
   --- End of inner exception stack trace ---
   at Program.InnerType`1.InnerException2(Int32 count)
   at Program.InnerException1[T](Int32 count)
   --- End of inner exception stack trace ---
   at Program.InnerException1[T](Int32 count)
   at Program.NonAsyncValueTask(Int32 count, String name)
   at Program.<ValueTaskAsync>d__10.MoveNext()
   at Program.<TaskTAsync>d__12.MoveNext()
   at Program.<MethodAsync>d__15.MoveNext()
   at Program.<ConfiguredAsync>d__14.MoveNext()
   at Program.<MethodAsync>d__15.MoveNext()
   at Program.<ConfiguredAsync>d__14.MoveNext()
   at Program.<MethodAsync>d__15.MoveNext()
   at Program.<Main>d__1.MoveNext()

Rather than the less terse current version

benaadams · 2017-10-22T11:58:16Z

Added api request to make the changes public and for wider use https://github.com/dotnet/corefx/issues/24796

benaadams · 2017-10-22T12:40:58Z

Corert change for moving (some) Debugger attributes to shared partition dotnet/corert#4774

tmat · 2017-10-22T16:41:41Z

A couple of things I'm not fan of:

the formatting options in a static field
extra parameter to DebuggerNonUserCodeAttribute

I'd prefer the following API:

namespace System.Diagnostics
{
  [Flags]
  public enum StackTraceFormat
  {
     Default = 0,
     ExcludeNonUserCode = 1 << 0,
     ExcludeDispatchBoundaries = 1 << 1    
  }

  public class StackTrace
  {
     // New overload:
     public string ToString(StackTraceFormat format) { ... }

     // Existing overload - no change in behavior
     public string ToString() => ToString(StackTraceFormat.Default);
  }
}

and maybe:

namespace System
{
  public class Exception
  {
     string ToString(StackTraceFormat format);
  }
}

Although one can get a stack trace from exception like so: new StackTrace(e).ToString(), the list of inner exception messages would need to be included manually.

aelij · 2017-10-22T16:51:35Z

@tmat

the formatting options in a static field

But then there wouldn't be a way to set the default, which is used by Exception.ToString(), and during exception serialization (stack trace is serialized as a string).

benaadams · 2017-10-22T16:54:24Z

I'd prefer the following API:

With

ToString() => ToString(StackTraceFormat.ExcludeNonUserCode | StackTraceFormat.ExcludeDispatchBoundaries)

Then would be a breaking(ish) change on DebuggerNonUserCodeAttribute

If its only for the string overload; then it would need an additional Exception.ToString(StackTraceFormat format); also be opt-in - so back to not being effective for the 95% case; so the only people that it will be effective for are those people probably already processing their stack traces and your common or garden dev will still be thinking wtf when looking at their logs.

tmat · 2017-10-22T16:55:07Z

@benaadams I'm not proposing that. My proposal is: ToString() => ToString(StackTraceFormat.Default)

tmat · 2017-10-22T16:56:32Z

@aelij I have updated my comment to add Exception.ToString(format). For serialization -- not sure, but perhaps we need something serialization specific. But I'd rather avoid global state that affects everything.

tmat · 2017-10-22T16:58:32Z

@benaadams Updated my proposal to make it clear.

benaadams · 2017-10-22T16:58:33Z

@tmat doesn't that render this change largely pointless? The aim is to change the default behaviour

tmat · 2017-10-22T17:00:25Z

@benaadams No it's not pointless. I don't think we can change the default behavior without breaking things. Frameworks that communicate errors to users that include stack traces can use the new API (e.g. test frameworks, loggers, etc.).

tmat · 2017-10-22T17:01:31Z

Also, we can start with explicit API and then add global App.config switch that allows users to opt-in (change the global default) without changing their app code.

stephentoub · 2017-10-22T17:02:08Z

I don't think we can change the default behavior without breaking things.

What are you concerned about breaking? Relying on the exact frames that show up in a stack trace string from an exception is super brittle, changes based on optimizations applied, etc.

benaadams · 2017-10-22T17:04:56Z

I don't think we can change the default behavior without breaking things.

As shown in #14564 I can erase all these stack traces anyway by changing the code where the exception is thrown (rather than the stack traces). So saying it can't be changed means no new private functions can be renamed or way they call each other changed.

As a lot of the functions flow have already changed; using span and memory overloads internally, those would then also be breaking changes by this definition.

tmat · 2017-10-22T17:05:58Z

Someone will be broken for sure. If we decide we don't care about those cases that's fine. The value of the default is an orthogonal discussion. Let's add an explicit API first then we can discuss what the default should be.

stephentoub · 2017-10-22T17:07:56Z

Someone will be broken for sure.

More so than from various refactorings? Why?

I believe the answer is "no", given that Ben already demonstrated he can achieve the same output just by refactoring.

Let's add an explicit API first then we can discuss what the default should be.

Why? As you say, they're orthogonal. I don't see why we need to hold up one over the other.

tmat · 2017-10-22T17:13:58Z

More so than from various refactorings? Why?

Because the attribute is not applied only on internal helpers we change, but users may have it on other code, including public API and methods marked with no inlining as well.

Why? As you say, they're orthogonal. I don't see why we need to hold up one over the other.

Fair point. We can indeed discuss in parallel, just wanted to separate one from the other. Explicit API seems to be useful and less controversial to me.

stephentoub · 2017-10-22T17:17:34Z

Because the attribute is not applied only on internal helpers we change

Not if we use the internal only attribute for now, like the previous version had. That's the one we should go with for now, and stop complicating this. If public API is exposed, a different attribute can be used then.

tmat · 2017-10-22T17:20:57Z

@stephentoub OK, I guess I missed the point of this change. If you just want to hide a few internal Framework methods then we can do whatever. I though we are designing something applicable more generally. If that's not the case then I don't think why we need to have any formatting options. Just put the attribute on the helpers and filter them out without any options.

benaadams · 2017-10-22T17:21:07Z

Because the attribute is not applied only on internal helpers we change, but users may have it on other code, including public API and methods marked with no inlining as well.

Which is why it should either be a different attribute or an additional parameter.

The issue isn't non-user code stack frames; its stack frames that are not part of the error.

This code isn't run until after the exception is thrown; its nothing to do with the error - it helps nothing with diagnostics - it is literally noise:

   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()

This code is after the error, its just factored out into a separate throw helper to avoid bloating the generic code

   at System.ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumFailedVersion()

Is all post-error frames which are being filtered out

tmat · 2017-10-22T17:23:35Z

@benaadams If you are only trying to address the issue with async then why do we need an attribute at all? Can't we just hide all frames from TaskAwaiter type?

benaadams · 2017-10-22T17:25:06Z

Also hiding

System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()

and anything in the ThrowHelper class

benaadams · 2017-10-22T17:28:02Z

Not if we use the internal only attribute for now, like the previous version had. That's the one we should go with for now, and stop complicating this.

Will revert

tmat · 2017-10-22T17:28:03Z

The same thing. That can be all easily hard coded in a helper method that checks for 3 declaring types.

tmat · 2017-10-22T17:31:30Z

BTW: http://source.roslyn.io/#Microsoft.CodeAnalysis.Scripting/Hosting/CommonMemberFilter.cs

benaadams · 2017-10-22T17:40:12Z

8 types

ConfiguredValueTaskAwaitable<TResult>.ConfiguredValueTaskAwaiter
ValueTaskAwaiter<TResult>
TaskAwaiter
TaskAwaiter<TResult>
ConfiguredTaskAwaitable.ConfiguredTaskAwaiter
ConfiguredTaskAwaitable<TResult>.ConfiguredTaskAwaiter
ExceptionDispatchInfo
ThrowHelper

Also for the awaiters its only certain methods (the ones that happen post-exception) that should be filtered.

BTW: http://source.roslyn.io/#Microsoft.CodeAnalysis.Scripting/Hosting/CommonMemberFilter.cs

Its already missing some for ValueTask; attribute is less fragile

tmat · 2017-10-22T18:08:02Z

@benaadams Fair enough, internal attribute distinct from DebuggerNonUserCodeAttribute is a fine solution that addresses the very specific task of removing these helper frames from stack trace strings.
However, it won't help anyone who needs to do their stack frame processing, nor it will help the debugger to identify these members and hide them when JMC is on.

Perhaps we can mark the methods/types by both attributes.

I was hoping though that we could add public API on StackTrace that would remove all "non user code" frames, so that this stack clean up isn't limited only to CoreFX libraries. I guess we can pursue that another time.

tmat · 2017-10-22T18:10:20Z

Its already missing some; attribute is less fragile

It's missing those that were added after this code was written. Having the special attribute won't fix it for scripting, since this code is not in CoreFX thus can't use an internal attribute.

benaadams · 2017-10-22T18:25:28Z

I was hoping though that we could add public API on StackTrace that would remove all "non user code" frames, so that this stack clean up isn't limited only to CoreFX libraries. I guess we can pursue that another time.

API request might be a good place to follow up https://github.com/dotnet/corefx/issues/24796

benaadams · 2017-10-22T18:38:12Z

StackTraceHidden PR #14652

StackTraceFormatting{Options}

fb4a4f8

dnfclas added the cla-already-signed label Oct 22, 2017

benaadams commented Oct 22, 2017

View reviewed changes

Use named params

1a71e33

benaadams mentioned this pull request Oct 22, 2017

More readable stack traces (StackTrace method opt-out) #14608

Closed

Move (most) Debugger Attributes to shared partition

4e8e72a

benaadams force-pushed the stacktrace2 branch from 00fc7f0 to 4e8e72a Compare October 22, 2017 12:39

benaadams mentioned this pull request Oct 22, 2017

Debugger Attributes in shared partition dotnet/corert#4774

Closed

benaadams closed this Oct 22, 2017

Conversation

benaadams commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Oct 22, 2017

Uh oh!

benaadams commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelij commented Oct 22, 2017

Uh oh!

benaadams commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

benaadams commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Oct 22, 2017

Uh oh!

benaadams commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

stephentoub commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

benaadams commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

benaadams commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

tmat commented Oct 22, 2017

Uh oh!

benaadams commented Oct 22, 2017

Uh oh!

benaadams commented Oct 22, 2017

Uh oh!

Reviewers

Assignees

benaadams commented Oct 22, 2017 •

edited

Loading

benaadams Oct 22, 2017 •

edited

Loading

benaadams commented Oct 22, 2017 •

edited

Loading

tmat commented Oct 22, 2017 •

edited

Loading

tmat commented Oct 22, 2017 •

edited

Loading

benaadams commented Oct 22, 2017 •

edited

Loading

stephentoub commented Oct 22, 2017 •

edited

Loading

tmat commented Oct 22, 2017 •

edited

Loading

benaadams commented Oct 22, 2017 •

edited

Loading

benaadams commented Oct 22, 2017 •

edited

Loading

tmat commented Oct 22, 2017 •

edited

Loading

benaadams commented Oct 22, 2017 •

edited

Loading