[llvm-objdump] Implement decoding auxiliary header for xcoff with llvm-objdump --private-headers #105682

diggerlin · 2024-08-22T15:51:55Z

Implement decoding auxiliary header of XCOFF object file with llvm-objdump --private-headers

…vate-headers

llvmbot · 2024-08-22T15:52:37Z

@llvm/pr-subscribers-llvm-binary-utilities

Author: zhijian lin (diggerlin)

Changes

Implement decoding auxiliary header for xcoff with llvm-objdump --private-headers

Full diff: https://github.com/llvm/llvm-project/pull/105682.diff

2 Files Affected:

(added) llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test (+126)
(modified) llvm/tools/llvm-objdump/XCOFFDump.cpp (+184-4)

diff --git a/llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test b/llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test
new file mode 100644
index 00000000000000..848b43b5db2d74
--- /dev/null
+++ b/llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test
@@ -0,0 +1,126 @@
+## Test that `the llvm-objdump --private-headers` print out the auxiliary header of XCOFF object file.
+
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-objdump --private-headers %t1 | FileCheck %s --check-prefix=CHECK32
+# RUN: cp %t1 %t1_err1
+# RUN: python -c "with open(r'%t1_err1', 'r+b') as input: input.seek(17); input.write(b'\x45')"
+# RUN: python -c "with open(r'%t1_err1', 'r+b') as input: input.seek(4); input.write(b'\x00')"
+# RUN: llvm-objdump --private-headers %t1_err1 2>&1 | FileCheck %s --check-prefix=WARN1
+# RUN: cp %t1 %t1_extra
+# RUN: python -c "with open(r'%t1_extra', 'r+b') as input: input.seek(4); input.write(b'\x00')"
+# RUN: python -c "with open(r'%t1_extra', 'r+b') as input: input.seek(17); input.write(b'\x4f')"
+# RUN: llvm-objdump --private-headers %t1_extra 2>&1 | FileCheck %s --check-prefix=EXTRA
+# RUN: yaml2obj %s -DMAGIC=0x1F7 -DFLAG64=0x2 -o %t2
+# RUN: llvm-objdump --private-headers %t2 | FileCheck %s --check-prefix=CHECK64
+
+
+# CHECK32:      ---Auxiliary Header:
+# CHECK32-NEXT: Magic:                              0x10b
+# CHECK32-NEXT: Version:                            0x1
+# CHECK32-NEXT: Size of .text section:              0x8
+# CHECK32-NEXT: Size of .data section:              0x9
+# CHECK32-NEXT: Size of .bss section:               0x10
+# CHECK32-NEXT: Entry point address:                0x1111
+# CHECK32-NEXT: .text section start address:        0x2222
+# CHECK32-NEXT: .data section start address;        0x3333
+# CHECK32-NEXT: TOC anchor address:                 0x4444
+# CHECK32-NEXT: Section number of entryPoint:       1
+# CHECK32-NEXT: Section number of .text:            2
+# CHECK32-NEXT: Section number of .data:            3
+# CHECK32-NEXT: Section number of TOC:              4
+# CHECK32-NEXT: Section number of loader data:      5
+# CHECK32-NEXT: Section number of .bss:             6
+# CHECK32-NEXT: Maxium alignment of .text:          0x7
+# CHECK32-NEXT: Maxium alignment of .data:          0x3
+# CHECK32-NEXT: Module type;                        0x0
+# CHECK32-NEXT: CPU type of objects:                0x1
+# CHECK32-NEXT: (Reserved):                         0x0
+# CHECK32-NEXT: Maximum stack size:                 0x0
+# CHECK32-NEXT: Maximum data size:                  0x0
+# CHECK32-NEXT: Reserved for debugger:              0x0
+# CHECK32-NEXT: Text page size:                     0x1
+# CHECK32-NEXT: Data page size:                     0x1
+# CHECK32-NEXT: Stack page size:                    0x1
+# CHECK32-NEXT: Flag:                               0x0
+# CHECK32-NEXT: Alignment of thread-local storage:  0x1
+# CHECK32-NEXT: Section number for .tdata:          7
+# CHECK32-NEXT: Section number for .tbss:           8
+
+# CHECK64:      ---Auxiliary Header:
+# CHECK64-NEXT: Magic:                              0x10b
+# CHECK64-NEXT: Version:                            0x1
+# CHECK64-NEXT: Reserved for debugger:              0x0
+# CHECK64-NEXT: .text section start address:        0x2222
+# CHECK64-NEXT: .data section start address:        0x3333
+# CHECK64-NEXT: TOC anchor address:                 0x4444
+# CHECK64-NEXT: Section number of entryPoint:       1
+# CHECK64-NEXT: Section number of .text:            2
+# CHECK64-NEXT: Section number of .data:            3
+# CHECK64-NEXT: Section number of TOC:              4
+# CHECK64-NEXT: Section number of loader data:      5
+# CHECK64-NEXT: Section number of .bss:             6
+# CHECK64-NEXT: Maxium alignment of .text:          0x7
+# CHECK64-NEXT: Maxium alignment of .data:          0x3
+# CHECK64-NEXT: Module type:                        0x0
+# CHECK64-NEXT: CPU type of objects:                0x1
+# CHECK64-NEXT: (Reserved):                         0x0
+# CHECK64-NEXT: Text page size:                     0x1
+# CHECK64-NEXT: Data page size:                     0x1
+# CHECK64-NEXT: Stack page size:                    0x1
+# CHECK64-NEXT: Flag:                               0x0
+# CHECK64-NEXT: Alignment of thread-local storage:  0x1
+# CHECK64-NEXT: Size of .text section:              0x8
+# CHECK64-NEXT: Size of .data section:              0x9
+# CHECK64-NEXT: Size of .bss section:               0x10
+# CHECK64-NEXT: Entry point address:                0x1111
+# CHECK64-NEXT: Maximum stack size:                 0x0
+# CHECK64-NEXT: Maximum data size:                  0x0
+# CHECK64-NEXT: Section number for .tdata:          7
+# CHECK64-NEXT: Section number for .tbss:           8
+# CHECK64-NEXT: Additional flags 64-bit XCOFF:      0x2
+
+# WARN1:       {{.*}}: only partial field for Section number for .tdata: at offset (68)
+# WARN1-NEXT:  Raw data (00)
+
+# EXTRA: Extra raw data (00000000 000000)
+
+--- !XCOFF
+FileHeader:
+  MagicNumber: [[MAGIC=0x1DF]]
+AuxiliaryHeader:
+  Magic:                 0x10B
+  Version:               0x1
+  TextSectionSize:       0x8
+  DataSectionSize:       0x9
+  BssSectionSize:        0x10
+  EntryPointAddr:        0x1111
+  TextStartAddr:         0x2222
+  DataStartAddr:         0x3333
+  TOCAnchorAddr:         0x4444
+  SecNumOfEntryPoint:    1
+  SecNumOfText:          2
+  SecNumOfData:          3
+  SecNumOfTOC:           4
+  SecNumOfLoader:        5
+  SecNumOfBSS:           6
+  MaxAlignOfText:        0x7
+  MaxAlignOfData:        0x3
+  ModuleType:            0x1
+  TextPageSize:          0x1
+  DataPageSize:          0x1
+  StackPageSize:         0x1
+  SecNumOfTData:         7
+  SecNumOfTBSS:          8
+  FlagAndTDataAlignment: 0x1
+  Flag:                  [[FLAG64=<none>]]
+Sections:
+  - Flags:       [ STYP_TEXT ]
+    SectionData: "1232"
+  - Flags:       [ STYP_DATA ]
+    SectionData: "5678"
+  - Flags:       [ STYP_BSS ]
+    SectionData: "9101"
+  - Flags:       [ STYP_TDATA ]
+    SectionData: "1112"
+  - Flags:       [ STYP_TBSS ]
+    SectionData: "1314"
diff --git a/llvm/tools/llvm-objdump/XCOFFDump.cpp b/llvm/tools/llvm-objdump/XCOFFDump.cpp
index 524bf07c372a9b..34718fedfc14b5 100644
--- a/llvm/tools/llvm-objdump/XCOFFDump.cpp
+++ b/llvm/tools/llvm-objdump/XCOFFDump.cpp
@@ -37,18 +37,26 @@ class XCOFFDumper : public objdump::Dumper {
 
 public:
   XCOFFDumper(const object::XCOFFObjectFile &O) : Dumper(O), Obj(O) {}
+  void printBinary(StringRef Name, ArrayRef<uint8_t> B);
+  void printHex(StringRef Name, uint64_t Value);
+  void printNumber(StringRef Name, uint64_t Value);
 
 private:
   void printPrivateHeaders() override;
   void printFileHeader();
+  void printAuxiliaryHeader();
+  void printAuxiliaryHeader(const XCOFFAuxiliaryHeader32 *AuxHeader);
+  void printAuxiliaryHeader(const XCOFFAuxiliaryHeader64 *AuxHeader);
   FormattedString formatName(StringRef Name);
-  void printHex(StringRef Name, uint64_t Value);
-  void printNumber(StringRef Name, uint64_t Value);
   void printStrHex(StringRef Name, StringRef Str, uint64_t Value);
   void setWidth(unsigned W) { Width = W; };
+  unsigned getWidth() { return Width; }
 };
 
