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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:229-232
+  typedef struct {
+    support::big32_t Magic; // Zero indicates name in string table.
+    support::ubig32_t Offset;
+  } NameInStrTblType;
----------------
Use C++ style struct naming. I'm not a big fan of the struct name either. We don't generally call any of our types "...Type", since that part of the name is typically redundant (it will be used as a type or not as a type, so the usage tells us it's a type). How about simply "SymbolNameOffset"? The usage will make it clear it's not a pure integer, I think. Other ideas are welcome too.

(If the XCOFF spec has a sepcific name for this struct, then it's fine to use that name, I guess).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:93
+template <typename T>
+Expected<StringRef> getLoadSectionSymName(const T *LDHeader, uint64_t Offset) {
+  if (LDHeader->LengthOfStrTbl > Offset)
----------------
jhenderson wrote:
> 
Marked as done, but this comment hasn't been addressed. Please don't mark comments as done if you haven't done them.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/loader-section-symbol-invalid.test:1
+## Test invalid offest to symbol string table of loader section  for --loader-section-symbols option.
+
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:217-221
+  const LoaderSectionHeader *LoadSecHeader =
+      reinterpret_cast<const LoaderSectionHeader *>(LoaderSectionAddr);
+  const LoaderSectionSymbolEntry *LoadSecSymEntPtr =
+      reinterpret_cast<LoaderSectionSymbolEntry *>(
+          LoaderSectionAddr + uintptr_t(LoadSecHeader->getOffsetToSymTbl()));
----------------
Do these two calculations need checking to make sure they are within the file (in case of malformed input)?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:159
+
+  // TODO: Need to relocation entry of loader section later.
   // For example:
----------------
jhenderson wrote:
> 
Updated comment still doesn't completely match my suggestion. Please fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135887