Show the expected stack trace from a rethrown exception. #16464
Conversation
| } | ||
|
|
||
| if (bSkipLastElement && gc.stackTrace.Size() != 0) | ||
| gc.stackTrace.AppendSkipLast(m_pStackTrace, m_pStackTrace + m_dFrameCount); |
There was a problem hiding this comment.
AppendSkipLast seems to be unused now. Delete it?
There was a problem hiding this comment.
Also: StackTraceElement.PartiallyEqual/PartialAtomicUpdate
|
@ateoi Thanks for fixing this! |
|
Great to see a fix for this. You might want to open a bug in the dotnet/docs repo to update this text for 2.1: |
|
Also, would you be willing to open a PR in the corefx repo to add a test? I am not certain the best place, I don't see tests specifically for what is in Exception.StackTrace. Some possible places are
static int expectedThrowLine;
[Fact]
static void ThrowStatementDoesNotResetExceptionStackLine()
{
try
{
DoStuff();
}
catch (Exception ex)
{
int reportedThrowLine = Convert.ToInt32(Regex.Match(ex.StackTrace, @".*DoStuff[^\n]*line (\d+)", RegexOptions.Multiline).Groups[1].Value);
Assert.Equals(expectedThrowLine, reportedThrowLine);
}
}
private static void DoStuff()
{
try
{
expectedThrowLine = 1 + Convert.ToInt32(Regex.Match(Environment.StackTrace, @".*DoStuff[^\n]*line (\d+)", RegexOptions.Multiline).Groups[1].Value);
throw new Exception("Boom!");
}
catch (Exception)
{
throw;
}
} |
| if (bSkipLastElement && gc.stackTrace.Size() != 0) | ||
| gc.stackTrace.AppendSkipLast(m_pStackTrace, m_pStackTrace + m_dFrameCount); | ||
| else | ||
| if (!bSkipLastElement) |
There was a problem hiding this comment.
Despite reading the code, I cannot form a picture of what is going on here. What is the purpose of bSkipLastElement ?
There was a problem hiding this comment.
From my analysis, this flag is set during a rethrown exception handling.
coreclr/src/vm/exceptionhandling.cpp
Lines 2555 to 2558 in 591459f
|
Our tests for ExceptionDispatchInfo (all in |
- StackTraceArray::AppendSkipLast - StackTraceElement::PartiallyEqual - StackTraceElement::PartialAtomicUpdate
|
Ok, I can open a bug for the doc and submit a PR to add a test. |
|
@jkotas any other feedback? |
|
Oop, we generally wait for the test PR to be up before merging the implementation PR. |
Looks fine to me.
As far as I know, we have no automated tests for the stacktraces, and we have merged number of tweaks of the stacktraces without them (e.g. #14655). It would be a bit unfair to block this one on tests. I agree that we should add tests for this though. |
|
OK |
4 participants
Fix #15780