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

Show the expected stack trace from a rethrown exception. #16464

Merged
danmoseley merged 2 commits into from
Feb 21, 2018
Merged

Show the expected stack trace from a rethrown exception. #16464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4205815
Show the expected stack trace from a rethrown exception.
ateoi Feb 20, 2018
d938b0c
Remove now unused methods
ateoi Feb 21, 2018
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
10 changes: 0 additions & 10 deletions src/vm/clrex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,6 @@ struct StackTraceElement
{
return !(*this == rhs);
}

bool PartiallyEqual(StackTraceElement const & rhs) const
{
return pFunc == rhs.pFunc;
}

void PartialAtomicUpdate(StackTraceElement const & rhs)
{
ip = rhs.ip;
}
};

class StackTraceInfo
Expand Down
4 changes: 1 addition & 3 deletions src/vm/excep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2428,9 +2428,7 @@ void StackTraceInfo::SaveStackTrace(BOOL bAllowAllocMem, OBJECTHANDLE hThrowable
}
}

if (bSkipLastElement && gc.stackTrace.Size() != 0)
gc.stackTrace.AppendSkipLast(m_pStackTrace, m_pStackTrace + m_dFrameCount);

@jkotas jkotas Feb 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AppendSkipLast seems to be unused now. Delete it?

@jkotas jkotas Feb 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also: StackTraceElement.PartiallyEqual/PartialAtomicUpdate

@ateoi ateoi Feb 21, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Pushed d938b0c to remove them.

else
if (!bSkipLastElement)

@danmoseley danmoseley Feb 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Despite reading the code, I cannot form a picture of what is going on here. What is the purpose of bSkipLastElement ?

@ateoi ateoi Feb 21, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

From my analysis, this flag is set during a rethrown exception handling.

coreclr/src/vm/exceptionhandling.cpp

Lines 2555 to 2558 in 591459f

if (STS_FirstRethrowFrame == STState)
{
bSkipLastElement = TRUE;
}

gc.stackTrace.Append(m_pStackTrace, m_pStackTrace + m_dFrameCount);

//////////////////////////////
Expand Down
49 changes: 0 additions & 49 deletions src/vm/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1970,55 +1970,6 @@ void StackTraceArray::Append(StackTraceElement const * begin, StackTraceElement
#endif
}

void StackTraceArray::AppendSkipLast(StackTraceElement const * begin, StackTraceElement const * end)
{
CONTRACTL
{
THROWS;
GC_TRIGGERS;
MODE_COOPERATIVE;
PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)this));
}
CONTRACTL_END;

// to skip the last element, we need to replace it with the first element
// from m_pStackTrace and do it atomically if possible,
// otherwise we'll create a copy of the entire array, which is bad for performance,
// and so should not be on the main path
//

// ensure that only one thread can write to the array
EnsureThreadAffinity();

assert(Size() > 0);

StackTraceElement & last = GetData()[Size() - 1];
if (last.PartiallyEqual(*begin))
{
// fast path: atomic update
last.PartialAtomicUpdate(*begin);

// append the rest
if (end - begin > 1)
Append(begin + 1, end);
}
else
{
// slow path: create a copy and append
StackTraceArray copy;
GCPROTECT_BEGIN(copy);
copy.CopyFrom(*this);
copy.SetSize(copy.Size() - 1);
copy.Append(begin, end);
this->Swap(copy);
GCPROTECT_END();
}

#if defined(_DEBUG)
CheckState();
#endif
}

void StackTraceArray::CheckState() const
{
CONTRACTL
Expand Down
1 change: 0 additions & 1 deletion src/vm/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2910,7 +2910,6 @@ class StackTraceArray
StackTraceElement & operator[](size_t index);

void Append(StackTraceElement const * begin, StackTraceElement const * end);
void AppendSkipLast(StackTraceElement const * begin, StackTraceElement const * end);

I1ARRAYREF Get() const
{
Expand Down