This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] XCOFF exception section support on the integrated assembler path
ClosedPublic

Authored by pscoro on Sep 19 2022, 9:58 AM.

Details

Summary

Continuation of https://reviews.llvm.org/D132146 (direct assembly path support, needs to merge first). Adds support to the integrated assembler path for emitting XCOFF exception sections. Both features need https://reviews.llvm.org/D133030 to merge first

Diff Detail

Event Timeline

pscoro created this revision.Sep 19 2022, 9:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2022, 9:58 AM
pscoro requested review of this revision.Sep 19 2022, 9:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2022, 9:58 AM
pscoro retitled this revision from finished integrated assembler version to [PowerPC] XCOFF exception section support on the integrated assembler path.Sep 19 2022, 10:02 AM
pscoro edited the summary of this revision. (Show Details)
pscoro updated this revision to Diff 462788.Sep 25 2022, 7:14 PM

adding missing file change

pscoro updated this revision to Diff 462793.Sep 25 2022, 7:51 PM

mostly formatting fixes

tschuett added inline comments.
llvm/include/llvm/MC/MCObjectWriter.h
108

Is this going to be an XCOFF only function?

pscoro added inline comments.Oct 9 2022, 3:04 PM
llvm/include/llvm/MC/MCObjectWriter.h
108

Yes, exception sections are an XCOFF only feature. The specifics can be found in the XCOFF documentation: https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#d236277e7374

tschuett added inline comments.Oct 10 2022, 1:04 AM
llvm/include/llvm/MC/MCObjectWriter.h
108

In the other diff, there was a report_fatal_error if used by other than XCOFF.

pscoro updated this revision to Diff 466800.Oct 11 2022, 6:58 AM

added addExceptionEntry fatal error if not XCOFF

pscoro marked an inline comment as done.Oct 11 2022, 7:00 AM
pscoro added inline comments.
llvm/include/llvm/MC/MCObjectWriter.h
108

Yep, the other diff had a fatal error for emitXCOFFExceptDirective, a similar error was needed here, good catch thanks.

shchenz added inline comments.Oct 14 2022, 2:59 AM
llvm/include/llvm/MC/MCObjectWriter.h
111

Format here seems strange...

llvm/lib/MC/XCOFFObjectWriter.cpp
214

Move this field to ExceptionSectionEntry as the key.

220

Can we change this to a map? The key is the function symbol(The Symbol in ExceptionInfo struct) and the value is the ExceptionInfo struct which contains all entries for a function. So we don't need to go through all the functions exception table to find a ExceptionInfo for a given symbol? I saw too many such loops in your implementation that can be optimized out by using a map.

356

Can we use !ExceptionTable.empty() for this? getExceptionSectionSize() sounds like too heavy.

792

Can we explain why we need to add 0x0020 for 32-bit symbol?

795

If we are using a map for ExceptionTable, we can reuse writeSymbolEntry() and the last writeSymbolAuxCsectEntry() with legacy codes. Just need to do some special handling for the last parameter NumberOfAuxEntries for symbol which has except entries.

1098

Can optimize this by using a map.

1115

Maybe we need comments here for the +1? The first exception entry for a function.

1422

Please add comments here for the first section entry.

llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section-debug.ll
2

nit: is the -O0 required?

7

For this test, if it is to test the Exception Auxiliary Entry aux symbol, maybe we can only keep one RUN line to only check the symbols. Other test points should be covered in other tests, right?

llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll
208

This is exactly the same with 32-bit? If so, we don't need to make another copy.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-td.ll
7

We should not remove the RUN line as it is the only one test for -filetype=obj. Maybe we can modify the check to a simple check, for example, ; OBJ-LABEL: test__trapd_annotation: to check that the case does not crash in obj mode now?

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-tw.ll
14

ditto.

DiggerLin added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
220

agree with shchenz.

1135

line 1128-1134 can change to

Offset += (is64Bit()) ? (SymbolEntry.Entries.size() + 1) *
                              XCOFF::ExceptionSectionEntrySize64
                        : (SymbolEntry.Entries.size() + 1) *
                              XCOFF::ExceptionSectionEntrySize32;
1200

map also will improve the code efficiency here without a loop.

1215

bool hasExceptionSection() { return (bool) getExceptionSectionSize(); } }
and XCOFFObjectWriter::getExceptionSectionSize() loop for all ExceptionSection.ExceptionTable.

it is low efficiency here. I think you can use variable to calculate once at the begin of the function.

and there are several place calll hasExceptionSection() in the patch, maybe we can reimplement as
bool hasExceptionSection() { !ExceptionSection.ExceptionTable.empty();}

1216

Maybe better to add some comment here to explain the following code?

pscoro updated this revision to Diff 468255.Oct 17 2022, 10:45 AM
pscoro marked an inline comment as done.

optimizations

pscoro marked 14 inline comments as done.Oct 17 2022, 10:52 AM
pscoro added inline comments.
llvm/include/llvm/MC/MCObjectWriter.h
111

I ran clang-format on these lines and no changes were made

llvm/lib/MC/XCOFFObjectWriter.cpp
795

Not entirely sure what you had in mind here. I can not search the exception map directly in writeSymbolEntry because the symbol is passed by name (stringref not MCSymbol) so I'd either have to change writeSymbolEntry's function signature or find the symbol in the map beforehand (what it is doing now). Let me know what you think of the code changes here and if more changes are needed.

1215

and there are several place calll hasExceptionSection() in the patch, maybe we can reimplement as bool hasExceptionSection() { !ExceptionSection.ExceptionTable.empty();}

Went ahead with this fix

1216

Made the above comment explain the number of entries in better detail

pscoro updated this revision to Diff 468260.Oct 17 2022, 10:59 AM
pscoro marked 4 inline comments as done.

changes to debug lit tests

pscoro marked 3 inline comments as done.Oct 17 2022, 11:03 AM
pscoro added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll
208

For some reason the order of the exception section entries is backwards on 64-bit, is this an issue? It shouldn't affect the correctness of the exception section

DiggerLin added inline comments.Oct 18 2022, 11:26 AM
llvm/include/llvm/MC/MCObjectWriter.h
111

need a test case for this code.

llvm/lib/MC/XCOFFObjectWriter.cpp
205

the unsigned is 4 bytes no matter in 32bit and 64bits.
the e_addr.e_paddr is 8bytes in 64bits. I do not think "unsigned TrapAddress" is enough to hold the data. should be change to uint64_t.

224

need assert assert(N.size() <= XCOFF::NameSize && "section name too long"); before memcpy as otherSectionEntry constructor.

227

I do not think we need the line , the default move constructor is generated by default in this scenario.

694

why delete the comment ?

809

add some comment to explain why +4 and +3 here.

854

nit: still can use writeWord(LineNumberPointer);

1115

change the code to

unsigned EntryNum=0;

for (auto it = ExceptionSection.ExceptionTable.begin(); it != ExceptionSection.ExceptionTable.end(); ++it)
  // The size() gets +1 to account for the initial entry containing the
  // symbol table index.
  EntryNum += it->second.Entries.size() + 1;

return    EntryNum * (is64Bit() ? XCOFF::ExceptionSectionEntrySize64 : XCOFF::ExceptionSectionEntrySize32);

it only need one time of multiplication , it can improve the efficiency.

1135

Same reason as above. the function can change to

unsigned EntrtNum = 0;

for (auto it = ExceptionSection.ExceptionTable.begin(); it != ExceptionSection.ExceptionTable.end(); ++it) {
  if (Symbol == it->first)
    break;
  EntryNum += it->second.Entries.size() + 1;
}
return EntryNum * (is64Bit() ? XCOFF::ExceptionSectionEntrySize64 : XCOFF::ExceptionSectionEntrySize32);
1428

add comment to indicate this is 4 bytes padding when the entry is symbol index for 64bits.

1436
if (is64Bit()) {
  W.write<uint64_t>(TrapEntry.TrapAddress);
} else {
  W.write<uint32_t>(TrapEntry.TrapAddress);
}

