Skip to content

Conversation

filipnavara
Copy link
Member

@ghost ghost added the area-VM-coreclr label Apr 1, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 1, 2025
@filipnavara
Copy link
Member Author

The diff is not well matched, it is all just deleted lines.

@filipnavara filipnavara requested a review from janvorli April 1, 2025 21:09
@filipnavara filipnavara changed the title Delete dead code in ExceptionTracker Delete dead code in ExceptionTracker and leftover old EH code Apr 1, 2025
@jkotas jkotas requested a review from mangod9 April 1, 2025 21:24
static bool FindNonvolatileRegisterPointers(Thread* pThread, UINT_PTR uOriginalSP, REGDISPLAY* pRegDisplay, TADDR uResumeFrameFP);
static void UpdateNonvolatileRegisters(T_CONTEXT* pContextRecord, REGDISPLAY *pRegDisplay, bool fAborting);

PTR_Frame GetLimitFrame()
Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed

Copy link
Member Author

Choose a reason for hiding this comment

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

This still seems to be called from ResetThreadAbortState.

Copy link
Member

Choose a reason for hiding this comment

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

That one is not needed either.

Copy link
Member

Choose a reason for hiding this comment

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

For the FEATURE_EH_FUNCLETS

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll submit a follow up PR once this is merged. Rough preview: 7891236


private: ;

void ReleaseResources();
Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bit harder to reason about this since it's not purely unreferenced code.

Copy link
Member

Choose a reason for hiding this comment

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

Basically anything that references non-static methods in the ExceptionTracker is a dead code. No ExceptionTracker instance gets ever created with the new EH. The ExInfo has replaced it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense.

DWORD dwEnclosingClauseOffset,
UINT_PTR uEnclosingClauseCallerSP);

struct EnclosingClauseInfo
Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a rather long usage chain for this going from StackFrameIterator::Filter -> GetCallerOfEnclosingClause.

Copy link
Member

Choose a reason for hiding this comment

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

The StackFrameIterator::Filter contains dead code too. Anything that executes when pTracker is not null.

@filipnavara
Copy link
Member Author

@janvorli FWIW This is not comprehensive. I can address the other things you pointed out but I would prefer to do it separately from just deleting unreferenced code (to keep it reviewable).

@janvorli
Copy link
Member

janvorli commented Apr 1, 2025

I can address the other things you pointed out but I would prefer to do it separately from just deleting unreferenced code (to keep it reviewable).

I am fine with that approach.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli
Copy link
Member

janvorli commented Apr 2, 2025

/ba-g the test failures occur on all PRs recently

@janvorli janvorli merged commit 62f3b4d into dotnet:main Apr 2, 2025
95 of 98 checks passed
@filipnavara filipnavara deleted the exception-tracker-dead-code branch April 2, 2025 19:54
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-ExceptionHandling-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants