Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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
796

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.

1221

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

1222

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 ?

810

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

858

nit: still can use writeWord(LineNumberPointer);

1119

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.

1139

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);
1434

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

1442
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
797

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
797

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
797

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
1205

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.Sun, Nov 20, 8:02 PM

another determinable order fix for the exception map

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

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