change to

W.wrtieWord(TrapEntry.TrapAddress);
DiggerLin added inline comments.Oct 18 2022, 12:35 PM
llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section-debug.ll
3

should be add a 32bit test case?

113

in order to simplify the test case, suggest to delete the checking symbol "Name: sub_test" from line 94-112.

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

same question as zhengchen put : "is the -O0 required?"

4

I think you have generate the %t.o in line 1, I do not think we need to generate again ? same problem in the following.

77

I do not think you need to check the section .text and .data

117

delete the unrelated the symbol check for the patch.

pscoro updated this revision to Diff 469221.Oct 20 2022, 7:12 AM
pscoro marked 2 inline comments as done.

optimizations and lit test fixes

pscoro marked 14 inline comments as done.Oct 20 2022, 7:13 AM
DiggerLin added inline comments.Oct 21 2022, 11:40 AM
llvm/include/llvm/MC/MCObjectWriter.h
108

change "MCSymbol *Trap" to " const MCSymbol *Trap"

DiggerLin added inline comments.Oct 21 2022, 1:29 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
204

I do not think you need to change the content of MCSymbol pointed by *Trap.

const MCSymbol *Trap;

will be better.

DiggerLin added inline comments.Oct 24 2022, 7:30 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
796

we emit the xcoff object code as old interpret (SymbolRef.getVisibilityType() | 0x0020). ?

if we want to interpret as old version what about the code in line 818.

writeSymbolEntry(SymbolRef.getSymbolTableName(),
                     CSectionRef.Address + SymbolOffset, SectionIndex,
                     SymbolRef.getVisibilityType(),
                     SymbolRef.getStorageClass());
llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section-debug.ll
2

Maybe better to add comment to explain the purpose of the new added test case.

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

Maybe better to add comment to explain the purpose of the new added test case.

6

is the test duplicate with

; RUN: llvm-readobj --section-headers %t.o | FileCheck %s --check-prefix=READ ?

pscoro updated this revision to Diff 470257.Oct 24 2022, 12:28 PM
pscoro marked 4 inline comments as done.

fixes

pscoro marked 4 inline comments as done.Oct 24 2022, 12:30 PM
pscoro added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
796

We ran into an issue where this feature would not work without the 0x0020 bit set, which according to the documentation is part of the old xcoff implementation. It would make sense that the OR 0x0020 should be added to line 818 as well, but first I need to double check that my understanding is correct. Am I correct that this void XCOFFObjectWriter::writeSymbolEntryForCsectMemberLabel function is called with const Symbol SymbolRefs that are always function symbols? If this is the case, then I can add the 0x0020 to line 818 as well.

llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll
6

No, the output of llvm-readobj and llvm-objdump with --section-headers are not the same.

DiggerLin added inline comments.Oct 24 2022, 1:44 PM
llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll
6

you want to check the content of the .exception header? I think both check it.

pscoro added inline comments.Oct 24 2022, 2:13 PM
llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll
6

Yes, my thinking behind the llvm-objdump line was to test that the output of that command is correct in addition to the llvm-readobj output. I see that this is technically just testing that llvm-objdump is working correctly and isn't really adding anything to exception section testing, so I should just remove the llvm-objdump tests?

DiggerLin added inline comments.Oct 25 2022, 6:10 AM
llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll
6

I think we should keep llvm-readobj test. it have more detail info, at lease you also check following in the llvm-readobj test

; READ-NEXT: Type: STYP_EXCEPT (0x100)

DiggerLin added inline comments.Oct 25 2022, 6:50 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
796

Am I correct that this void XCOFFObjectWriter::writeSymbolEntryForCsectMemberLabel function is called with const Symbol SymbolRefs that are always function symbols?

I think when -fdata-section is off, A global data symbol also enter into the function. You can write test case and test it.

pscoro updated this revision to Diff 470573.Oct 25 2022, 11:19 AM
pscoro marked 2 inline comments as done.

Removed unneeded llvm-objdump tests