-void XCOFFDumper::printPrivateHeaders() { printFileHeader(); }
+void XCOFFDumper::printPrivateHeaders() {
+  printFileHeader();
+  printAuxiliaryHeader();
+}
 
 FormattedString XCOFFDumper::formatName(StringRef Name) {
   return FormattedString(Name, Width, FormattedString::JustifyLeft);
@@ -67,6 +75,179 @@ void XCOFFDumper::printStrHex(StringRef Name, StringRef Str, uint64_t Value) {
          << ")\n";
 }
 
+void XCOFFDumper::printBinary(StringRef Name, ArrayRef<uint8_t> B) {
+  unsigned OrgWidth = getWidth();
+  setWidth(0);
+  outs() << formatName(Name) << " (" << format_bytes(B) << ")\n";
+  setWidth(OrgWidth);
+}
+
+void XCOFFDumper::printAuxiliaryHeader() {
+  setWidth(36);
+  if (Obj.is64Bit())
+    printAuxiliaryHeader(Obj.auxiliaryHeader64());
+  else
+    printAuxiliaryHeader(Obj.auxiliaryHeader32());
+}
+
+enum PrintStyle { Hex, Number };
+template <typename T, typename V>
+static void printAuxMemberHelper(PrintStyle Style, const char *MemberName,
+                                 const T &Member, const V *AuxHeader,
+                                 uint16_t AuxSize, uint16_t &PartialFieldOffset,
+                                 const char *&PartialFieldName,
+                                 XCOFFDumper *Dumper) {
+  ptrdiff_t Offset = reinterpret_cast<const char *>(&Member) -
+                     reinterpret_cast<const char *>(AuxHeader);
+  if (Offset + sizeof(Member) <= AuxSize)
+    Style == Hex ? Dumper->printHex(MemberName, Member)
+                 : Dumper->printNumber(MemberName, Member);
+  else if (Offset < AuxSize) {
+    PartialFieldOffset = Offset;
+    PartialFieldName = MemberName;
+  }
+}
+
+template <class T>
+void checkAndPrintAuxHeaderParseError(const char *PartialFieldName,
+                                      uint16_t PartialFieldOffset,
+                                      uint16_t AuxSize, T &AuxHeader,
+                                      XCOFFDumper *Dumper) {
+  if (PartialFieldOffset < AuxSize) {
+    Dumper->reportUniqueWarning(Twine("only partial field for ") +
+                                PartialFieldName + " at offset (" +
+                                Twine(PartialFieldOffset) + ")");
+    Dumper->printBinary(
+        "Raw data",
+        ArrayRef<uint8_t>(reinterpret_cast<const uint8_t *>(&AuxHeader) +
+                              PartialFieldOffset,
+                          AuxSize - PartialFieldOffset));
+  } else if (sizeof(AuxHeader) < AuxSize)
+    Dumper->printBinary(
+        "Extra raw data",
+        ArrayRef<uint8_t>(reinterpret_cast<const uint8_t *>(&AuxHeader) +
+                              sizeof(AuxHeader),
+                          AuxSize - sizeof(AuxHeader)));
+}
+
+void XCOFFDumper::printAuxiliaryHeader(
+    const XCOFFAuxiliaryHeader32 *AuxHeader) {
+  if (AuxHeader == nullptr)
+    return;
+  outs() << "\n---Auxiliary Header:\n";
+  uint16_t AuxSize = Obj.getOptionalHeaderSize();
+  uint16_t PartialFieldOffset = AuxSize;
+  const char *PartialFieldName = nullptr;
+
+  auto PrintAuxMember = [&](PrintStyle Style, const char *MemberName,
+                            auto &Member) {
+    printAuxMemberHelper(Style, MemberName, Member, AuxHeader, AuxSize,
+                         PartialFieldOffset, PartialFieldName, this);
+  };
+
+  PrintAuxMember(Hex, "Magic:", AuxHeader->AuxMagic);
+  PrintAuxMember(Hex, "Version:", AuxHeader->Version);
+  PrintAuxMember(Hex, "Size of .text section:", AuxHeader->TextSize);
+  PrintAuxMember(Hex, "Size of .data section:", AuxHeader->InitDataSize);
+  PrintAuxMember(Hex, "Size of .bss section:", AuxHeader->BssDataSize);
+  PrintAuxMember(Hex, "Entry point address:", AuxHeader->EntryPointAddr);
+  PrintAuxMember(Hex, ".text section start address:", AuxHeader->TextStartAddr);
+  PrintAuxMember(Hex, ".data section start address;", AuxHeader->DataStartAddr);
+  PrintAuxMember(Hex, "TOC anchor address:", AuxHeader->TOCAnchorAddr);
+  PrintAuxMember(
+      Number, "Section number of entryPoint:", AuxHeader->SecNumOfEntryPoint);
+  PrintAuxMember(Number, "Section number of .text:", AuxHeader->SecNumOfText);
+  PrintAuxMember(Number, "Section number of .data:", AuxHeader->SecNumOfData);
+  PrintAuxMember(Number, "Section number of TOC:", AuxHeader->SecNumOfTOC);
+  PrintAuxMember(Number,
+                 "Section number of loader data:", AuxHeader->SecNumOfLoader);
+  PrintAuxMember(Number, "Section number of .bss:", AuxHeader->SecNumOfBSS);
+  PrintAuxMember(Hex, "Maxium alignment of .text:", AuxHeader->MaxAlignOfText);
+  PrintAuxMember(Hex, "Maxium alignment of .data:", AuxHeader->MaxAlignOfData);
+  PrintAuxMember(Hex, "Module type;", AuxHeader->ModuleType);
+  PrintAuxMember(Hex, "CPU type of objects:", AuxHeader->CpuFlag);
+  PrintAuxMember(Hex, "(Reserved):", AuxHeader->CpuType);
+  PrintAuxMember(Hex, "Maximum stack size:", AuxHeader->MaxStackSize);
+  PrintAuxMember(Hex, "Maximum data size:", AuxHeader->MaxDataSize);
+  PrintAuxMember(Hex, "Reserved for debugger:", AuxHeader->ReservedForDebugger);
+  PrintAuxMember(Hex, "Text page size:", AuxHeader->TextPageSize);
+  PrintAuxMember(Hex, "Data page size:", AuxHeader->DataPageSize);
+  PrintAuxMember(Hex, "Stack page size:", AuxHeader->StackPageSize);
+  if (offsetof(XCOFFAuxiliaryHeader32, FlagAndTDataAlignment) +
+          sizeof(XCOFFAuxiliaryHeader32::FlagAndTDataAlignment) <=
+      AuxSize) {
+    printHex("Flag:", AuxHeader->getFlag());
+    printHex("Alignment of thread-local storage:",
+             AuxHeader->getTDataAlignment());
+  }
+
+  PrintAuxMember(Number,
+                 "Section number for .tdata:", AuxHeader->SecNumOfTData);
+  PrintAuxMember(Number, "Section number for .tbss:", AuxHeader->SecNumOfTBSS);
+
+  checkAndPrintAuxHeaderParseError(PartialFieldName, PartialFieldOffset,
+                                   AuxSize, *AuxHeader, this);
+}
+
+void XCOFFDumper::printAuxiliaryHeader(
+    const XCOFFAuxiliaryHeader64 *AuxHeader) {
+  if (AuxHeader == nullptr)
+    return;
+  uint16_t AuxSize = Obj.getOptionalHeaderSize();
+  outs() << "\n---Auxiliary Header:\n";
+  uint16_t PartialFieldOffset = AuxSize;
+  const char *PartialFieldName = nullptr;
+
+  auto PrintAuxMember = [&](PrintStyle Style, const char *MemberName,
+                            auto &Member) {
+    printAuxMemberHelper(Style, MemberName, Member, AuxHeader, AuxSize,
+                         PartialFieldOffset, PartialFieldName, this);
+  };
+
+  PrintAuxMember(Hex, "Magic:", AuxHeader->AuxMagic);
+  PrintAuxMember(Hex, "Version:", AuxHeader->Version);
+  PrintAuxMember(Hex, "Reserved for debugger:", AuxHeader->ReservedForDebugger);
+  PrintAuxMember(Hex, ".text section start address:", AuxHeader->TextStartAddr);
+  PrintAuxMember(Hex, ".data section start address:", AuxHeader->DataStartAddr);
+  PrintAuxMember(Hex, "TOC anchor address:", AuxHeader->TOCAnchorAddr);
+  PrintAuxMember(
+      Number, "Section number of entryPoint:", AuxHeader->SecNumOfEntryPoint);
+  PrintAuxMember(Number, "Section number of .text:", AuxHeader->SecNumOfText);
+  PrintAuxMember(Number, "Section number of .data:", AuxHeader->SecNumOfData);
+  PrintAuxMember(Number, "Section number of TOC:", AuxHeader->SecNumOfTOC);
+  PrintAuxMember(Number,
+                 "Section number of loader data:", AuxHeader->SecNumOfLoader);
+  PrintAuxMember(Number, "Section number of .bss:", AuxHeader->SecNumOfBSS);
+  PrintAuxMember(Hex, "Maxium alignment of .text:", AuxHeader->MaxAlignOfText);
+  PrintAuxMember(Hex, "Maxium alignment of .data:", AuxHeader->MaxAlignOfData);
+  PrintAuxMember(Hex, "Module type:", AuxHeader->ModuleType);
+  PrintAuxMember(Hex, "CPU type of objects:", AuxHeader->CpuFlag);
+  PrintAuxMember(Hex, "(Reserved):", AuxHeader->CpuType);
+  PrintAuxMember(Hex, "Text page size:", AuxHeader->TextPageSize);
+  PrintAuxMember(Hex, "Data page size:", AuxHeader->DataPageSize);
+  PrintAuxMember(Hex, "Stack page size:", AuxHeader->StackPageSize);
+  if (offsetof(XCOFFAuxiliaryHeader64, FlagAndTDataAlignment) +
+          sizeof(XCOFFAuxiliaryHeader64::FlagAndTDataAlignment) <=
+      AuxSize) {
+    printHex("Flag:", AuxHeader->getFlag());
+    printHex("Alignment of thread-local storage:",
+             AuxHeader->getTDataAlignment());
+  }
+  PrintAuxMember(Hex, "Size of .text section:", AuxHeader->TextSize);
+  PrintAuxMember(Hex, "Size of .data section:", AuxHeader->InitDataSize);
+  PrintAuxMember(Hex, "Size of .bss section:", AuxHeader->BssDataSize);
+  PrintAuxMember(Hex, "Entry point address:", AuxHeader->EntryPointAddr);
+  PrintAuxMember(Hex, "Maximum stack size:", AuxHeader->MaxStackSize);
+  PrintAuxMember(Hex, "Maximum data size:", AuxHeader->MaxDataSize);
+  PrintAuxMember(Number,
+                 "Section number for .tdata:", AuxHeader->SecNumOfTData);
+  PrintAuxMember(Number, "Section number for .tbss:", AuxHeader->SecNumOfTBSS);
+  PrintAuxMember(Hex, "Additional flags 64-bit XCOFF:", AuxHeader->XCOFF64Flag);
+
+  checkAndPrintAuxHeaderParseError(PartialFieldName, PartialFieldOffset,
+                                   AuxSize, *AuxHeader, this);
+}
+
 void XCOFFDumper::printFileHeader() {
   setWidth(20);
   outs() << "\n---File Header:\n";
@@ -112,7 +293,6 @@ void XCOFFDumper::printFileHeader() {
   printHex("OptionalHeaderSize:", Obj.getOptionalHeaderSize());
   printHex("Flags:", Obj.getFlags());
 }
-
 } // namespace
 
 std::unique_ptr<objdump::Dumper>

jh7370 · 2024-08-28T06:59:02Z

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

@@ -0,0 +1,128 @@
+## Test that `the llvm-objdump --private-headers` print out the auxiliary header of XCOFF object file.


Please review what I've said in your past reviews about providing comments for individual cases within the test, ordering of RUN/YAML/CHECK lines and whitespace (especially blank lines), and act on them in this test file too.

Have you run this test locally at all?

yes, I run this test locally

According to the pre-merge checks, this test is failing on Linux. Please review and fix.

I've also previously said that I'd prefer CHECK lines to be immediately after the corresponding test case, which you haven't addressed here.

jh7370 · 2024-08-28T07:00:13Z

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

+# CHECK32-NEXT:Maxium alignment of .data:          0x3
+# CHECK32-NEXT:Module type;                        0x0
+# CHECK32-NEXT:CPU type of objects:                0x1
+# CHECK32-NEXT:(Reserved):                         0x0


Does a Reserved field really need printing?

thank, I deleted it.

jh7370 · 2024-08-28T07:05:13Z

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

+# RUN: python -c "with open(r'%t1_err1', 'r+b') as input: input.seek(17); input.write(b'\x45')"
+# RUN: python -c "with open(r'%t1_err1', 'r+b') as input: input.seek(4); input.write(b'\x00')"


Is there a reason these two lines can't be folded together into one python statement? You may also wish to look at using split-file, to allow you to have a proper python script, rather than these inline snippets.

thanks, combine to one line. This a one line python script, I do not think it need a split-file.

jh7370 · 2024-08-28T07:10:33Z

llvm/tools/llvm-objdump/XCOFFDump.cpp

+        ArrayRef<uint8_t>(reinterpret_cast<const uint8_t *>(&AuxHeader) +
+                              PartialFieldOffset,
+                          AuxSize - PartialFieldOffset));
+  } else if (sizeof(AuxHeader) < AuxSize)


