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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Some nits, but the idea of implementing expand relocs behaviour here seems reasonable to me. This will likely be my last post on this review until I'm back at the end of January. Feel free to not wait any further on me.



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:317
+                  << center_justify("Type", 14) << center_justify(" SectNum", 8)
+                  << center_justify("SymbolName(Index)", 20) << "\n";
+
----------------
Text intended to be read by humans should have spaces between words...


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:347
+
+    if (opts::ExpandRelocs) {
+      DictScope DS(W, "Relocation");
----------------
Perhaps worth putting the ExpandRelocs behaviour into a small helper function (not a lambda), just to avoid making this function too long.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:356-358
+        // The relocation encodes the bit length being relocated minus 1. Add
+        // back
+        //   the 1 to get the actual length being relocated.
----------------
Word wrapping has gone weird here.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:316
+    W.startLine() << center_justify("Vaddr", Obj.is64Bit() ? 18 : 10)
+                  << center_justify("Type", 8) << center_justify(" RelSect", 8)
+                  << center_justify("SymbolName(Index)", 20) << "\n";
----------------
Esme wrote:
> We seem to be more accustomed to abbreviating Section as Sec, is it more meaningful to change `RelSect` to `SecNum`?
I would actually here print "Section" or "SectionIndex". Ultimately though, do the same as we do for ELF, I recommend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136787