pscoro marked 2 inline comments as done.Oct 25 2022, 11:21 AM
pscoro updated this revision to Diff 471886.Oct 30 2022, 5:24 PM

Replacing DenseMap usage with std::map to ensure a determinable order of exception entries

Replacing DenseMap usage with std::map to ensure a determinable order of exception entries

Could you please check whether you find something for your use case:
https://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task

std::map is frowned upon in LLVM. It is very heap heavy.

Replacing DenseMap usage with std::map to ensure a determinable order of exception entries

Could you please check whether you find something for your use case:
https://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task

std::map is frowned upon in LLVM. It is very heap heavy.

I need a map data structure with a determinable order, I was looking at that page earlier when I settled on std::map. The only other option I saw was MapVector:

MapVector<KeyT,ValueT> provides a subset of the DenseMap interface. The main difference is that the iteration order is guaranteed to be the insertion order, making it an easy (but somewhat expensive) solution for non-deterministic iteration over maps of pointers.

I chose std::map over this because I thought MapVector would be too expensive, would you know if MapVector would be less expensive and preferable to std::map here?

Replacing DenseMap usage with std::map to ensure a determinable order of exception entries

Could you please check whether you find something for your use case:
https://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task

std::map is frowned upon in LLVM. It is very heap heavy.

I need a map data structure with a determinable order, I was looking at that page earlier when I settled on std::map. The only other option I saw was MapVector:

MapVector<KeyT,ValueT> provides a subset of the DenseMap interface. The main difference is that the iteration order is guaranteed to be the insertion order, making it an easy (but somewhat expensive) solution for non-deterministic iteration over maps of pointers.

I chose std::map over this because I thought MapVector would be too expensive, would you know if MapVector would be less expensive and preferable to std::map here?

Unfortunately no.

looks almost good to me. Just some nits left.

Please also make sure all @DiggerLin 's comments be addressed.

llvm/include/llvm/MC/MCObjectWriter.h
111

clang-format can not merge the string in different lines to one line? I don't think we need two lines here

llvm/lib/MC/XCOFFObjectWriter.cpp
1200

nit: alignment here

llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section-debug.ll
5

do we still need -O0?

llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll
4

Ditto

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-td.ll
7

This comment seems not be addressed?

pscoro updated this revision to Diff 472618.Nov 2 2022, 8:14 AM

Addressed nits

pscoro marked 7 inline comments as done.Nov 2 2022, 8:18 AM
pscoro added inline comments.
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-td.ll
7

I can't add OBJ-LABEL because this isnt checking assembly labels but I added the equivalent for object files

shchenz accepted this revision.Nov 2 2022, 6:27 PM

LGTM now. Please wait for some days in case other reviewers have comments.
Thanks for adding the new feature.

This revision is now accepted and ready to land.Nov 2 2022, 6:27 PM
DiggerLin added inline comments.Nov 6 2022, 10:15 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
694

sorry, I think you maybe have to delete the comment. since you have set "the function indicator" in writeSymbolEntryForCsectMemberLabel.

and I am not sure whether we need to set the "the function indicator" for SymbolType when there is no except auxiliary entry , if there is , we maybe need to add the a TODO comment on it.

pscoro updated this revision to Diff 473842.Nov 7 2022, 5:58 PM

removing comment

pscoro marked an inline comment as done.Nov 7 2022, 5:59 PM
DiggerLin accepted this revision.Nov 8 2022, 6:45 AM

LGTM

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-tw.ll
52

nit: delete last line

pscoro updated this revision to Diff 474053.Nov 8 2022, 11:19 AM

Removed whitespace

pscoro marked an inline comment as done.Nov 8 2022, 11:19 AM
pscoro updated this revision to Diff 476794.Nov 20 2022, 8:02 PM

another determinable order fix for the exception map

This revision was landed with ongoing or failed builds.Nov 20 2022, 10:16 PM
This revision was automatically updated to reflect the committed changes.
Esme added a subscriber: Esme.Dec 4 2022, 9:02 PM

FYI. I committed an NFC patch rG664cbfaf07e0 to format this patch.