[PATCH] D89049: [AIX][XCOFF] print out the traceback info
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 12 01:47:04 PDT 2023
jhenderson added a comment.
Pre-merge check is complaining about clang-format. Please make sure to run clang-format on all your changes.
There are a lot of cases where you've used the `.value()` method of `std::optional`. I believe `operator*()`/`operator->()` are more typical ways of achieving the same thing (and according to cppreference.com, may even be more efficient).
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1435
SmallVector<uint32_t, 8> Disp;
- Disp.reserve(*NumOfCtlAnchors);
+ Disp.reserve(NumOfCtlAnchors.value());
for (uint32_t I = 0; I < NumOfCtlAnchors && Cur; ++I)
----------------
Is this change needed?
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1488
+ if (ExtensionTable.value() & ExtendedTBTableFlag::TB_EH_INFO) {
+ // eh_info displacement must be 4-byte aligned.
----------------
Is this an option?
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test:1
+## Test that parsing of an invalid XCOFF traceback table having a too
+## big function name length causes an out of range error.
----------------
Sorry, didn't spot this before.
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test:22
+ - Type: AUX_CSECT
+ SectionOrLength: 0x24
+ SymbolAlignmentAndType: 0x21
----------------
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:2-3
+## Test that "llvm-objdump --traceback-table" decodes the ControlledStorageInfo,
+## AllocaRegister, and extension table of the traceback table, which cannot
+## currently be generated by llc.
+
----------------
I don't think the part about llc is relevant. It will also go out-of-date if llc should ever be able to generate it in the future.
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:19
+ Flags: [ STYP_TEXT ]
+ SectionData: "9421ffc0000000000000204080000201000000000000000400064164644e756d0000000093e1fffc0000000000002a6080c202072c90000000000004000000036f0000010000000a000001000003666f6f1f0203c00000000000200000000000000000000000000001000000"
+Symbols:
----------------
How much of this text data is actually important to the test? If a significant proportion of it is, it would be worth giving instructions on how to generate the machine code that this represents.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:132
+ Index += 4; \
+ OS << "\t# " << #Field << " = " << TbTable.get##Field().value(); \
+ }
----------------
?
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:138
+ const MCSubtargetInfo &STI,
+ const ObjectFile *Obj) {
+ uint64_t Index = 0;
----------------
As this is specific to XCOFFObjectFile, I think it would make sense to pass in XCOFFObjectFile, rather than ObjectFile, and do the cast outside this function.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:268
+ SmallVector<uint32_t, 8> Disp =
+ TbTable.getControlledStorageInfoDisp().value();
+ for (unsigned I = 0; I < Disp.size(); ++I) {
----------------
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:279
+ if (TbTable.isFuncNamePresent()) {
+ uint16_t FunctionNameLen = TbTable.getFunctionName().value().size();
+
----------------
?
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:304
+ if (!HasPrinted) {
+ OS << "\t# FunctionName = " << TbTable.getFunctionName().value();
+ HasPrinted = true;
----------------
?
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:314
+ OS << format("\t# AllocaRegister = %u",
+ TbTable.getAllocaRegister().value());
+ }
----------------
?
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:319
+ OS << "\n";
+ TBVectorExt VecExt = TbTable.getVectorExt().value();
+ // Print first byte of VectorExt.
----------------
?
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:346
+ ExtendedTBTableFlag Flag =
+ static_cast<ExtendedTBTableFlag>(TbTable.getExtensionTable().value());
+ OS << "\t# ExtensionTable = " << getExtendedTBTableFlagString(Flag);
----------------
?
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:360-361
+ OS << "\t# EH info displacement";
+ // Print another 4 bytes for 64 bit. The displacement (address) is 8 bytes
+ // in 64 bit object files.
+ if (Is64Bit) {
----------------
I think the comment can be simplified, as suggested. Please reflow appropriately.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:384
+ return;
+ } else {
+ // Align to the word boundary for padding on the next line.
----------------
Don't use `else` after a `return`.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:391
+ // Return true if the end is reached.
+ auto Print4BytesOrLess = [&]() {
+ uint64_t PrintSize = Size - Index >= 4 ? 4 : Size - Index;
----------------
This is more grammatically correct.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:416
+
+ // Print four bytes which has non-zero value.
+ if (Print4BytesOrLess())
----------------
I'm not sure this comment makes sense (some of the bytes can be zero, just not the first one), but regardless, I don't think it's really needed.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:419
+ return;
+ if (Print4BytesOrLess())
+ return;
----------------
Is this supposed to be duplicated?
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:421
+ return;
+
+ } else {
----------------
Don't end a block with a blank line.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:378
+ if (Remaining >= 8) {
+ while (Remaining > 0 && Bytes[Size - Remaining] == 0)
+ --Remaining;
----------------
DiggerLin wrote:
> xingxue wrote:
> > DiggerLin wrote:
> > > xingxue wrote:
> > > > If the remaining bytes are paddings, why do we differentiate if a padding is zero or not? Can we just print `...` or `... # Paddings` for paddings?
> > > it just to print out the "... # Paddings" if all the remaining is zero and there are 8 or more bytes remaining in the end.
> > >
> > > otherwise, it will print all the bytes out with following code.
> > >
> > >
> > > ```
> > > uint16_t AlignmentLen = 4 - Index % 4;
> > > printRawData(Bytes.slice(Index, AlignmentLen), Address + Index, OS, STI);
> > > OS << "\t# Padding\n";
> > > Index += AlignmentLen;
> > > while (Index < End - Address) {
> > > printRawData(Bytes.slice(Index, 4), Address + Index, OS, STI);
> > > OS << "\t# Padding\n";
> > > Index += 4;
> > > }
> > > ```
> > >
> > I meant to print `...` or `...\t# Paddings` to indicate the rest are paddings regardless of whether a padding byte is zero or not. @stephenpeckham @jhenderson, what do you think?
> Print out no-zero value will letting user know what is non-zero value of the padding(in our llvm xcoff ojbect writer, the padding will always zero for sections, if not zero means ,something wrong, and printing out non-zero will let user get more info)
> @hubert.reinterpretcast , do you have any option on above xing's comment ?
I don't have a strong opinion on whether non-zero padding should be printed or not. I'm happy to defer to others on this.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1750
+ Symbols[SI - 1].XCOFFSymInfo.StorageMappingClass &&
+ (Symbols[SI - 1].XCOFFSymInfo.StorageMappingClass.value() ==
+ XCOFF::XMC_PR);
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89049/new/
https://reviews.llvm.org/D89049
More information about the llvm-commits
mailing list