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
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/include/llvm/MC/MCObjectWriter.h | ||
---|---|---|
108 | Is this going to be an XCOFF only function? |
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 |
llvm/include/llvm/MC/MCObjectWriter.h | ||
---|---|---|
108 | In the other diff, there was a report_fatal_error if used by other than XCOFF. |
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. |
llvm/include/llvm/MC/MCObjectWriter.h | ||
---|---|---|
111 | Format here seems strange... | |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
213 | Move this field to ExceptionSectionEntry as the key. | |
219 | 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. | |
1094 | Can optimize this by using a map. | |
1111 | Maybe we need comments here for the +1? The first exception entry for a function. | |
1418 | 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. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
219 | agree with shchenz. | |
1131 | line 1128-1134 can change to Offset += (is64Bit()) ? (SymbolEntry.Entries.size() + 1) * XCOFF::ExceptionSectionEntrySize64 : (SymbolEntry.Entries.size() + 1) * XCOFF::ExceptionSectionEntrySize32; | |
1196 | map also will improve the code efficiency here without a loop. | |
1211 | bool hasExceptionSection() { return (bool) getExceptionSectionSize(); } } 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 | |
1212 | Maybe better to add some comment here to explain the following code? |
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. | |
1211 |
Went ahead with this fix | |
1212 | Made the above comment explain the number of entries in better detail |
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 |
llvm/include/llvm/MC/MCObjectWriter.h | ||
---|---|---|
111 | need a test case for this code. | |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
204 | the unsigned is 4 bytes no matter in 32bit and 64bits. | |
223 | need assert assert(N.size() <= XCOFF::NameSize && "section name too long"); before memcpy as otherSectionEntry constructor. | |
226 | 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. | |
850 | nit: still can use writeWord(LineNumberPointer); | |
1111 | 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. | |
1131 | Same reason as above. the function can change to
| |
1424 | add comment to indicate this is 4 bytes padding when the entry is symbol index for 64bits. | |
1432 | if (is64Bit()) { W.write<uint64_t>(TrapEntry.TrapAddress); } else { W.write<uint32_t>(TrapEntry.TrapAddress); } change to W.wrtieWord(TrapEntry.TrapAddress); |
llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section-debug.ll | ||
---|---|---|
2 | should be add a 32bit test case? | |
112 | 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 | ||
1 | same question as zhengchen put : "is the -O0 required?" | |
3 | 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. | |
76 | I do not think you need to check the section .text and .data | |
116 | delete the unrelated the symbol check for the patch. |
llvm/include/llvm/MC/MCObjectWriter.h | ||
---|---|---|
108 | change "MCSymbol *Trap" to " const MCSymbol *Trap" |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
203 | I do not think you need to change the content of MCSymbol pointed by *Trap. const MCSymbol *Trap; will be better. |
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 ? |
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. |
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. |
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? |
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) |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
796 |
I think when -fdata-section is off, A global data symbol also enter into the function. You can write test case and test it. |
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?
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 | ||
1196 | 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? |
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 |
LGTM now. Please wait for some days in case other reviewers have comments.
Thanks for adding the new feature.
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. |
LGTM
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-tw.ll | ||
---|---|---|
46 | nit: delete last line |
Is this going to be an XCOFF only function?