This is an archive of the discontinued LLVM Phabricator instance.

Change AArch64 Windows EH UnwindHelp object to be a fixed object
ClosedPublic

Authored by danielframpton on Mar 29 2020, 9:52 AM.

Details

Summary

The UnwindHelp object is used during exception handling by runtime code. It must be findable from a fixed offset from FP.

This change allocates the UnwindHelp object as a fixed object (as is done for x86_64) to ensure that both the generated code and runtime agree on the location of the object.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=45346

Diff Detail

Event Timeline

danielframpton created this revision.Mar 29 2020, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2020, 9:52 AM

If you're not using arcanist, please upload patches with full context (-U10000).

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
219

Maybe elaborate a bit; this is specifically the first space allocated in the function, next to sp on entry.

1011

Maybe move the "Var args are accounted for in the containing function" comment into getFixedObjectSize?

2683

This loop is strange. There's space explicitly allocated for the object in getFixedObjectSize, so you should be able to directly compute its offset.

2690

Why does UnwindHelp need to be 16-byte aligned?

llvm/test/CodeGen/AArch64/wineh-unwindhelp-via-fp.ll
11

This testcase seems overly complicated; if you're only checking the assembly output for ?func2@@YAXXZ, no need to include the other definitions.

Updates from review feedback.

danielframpton marked 5 inline comments as done.Mar 29 2020, 2:05 PM

Thanks for the review!

danielframpton marked 6 inline comments as done.Mar 29 2020, 2:51 PM

Added responses to the review and pointed out places where there are important codegen differences.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
219

Is this good?

1011

Done.

2683

This loop exists in the corresponding X86 code, but the use of FixedObject there is different.

I have changed to use the calculation from above instead.

2690

The unwind help object itself only needs 8 byte alignment, but the code was expecting the FixedObject area to be 16 byte aligned.

llvm/test/CodeGen/AArch64/seh-finally.ll
94–96

This is the key difference in codegen (using fp to store the -2).

213

This is the key difference in codegen (using fp to store the -2).

efriedma accepted this revision.Mar 30 2020, 2:05 PM

LGTM

I assume you don't have commit access, so I'll commit it for you. How would you like me to credit you in the commit message (name and email address)?

(The unnecessary alignment is probably something we could fix, but not worth the complexity.)

This revision is now accepted and ready to land.Mar 30 2020, 2:05 PM

LGTM

I assume you don't have commit access, so I'll commit it for you. How would you like me to credit you in the commit message (name and email address)?

(The unnecessary alignment is probably something we could fix, but not worth the complexity.)

I do not have commit access, so that would be very helpful, Thanks! You can use Daniel Frampton <Daniel.Frampton@microsoft.com>

LGTM

I assume you don't have commit access, so I'll commit it for you. How would you like me to credit you in the commit message (name and email address)?

(The unnecessary alignment is probably something we could fix, but not worth the complexity.)

I do not have commit access, so that would be very helpful, Thanks! You can use Daniel Frampton <Daniel.Frampton@microsoft.com>

I am doing some final validation of the version you accepted (using it to self-host rustc on windows). I will let you know that is complete.

LGTM

I assume you don't have commit access, so I'll commit it for you. How would you like me to credit you in the commit message (name and email address)?

(The unnecessary alignment is probably something we could fix, but not worth the complexity.)

I do not have commit access, so that would be very helpful, Thanks! You can use Daniel Frampton <Daniel.Frampton@microsoft.com>

I am doing some final validation of the version you accepted (using it to self-host rustc on windows). I will let you know that is complete.

My additional validation showed no issues. Please let me know if there is anything you need me to do.

This revision was automatically updated to reflect the committed changes.