Page MenuHomePhabricator

[XCOFF][AIX] Emit EH information in traceback table
ClosedPublic

Authored by DiggerLin on Dec 7 2020, 7:49 AM.

Details

Summary

In order for the runtime on AIX to find the compact unwind section(EHInfo table),
we would need to set the following on the traceback table:

  1. The 6th byte's longtbtable field to true to signal there is an Extended TB Table Flag.
  2. The Extended TB Table Flag to be 0x08 to signal there is an exception handling info presents.
  3. Emit the offset between ehinfo TC entry and TOC base after all other optional portions of traceback table.

Diff Detail

Event Timeline

jasonliu created this revision.Dec 7 2020, 7:49 AM
jasonliu requested review of this revision.Dec 7 2020, 7:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2020, 7:49 AM
majnemer added inline comments.
llvm/test/CodeGen/PowerPC/aix-exception.ll
119

Is this right? I'd expect '_', 'Z', '9',...

jasonliu added inline comments.Dec 7 2020, 11:20 AM
llvm/test/CodeGen/PowerPC/aix-exception.ll
119

I think this is the accepted syntax for the assembler on AIX at least.
https://www.ibm.com/support/knowledgecenter/ssw_aix_72/assembler/assembler_pdf.pdf

ASCII character constants (for example, 'X) and string constants (for example, Hello, world) can also
be assembled using the .byte pseudo-op.

and example in the guide:

.csect data[rw]
.byte "Hello, world"
.byte 'H,'e,'l,'l,'o,',,' ,'w,'o,'r,'l,'d
DiggerLin added inline comments.
llvm/lib/BinaryFormat/XCOFF.cpp
131

there already have a function define getExtendedTBTableFlagString in the https://reviews.llvm.org/D92398

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1988

since we only have ShouldEmitEHBlock. if the ShouldEmitEHBlock is false, it will still output an ExtensionTableFlag. Add an assert(ShouldEmitEHBlock) here ?

llvm/test/CodeGen/PowerPC/aix-exception.ll
2

we can delete -xcoff-traceback-table here after rebased on the latest https://reviews.llvm.org/D92398

jasonliu added inline comments.Dec 7 2020, 3:17 PM
llvm/lib/BinaryFormat/XCOFF.cpp
131

It's not in the Phabricator review that you point out. It's in D89049. But that revision is going to land after this one I think.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1988

I think it's more natural this way. As later we might add in more flags in ExtensionTableFlag (e.g. for stack protection canary). It won't be necessarily the only field for this ExtensionTableFlag anymore.

llvm/test/CodeGen/PowerPC/aix-exception.ll
2

For sure.

daltenty added inline comments.Dec 8 2020, 8:39 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
398

nit: since every other entry is missing a period, lets just omit it (and the s)

llvm/lib/CodeGen/AsmPrinter/AIXException.cpp
71

Compared with what was here before it seems like F.getPersonalityFn() can assert if hasPersonalityFn() is false, which can happen now because we won't check that before setting Per anymore, and we'll answer ShouldEmitEHBlock true if we have landing pads regardless of the personality setting.

jasonliu added inline comments.Dec 8 2020, 10:25 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
398

I think we are suppose to have periods in every comments (at least it's the most used form in this file as well), so I will do a drive by fix to add periods in every entry.

llvm/lib/CodeGen/AsmPrinter/AIXException.cpp
71

Runtime need to use personality routine to parses LSDA area to get to those landingpads. So without personality routine, there is no reason for us to have landingpads.
So if we have landingpads, but no personality rountine presents in this function, something might already gone wrong somewhere else.
I could add an assert here and if we hit it in any case, we would need to examine the situation at that point.

jasonliu updated this revision to Diff 310280.Dec 8 2020, 10:38 AM

Address comments.

DiggerLin added inline comments.Dec 8 2020, 12:40 PM
llvm/lib/CodeGen/AsmPrinter/AIXException.cpp
73

Constant *Function::getPersonalityFn() const {

assert(hasPersonalityFn() && getNumOperands());
return cast<Constant>(Op<0>());

}

do we need the
assert(F.hasPersonalityFn() &&

"Landingpads are presented, but no personality routine is found.");

once more ?

jasonliu added inline comments.Dec 8 2020, 1:53 PM
llvm/lib/CodeGen/AsmPrinter/AIXException.cpp
73

It helps to tell exactly what went wrong in this context. i.e. landingpad presents, but no personality routine gets us here.
But if you think it's really redundant, I could remove it.

jasonliu updated this revision to Diff 310752.Dec 9 2020, 7:59 PM

Rebased on the latest parent.

no further comment on it . It just need to rebased after

llvm/lib/CodeGen/AsmPrinter/AIXException.cpp
73

it is ok for me

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2077

it means there will be same symbol name in the different translation unit ?

This revision is now accepted and ready to land.Dec 10 2020, 7:07 AM
jasonliu added inline comments.Dec 10 2020, 7:09 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2077

Yes, and that's Okay because these symbols are internal(HIDE_EXT).

DiggerLin commandeered this revision.Dec 15 2020, 8:15 AM
DiggerLin edited reviewers, added: jasonliu; removed: DiggerLin.

for Jason is on vacation , I will take over the revision and rebase the patch based on the committed https://reviews.llvm.org/D92398

This revision now requires review to proceed.Dec 15 2020, 8:15 AM
DiggerLin updated this revision to Diff 311914.Dec 15 2020, 8:16 AM

rebased the patch based on the https://reviews.llvm.org/D92398.

This revision is now accepted and ready to land.Dec 15 2020, 5:21 PM
This revision was landed with ongoing or failed builds.Dec 16 2020, 6:36 AM
This revision was automatically updated to reflect the committed changes.