[PATCH] D135887: [XOCFF] llvvm-readobj support display symbol table of loader section of xcoff object file.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Typo in the patch topic: `[XOCFF]` -> `[XCOFF]`.



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:247
+  Expected<StringRef>
+  getSymbolName(const LoaderSectionHeader32 *LDHeader) const;
+};
----------------
Here and throughout: I'd change `LDHeader` to `LoaderHeader`, as it avoids the need for metal gymnastics to underestand what "LD" means here.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:93
+template <typename T>
+Expected<StringRef> getLoadSectionSymName(const T *LDHeader, uint64_t Offset) {
+  if (LDHeader->LengthOfStrTbl > Offset)
----------------



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:106-107
+    const LoaderSectionHeader32 *LDHeader32) const {
+  if (NameInStrTbl.Magic != XCOFFSymbolRef::NAME_IN_STR_TBL_MAGIC)
+    return generateXCOFFFixedNameStringRef(SymbolName);
+
----------------
This is undefined behaviour. From [[ https://en.cppreference.com/w/cpp/language/union | cppreference.com ]]: 

> It is undefined behavior to read from the member of the union that wasn't most recently written.

In other words, your design of the struct isn't guaranteed to work according to the C++ coding standard. You need to change it so that you can use a different mechanism to identify whether the name is in the string table or not.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/loader-section-symbol-invalid.test:38
+
+
+#CHECK: warning: '[[FILE]]': entry with offset 0xa2 in the loader section's string table with size 0xc is invalid
----------------
Get rid of extra blank line.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/loader-section-symbol-invalid.test:39
+
+#CHECK: warning: '[[FILE]]': entry with offset 0xa2 in the loader section's string table with size 0xc is invalid
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:159
+
+  // TODO: Need to relocation entry of loader section later.
   // For example:
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:214
+void XCOFFDumper::printLoaderSectionSymbols(uintptr_t LoaderSectionAddr) {
+
+  DictScope DS(W, "Loader Section Symbols");
----------------
As asked before, please take more care with your code before uploading for review. I have repeatedly pointed out that functions should not start with a blank line.

I'm sorry, but there may come a point where I simply won't be willing to review the patch until you've got rid of all the trivial errors like this, because it wastes my time reviewing it and having to comment on all these issues every time.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:220-225
+
+    if (!SymbolNameOrErr) {
+      reportUniqueWarning(SymbolNameOrErr.takeError());
+      return;
+    }
+    DictScope DS(W, "Symbol");
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:243
+    for (uint32_t i = 0; i < LoaderSec64->NumberOfSymTabEnt;
+         i++, LDSymEntPtr64++)
+      PrintLoadSecSymbolCommon(LDSymEntPtr64, LoaderSec64);
----------------
https://llvm.org/docs/CodingStandards.html#prefer-preincrement


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:246
+
+  } else {
+    const LoaderSectionHeader32 *LoaderSec32 =
----------------
As far as I can tell, the else block here is identical to the if part, except for different types. Put them into a helper template function to avoid code duplication.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:253
+    for (uint32_t i = 0; i < LoaderSec32->NumberOfSymTabEnt;
+         i++, LDSymEntPtr32++)
+      PrintLoadSecSymbolCommon(LDSymEntPtr32, LoaderSec32);
----------------
https://llvm.org/docs/CodingStandards.html#prefer-preincrement


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135887