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

[WIP] [x86/Linux] Call EH Funclet with CallEHFunclet#9489

Closed
wateret wants to merge 1 commit into
dotnet:masterfrom
wateret:wip/x86eh1
Closed

[WIP] [x86/Linux] Call EH Funclet with CallEHFunclet#9489
wateret wants to merge 1 commit into
dotnet:masterfrom
wateret:wip/x86eh1

Conversation

@wateret
Copy link
Copy Markdown
Member

@wateret wateret commented Feb 10, 2017

Implement CallEHFunclet same as ARM/ARM64.'

This follows ARM/ARM64 way for calling non-filter funclets which is actually not the way discussed in #9484. This can handle simple try-catch statements but fails for nested exceptions. (e.g. throw in catch block)

Implement CallEHFunclet same as ARM/ARM64.
@wateret
Copy link
Copy Markdown
Member Author

wateret commented Feb 10, 2017

cc: @seanshpark @parjong @YongseopKim

Comment thread cross/x86/toolchain.cmake
add_compile_options("--sysroot=${CROSS_ROOTFS}")
add_compile_options("-Wno-error=unused-command-line-argument")
add_compile_options("-mstack-alignment=4")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line should be excluded.

Copy link
Copy Markdown
Member 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
inst_RV(INS_push, REG_EBX, TYP_REF);
inst_RV(INS_push, REG_ESI, TYP_REF);
inst_RV(INS_push, REG_EDI, TYP_REF);

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 genPushCalleeSavedRegisters instead of explicitly adding push instructions.

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.

Is there a good reason for saving these registers here? The asm helper that calls the funclet will save them, so the funclet does not have to ... unless there is some other reason.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll remove them, thanks.

Comment thread src/jit/codegencommon.cpp
// TODO Restore callee-saved registers
inst_RV(INS_pop, REG_EDI, TYP_REF);
inst_RV(INS_pop, REG_ESI, TYP_REF);
inst_RV(INS_pop, REG_EBX, TYP_REF);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similarly before, genPopCalleeSavedRegisters would be better.

@wateret
Copy link
Copy Markdown
Member Author

wateret commented Feb 10, 2017

About nested catch failure, for the test case like below:

try {
  try {
    throw A;
  } catch { // catches A
    throw B;
  }
} catch { // catches B
}

When excuting the funclet that catches A, the funclet holds ebp that is restored by CallEHFunclet. Maybe it makes the unwider starts stack-walking from the main function frame, not from the funclet frame. (Actually I'm not sure how exactly unwinder works.)
If this is the case, I guess there would be no problem if it was AMD64 style funclet. That is: there is no CallEHFunclet and the ebp is saved and restored by the funclet itself.

@wateret
Copy link
Copy Markdown
Member Author

wateret commented Feb 13, 2017

@jkotas @parjong
I think the problem with nested catch failure is, like I said above, wrong ebp value. However now I don't think just switching to AMD64 style funclet would resolve this issue because ebp value is still not valid for unwinder.
For arm it seems like CallEHFunclet stores frame pointer in r7 and unwinder uses it as frame pointer. How could we make x86 work? Maybe knowing how AMD64 works would also help.

@parjong
Copy link
Copy Markdown

parjong commented Feb 13, 2017

@wateret While digging JIT.Methodical.casts.SEH._il_dbgcastclass_catch_neg test failure, I conclude that we need to revise CLR's managed unwinder to treat funclet frame as ESP frame.

I made a draft patch in https://github.com/parjong/coreclr/commit/a992b93ec85cf70c54558ff63a3b11ee0809b4db, it seems to work for JIT.Methodical.casts.SEH._il_dbgcastclass_catch_neg case. I'm currently running test to see its side-effect. I'll let you know when I have a result.

I'm not sure whether that patch could be aligned with your current patch as it does not use PSPSym at all, but it would be worthwhile to take a look.

@wateret
Copy link
Copy Markdown
Member Author

wateret commented Feb 20, 2017

Closing as #9601 includes this.

@wateret wateret closed this Feb 20, 2017
@wateret wateret deleted the wip/x86eh1 branch July 11, 2017 06:28
@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