This is an archive of the discontinued LLVM Phabricator instance.

[WinEH] Update exception pointer registers
ClosedPublic

Authored by JosephTremoulet on Nov 4 2015, 11:24 AM.

Details

Summary

The CLR's personality routine passes these in rdx/edx, not rax/eax.

Make getExceptionPointerRegister a virtual method parameterized by
personality function to allow making this distinction.

Similarly make getExceptionSelectorRegister a virtual method parameterized
by personality function, for symmetry.

Diff Detail

Event Timeline

JosephTremoulet retitled this revision from to [WinEH] Set ExceptionPointerRegister for CoreCLR.
JosephTremoulet updated this object.
JosephTremoulet added reviewers: rnk, pgavlin, majnemer.
JosephTremoulet added a subscriber: llvm-commits.
rnk edited edge metadata.Nov 4 2015, 11:36 AM

Maybe getExceptionPointerRegister() should be virtual and accept a personality function instead. I'm imagining a situation where CoreCLR personality functions are in a module with SEH personality functions, and this change as-is won't work.

You can avoid updating all targets by leaving behind the default implementation that returns the ExceptionPointerRegister field for targets that don't have split EH personality disorder. :)

In D14344#281435, @rnk wrote:

Maybe getExceptionPointerRegister() should be virtual and accept a personality function instead. I'm imagining a situation where CoreCLR personality functions are in a module with SEH personality functions, and this change as-is won't work.

You can avoid updating all targets by leaving behind the default implementation that returns the ExceptionPointerRegister field for targets that don't have split EH personality disorder. :)

I agree that conditionalizing this on the personality sounds more correct than doing it on the environment as I have it.

It feels funny to leave around setExceptionPointerRegister if it's sometimes effectively ignored. Grepping the in-tree targets, I just see 10 callers, 8 of which set to a constant value and 2 of which are parameterized on a property available on the TargetLowering, so updating those wouldn't seem to be much of a struggle (and likewise for ExceptionSelectorRegister).
Does that mean the Right Thing(TM) is to remove the backing field and update all the in-tree targets to encode the information in their override of the new virtual method instead, or is it better to just back off and leave the backing-field-based default implementation as you suggested, for the sake of out-of-tree target maintainers?

rnk added a comment.Nov 5 2015, 1:32 PM

It feels funny to leave around setExceptionPointerRegister if it's sometimes effectively ignored. Grepping the in-tree targets, I just see 10 callers, 8 of which set to a constant value and 2 of which are parameterized on a property available on the TargetLowering, so updating those wouldn't seem to be much of a struggle (and likewise for ExceptionSelectorRegister).
Does that mean the Right Thing(TM) is to remove the backing field and update all the in-tree targets to encode the information in their override of the new virtual method instead, or is it better to just back off and leave the backing-field-based default implementation as you suggested, for the sake of out-of-tree target maintainers?

If you're up for updating all the in-tree targets and removing setExceptionPointerRegister, that's probably the right way to go.

JosephTremoulet retitled this revision from [WinEH] Set ExceptionPointerRegister for CoreCLR to [WinEH] Update exception pointer registers.
JosephTremoulet updated this object.
JosephTremoulet edited edge metadata.
  • make getExceptionPointerRegister/getExceptionSelectorRegister virtual, parameterized by personality
  • expand wineh-coreclr test to check for the right exception pointer register
rnk accepted this revision.Nov 6 2015, 3:01 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Nov 6 2015, 3:01 PM