Please review the coding standards here: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements. Note in particular the need for uniformity in an if/else-if chain.

jh7370 · 2024-08-28T07:12:14Z

llvm/tools/llvm-objdump/XCOFFDump.cpp

+    Dumper->reportUniqueWarning(Twine("only partial field for ") +
+                                PartialFieldName + " at offset (" +
+                                Twine(PartialFieldOffset) + ")");
+    Dumper->printBinary(


You are presumably aware that this will print to stdout, whereas the warning will be printed to stderr, so they may not line up together?

llvm/tools/llvm-objdump/XCOFFDump.cpp

diggerlin · 2024-09-16T19:27:38Z

My apologies for the delay in addressing the comments.

jh7370 · 2024-09-17T07:46:57Z

My apologies for the delay in addressing the comments.

No need to apologise, as I now have to apologise as I'm about to go away on PTO and won't be able to look at this for 3-4 weeks at least. If it's urgent, I don't mind others confirming my comments have been addressed and this being merged and I can follow up review. Otherwise, I'll take a look once back.

diggerlin · 2024-10-09T19:49:45Z

My apologies for the delay in addressing the comments.

No need to apologise, as I now have to apologise as I'm about to go away on PTO and won't be able to look at this for 3-4 weeks at least. If it's urgent, I don't mind others confirming my comments have been addressed and this being merged and I can follow up review. Otherwise, I'll take a look once back.

Thank you for letting me know! No problem at all. I’m happy to wait for your review when you’re back

jh7370 · 2024-10-11T08:28:05Z

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

@@ -0,0 +1,128 @@
+## Test that `the llvm-objdump --private-headers` print out the auxiliary header of XCOFF object file.


According to the pre-merge checks, this test is failing on Linux. Please review and fix.

I've also previously said that I'd prefer CHECK lines to be immediately after the corresponding test case, which you haven't addressed here.

jh7370 · 2024-10-11T08:28:57Z

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

@@ -0,0 +1,126 @@
+## Test that `the llvm-objdump --private-headers` print out the auxiliary header of XCOFF object file.


Suggested change

## Test that `the llvm-objdump --private-headers` print out the auxiliary header of XCOFF object file.

## Test that `the llvm-objdump --private-headers` prints out the auxiliary header of an XCOFF object file.

jh7370 · 2024-10-11T08:30:14Z

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

+# RUN: llvm-objdump --private-headers %t2 | \
+# RUN:   FileCheck %s --check-prefix=CHECK64 --match-full-lines --strict-whitespace
+
+## Test the auxiliary header of the XCOFF object file contains a partial field.


You aren't testing the contents of the auxiliary header, which is what this comment is saying; you're testing the behaviour of llvm-objdump when it tries to print such an auxiliary header.

Suggested change

## Test the auxiliary header of the XCOFF object file contains a partial field.

## Test how llvm-objdump behaves when the auxiliary header of the XCOFF object file contains a partial field.

Make sure to wrap the suggestion at an appropriate point.

jh7370 · 2024-10-11T08:30:46Z

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

+# RUN: python -c "with open(r'%t1_err1', 'r+b') as input: input.seek(17); input.write(b'\x45'); input.seek(4); input.write(b'\x00')"
+# RUN: llvm-objdump --private-headers %t1_err1 2>&1 | FileCheck %s --check-prefix=WARN1 --match-full-lines --strict-whitespace
+
+## Test the auxiliary header of the XCOFF object file contains extra data.


Suggested change

## Test the auxiliary header of the XCOFF object file contains extra data.

## Test how llvm-objdump behaves when the auxiliary header of the XCOFF object file contains extra data.

jh7370 · 2024-10-11T08:33:11Z

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

+  - Flags:       [ STYP_TBSS ]
+    SectionData: "1314"
+
+#      CHECK32:---Auxiliary Header:


There's a lot of duplication between the 32-bit and 64-bit checks here. Could you use a pattern of something like CHECK for common parts, and CHECK32/CHECK64 for parts that are not common, to reduce the duplication?

jh7370 · 2024-10-11T08:35:10Z

llvm/tools/llvm-objdump/XCOFFDump.cpp

+}
+
+enum PrintStyle { Hex, Number };
+template <typename T, typename V>


I'd suggest naming these template parameters to more clearly indicate what the type is supposed to be.

I'm also wondering whether this and checkAndPrintAuxHeaderParseError should be private members of the XCOFFDumper class, so that you don't need to make methods like printHex public.

jh7370 · 2024-10-11T08:41:33Z

llvm/tools/llvm-objdump/XCOFFDump.cpp

+  }
+}
+
+template <class T>


Same comment as above re. clearer name. Also, be consistent with using typename/class for type parameters in templates.

llvm/tools/llvm-objdump/XCOFFDump.cpp

…vate-headers

diggerlin · 2024-10-18T18:28:44Z

According to the pre-merge checks, this test is failing on Linux. Please review and fix.

for window, It fail at

Failed Tests (2):
   LLVM :: MC/ELF/warn-newline-in-escaped-string.s
   LLVM :: tools/llvm-rc/tag-html.test

I do not think it is related to the PR.

for linux ,it fail at

/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-n7hfh-1/llvm-project/github-pull-requests/build/test/tools/llvm-objdump/XCOFF/Output/private-header-auxiliary.test.script: line 6: python: command not found

it maybe the build environment problem. I tested the test case in the patch in linux for powerpc in my local, the test case successful.

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

jh7370

Sorry for the delay in reviewing again. A few more small comments and then I think we're done.

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test

llvm/tools/llvm-objdump/XCOFFDump.cpp

diggerlin · 2024-11-05T18:30:38Z

Sorry for the delay in reviewing again. A few more small comments and then I think we're done.

No need to apologize; thank you very much for your professional feedback!

jh7370

LGTM.

…m-objdump --private-headers (llvm#105682) Implement decoding auxiliary header of XCOFF object file with llvm-objdump --private-headers

implement decoding auxiliary header for xcoff with llvm-objdump --pri…

3a52dd2

…vate-headers

llvmbot added the llvm:binary-utilities label Aug 22, 2024

diggerlin requested review from chenzheng1030 and jh7370 August 22, 2024 15:53

diggerlin added 2 commits August 22, 2024 10:55

minor change

935775c

add more FileCheck options

e38e11f

jh7370 reviewed Aug 28, 2024

View reviewed changes

diggerlin added 2 commits September 16, 2024 19:11

address comment

8bfbee4

reorder the run commands and add comments for test case

4535a2f

diggerlin requested a review from jh7370 September 16, 2024 19:27

jh7370 reviewed Oct 11, 2024

View reviewed changes

implement decoding auxiliary header for xcoff with llvm-objdump --pri…

de6dce5

…vate-headers

jh7370 reviewed Oct 21, 2024

View reviewed changes

llvm/test/tools/llvm-objdump/XCOFF/private-header-auxiliary.test Outdated Show resolved Hide resolved

change python to %python

081f917

diggerlin requested a review from jh7370 October 22, 2024 17:14

jh7370 reviewed Oct 30, 2024

View reviewed changes

address comment

00ce595

jh7370 approved these changes Nov 6, 2024

View reviewed changes

diggerlin merged commit e373ba4 into llvm:main Nov 7, 2024
6 of 8 checks passed

		@@ -0,0 +1,128 @@
		## Test that `the llvm-objdump --private-headers` print out the auxiliary header of XCOFF object file.

		# RUN: python -c "with open(r'%t1_err1', 'r+b') as input: input.seek(17); input.write(b'\x45')"
		# RUN: python -c "with open(r'%t1_err1', 'r+b') as input: input.seek(4); input.write(b'\x00')"

		@@ -0,0 +1,126 @@
		## Test that `the llvm-objdump --private-headers` print out the auxiliary header of XCOFF object file.

	## Test the auxiliary header of the XCOFF object file contains a partial field.
	## Test how llvm-objdump behaves when the auxiliary header of the XCOFF object file contains a partial field.

[llvm-objdump] Implement decoding auxiliary header for xcoff with llvm-objdump --private-headers #105682

[llvm-objdump] Implement decoding auxiliary header for xcoff with llvm-objdump --private-headers #105682

Uh oh!

Conversation

diggerlin commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diggerlin Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jh7370 Oct 11, 2024 • edited by diggerlin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diggerlin Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

diggerlin commented Sep 16, 2024

Uh oh!

jh7370 commented Sep 17, 2024

Uh oh!

diggerlin commented Oct 9, 2024

Uh oh!

jh7370 Oct 11, 2024 • edited by diggerlin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

diggerlin commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

diggerlin commented Nov 5, 2024

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

diggerlin commented Aug 22, 2024 •

edited

Loading

diggerlin Sep 16, 2024 •

edited

Loading

jh7370 Oct 11, 2024 •

edited by diggerlin

Loading

diggerlin Sep 16, 2024 •

edited

Loading

jh7370 Oct 11, 2024 •

edited by diggerlin

Loading

diggerlin commented Oct 18, 2024 •

edited

Loading