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

[x86/Linux] Initial patch to restore EBP for funclet#9300

Closed
seanshpark wants to merge 1 commit into
dotnet:masterfrom
seanshpark:ebp4funclet
Closed

[x86/Linux] Initial patch to restore EBP for funclet#9300
seanshpark wants to merge 1 commit into
dotnet:masterfrom
seanshpark:ebp4funclet

Conversation

@seanshpark
Copy link
Copy Markdown

@seanshpark seanshpark commented Feb 3, 2017

This is a initial patch to restore EBP and Exception object for funclets
With this patch it fixes lots of try-catch fails like PinObj-neg.exe
Further changes maybe needed for nested try-catch.

@seanshpark
Copy link
Copy Markdown
Author

Related #9245 with help and draft code from @parjong / CC @wateret .

Comment thread src/jit/codegencommon.cpp
getEmitter()->emitIns_R_AR(INS_lea, EA_PTRSIZE, REG_FPBASE, REG_FPBASE, -8);
// mov eax, [esp + 12] ; exception object to eax: funclet needs eax having the exception object
getEmitter()->emitIns_R_AR(INS_mov, EA_PTRSIZE, REG_EAX, REG_SPBASE, 12);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be better to use REG_EXCEPTION_OBJECT instead of REG_EAX (although they are same).

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.

OK, thanks!

Comment thread src/jit/codegencommon.cpp

// Save callee-saved registers
genPushCalleeSavedRegisters();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be better to invoke genPushCalleeSavedRegisters in the middle of unwindBegProlog and unwindEndProlog

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.

Problem is that it will change the ESP, now knowing the amount, so EBP restore offset and also for EAX should have calculated offset not fixed 8 and 12. If genPushCalleeSavedRegisters must be in between, I need to find another way to restore EBP and EAX.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Or, we may skip callee-saved registers save/restore (as discussed in #9245).

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.

OK, remove for now :)

Comment thread src/jit/codegencommon.cpp
* |-----------------------|
* | Outgoing arg space | // this only exists if the function makes a call
* |-----------------------|
* | | |
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There may be no outgoing arg space as FEATURE_FIXED_OUT_ARGS is not set for x86.

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.

It's for spaces when calling other methods, if any, inside funclets with "push" instruction.

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.

I think this block can be removed as you noted.

This is a initial patch to restore EBP and Exception object for funclets
With this patch it fixes lots of try-catch fails like PinObj-neg.exe
Further changes are needed for nested try-catch.
Comment thread src/jit/codegencommon.cpp
*
* Funclets have the following incoming arguments:
*
* catch/filter-handler: ebp+8 = CallerSP, ebp+12 = the exception object that was caught (see GT_CATCH_ARG)
Copy link
Copy Markdown
Member

@jkotas jkotas Feb 3, 2017

Choose a reason for hiding this comment

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

This calling convention for funclets looks odd. Was it inspired by something?

I think the natural calling convention for funclets would be:

ecx = establisher frame
edx = exception object
ebp, esi, edi, ebx = restored to what they were when the control flow left the method last time

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It seems that the following code in exceptionhandling.cpp invokes funclets:

   //
    // Invoke the funclet.
    //
    dwResumePC = pfnHandler(sf.SP, OBJECTREFToObject(throwable));

pfnHandler is of type HandlerFn *:

src/ $ grep -Rn 'pfnHandler'
vm/exceptionhandling.cpp:3241:    HandlerFn*          pfnHandler = (HandlerFn*)uHandlerStartPC;
src/ $ grep -Rn 'HandlerFn'
...
vm/exceptionhandling.h:35:typedef DWORD_PTR   (HandlerFn)(UINT_PTR uStackFrame, Object* pExceptionObj);

There is no calling convention annotation on HandlerFn which means that CDECL.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we introduce CallEHFunclet and CallEHFilterFunclet for x86 as in ARM/ARM64?

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.

I think so.

@seanshpark
Copy link
Copy Markdown
Author

I'm closing this issue as @wateret has started with #9489

@seanshpark seanshpark closed this Feb 10, 2017
@seanshpark seanshpark deleted the ebp4funclet branch March 2, 2017 02:22
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
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.

5 participants