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

[x86/Linux] Port 'TransitionFrame::UpdateRegDisplay'#8964

Merged
janvorli merged 2 commits into
dotnet:masterfrom
parjong:fix/x86_TransitionFrame_UpdateRegDisplay
Jan 20, 2017
Merged

[x86/Linux] Port 'TransitionFrame::UpdateRegDisplay'#8964
janvorli merged 2 commits into
dotnet:masterfrom
parjong:fix/x86_TransitionFrame_UpdateRegDisplay

Conversation

@parjong
Copy link
Copy Markdown

@parjong parjong commented Jan 17, 2017

This commit allows Core CLR to show "Unhandled Exception" message on x86/Linux.

@parjong
Copy link
Copy Markdown
Author

parjong commented Jan 18, 2017

\CC @seanshpark

@parjong
Copy link
Copy Markdown
Author

parjong commented Jan 18, 2017

@janvorli Please take a look.

Comment thread src/vm/i386/cgenx86.cpp
pRD->IsCallerSPValid = FALSE;

pRD->pCurrentContext->Eip = pRD->ControlPC;
pRD->pCurrentContext->Esp = GetSP();
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.

Why is the Eip read from the pRD and the Esp is not? It looks suspicious.

Copy link
Copy Markdown
Author

@parjong parjong Jan 18, 2017

Choose a reason for hiding this comment

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

It seems that stack walker recognizes the target frame using established frame address (unmodified Esp).

When I set pRD->pCurrentContext->Esp same as above, stack walker failed to recognize the target frame (which results in assertion failure).

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.

A have looked into how this should work in detail. The difference between the value returned by GetSP and the value set to pRD->Esp is that the pRD->Esp = GetSP() + cbStackPop. In other words, one represent the SP before pushing the stack arguments at the caller and the other SP after pushing them.
So the pRD->Esp should be set for WIN64EXCEPTIONS to the GetSP too. the pEsp and the pRD->pCurrentContext->Esp need to match.
And in fact, it happens later in the SyncRegDisplayToCurrentContext. So the lines 305 and 306 are not needed for WIN64EXCEPTIONS if you change the line 312 to

pRD->pCurrentContext->Eip = *PTR_PCODE(pRD->PCTAddr);

Could you please make this change and change the lines 305 and 306 to be for !WIN64EXCEPTIONS only?

Comment thread src/vm/i386/cgenx86.cpp
pRD->IsCallerSPValid = FALSE;

pRD->pCurrentContext->Eip = pRD->ControlPC;
pRD->pCurrentContext->Esp = GetSP();
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.

A have looked into how this should work in detail. The difference between the value returned by GetSP and the value set to pRD->Esp is that the pRD->Esp = GetSP() + cbStackPop. In other words, one represent the SP before pushing the stack arguments at the caller and the other SP after pushing them.
So the pRD->Esp should be set for WIN64EXCEPTIONS to the GetSP too. the pEsp and the pRD->pCurrentContext->Esp need to match.
And in fact, it happens later in the SyncRegDisplayToCurrentContext. So the lines 305 and 306 are not needed for WIN64EXCEPTIONS if you change the line 312 to

pRD->pCurrentContext->Eip = *PTR_PCODE(pRD->PCTAddr);

Could you please make this change and change the lines 305 and 306 to be for !WIN64EXCEPTIONS only?

Copy link
Copy Markdown
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 janvorli merged commit 9743c40 into dotnet:master Jan 20, 2017
@parjong parjong deleted the fix/x86_TransitionFrame_UpdateRegDisplay branch January 22, 2017 22:24
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…8964)

* [x86/Linux] Port 'TransitionFrame::UpdateRegDisplay'

* Use different ControlPC/Esp values for WIN64EXCEPTIONS


Commit migrated from dotnet/coreclr@9743c40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants