[PATCH] D136787: [XCOFF] Decode the relocation entries of loader section of xcoff for llvm-readobj

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:253
+  support::ubig32_t LengthOfStrTbl;
+  support::big32_t OffsetToStrTbl;
+
----------------
I was just looking at the XCOFF loader section header spec, and it refers to a `l_rdoff` field, i.e. the offset of the relocation entries, but this struct here doesn't seem to define an equivalent field. Is this not supposed to represent the actual loader header struct in the file? It seems to me like it is intended to, due to the way the symbol table offset is calculated. (Admittedly, the description for this field contradicts the point of having the field, so I'm not entirely sure what to make of the spec).


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:158
 
   // Only implement for XCOFF
   virtual void printAuxiliaryHeader() {}
----------------
Just spotted errors in this comment. See the Mach-O example below.


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:162
+  virtual void printLoaderSection(bool PrintHeader, bool PrintSymbolTable,
+                                  bool PrintRelocation) {}
 
----------------
This field should be `PrintRelocations` because there can be more than one relocation.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:44
+  void printLoaderSection(bool PrintHeader, bool PrintSymbolTable,
+                          bool PrintRelocation) override;
 
----------------
Same as above (`PrintRelocations`). Also throughout the changes in this file.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:281
+
+// There are five implicit external symbols, one each for the .text, .data,
+// .bss, .tdata, and .tbss sections. These symbols are referenced from the
----------------
It might be worth noting that this is a direct quote from the specification. Perhaps adding something like "From the XCOFF specification: there are five ...".

I'd be concerned putting in a detailed quote like this without any form of attribution.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:306
+    uintptr_t LoaderSectionAddr) {
+
+  auto PrintLoaderSecRelocationSymIndex = [&](const auto *LoaderSecHeader,
----------------
Nit


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:307
+
+  auto PrintLoaderSecRelocationSymIndex = [&](const auto *LoaderSecHeader,
+                                              const auto *LoaderSecRelEnt,
----------------
This lambda is only used once. What's the motivation for it being a lambda, rather than putting the code directly inline?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:320
+
+  auto GetLoaderSecRelSymEntPtr = [&](const auto &LoaderSecHeader,
+                                      const auto &LoaderSecRelEnt,
----------------
Again, this lambda is only used once. What's preventing the code being directly written inline?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:323-325
+    // Because there are implicit symbol index (-2,-1,0,1,2),
+    // LoaderSecRelEnt.SymbolIndex - 3 will get real symbol information from
+    // symboltable.
----------------
Please make sure to reflow again after making the suggested edits.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:345
+    if (Obj.is64Bit()) {
+      W.printEnum("Type", (uint8_t)LoaderSecRelEntPtr->Type,
+                  makeArrayRef(RelocationTypeNameclass));
----------------
Prefer `static_cast` not C-style casts.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:347
+                  makeArrayRef(RelocationTypeNameclass));
+      W.printNumber("SectionNUmber", LoaderSecRelEntPtr->SectionNum);
+    }
----------------
Typo. This appears in the output. Did you just copy the output of the tool to write your test? Be careful...


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:350
+
+    if (LoaderSecRelEntPtr->SymbolIndex >= 3) {
+      const LoaderSectionSymbolEntry *LoaderSecRelSymEntPtr =
----------------
It would be worth factoring this "3" into a named constant in the function. Something like `FirstSymbolIndex`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:357
+                                       LoaderSecRelSymEntPtr);
+
+    } else
----------------
Nit: Don't end blocks with a blank line.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:364
+
+    if (!Obj.is64Bit()) {
+      W.printEnum("Type", (uint8_t)LoaderSecRelEntPtr->Type,
----------------
I'm not sure I follow why this and the `if(Obj.is64Bit())` cases need distinguishing?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:365
+    if (!Obj.is64Bit()) {
+      W.printEnum("Type", (uint8_t)LoaderSecRelEntPtr->Type,
+                  makeArrayRef(RelocationTypeNameclass));
----------------
Use `static_cast`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:367
+                  makeArrayRef(RelocationTypeNameclass));
+      W.printNumber("SectionNUmber", LoaderSecRelEntPtr->SectionNum);
+    }
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136787/new/

https://reviews.llvm.org/D136787