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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin 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;
----------------
jhenderson wrote:
> 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).
 We already define "NameOffset" in the LoaderSectionSymbolEntry32, so we do not need the "Symbol" in SymbolNameOffset.  I change to use the NameOffsetInStrTbl.


================
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:
> 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.
I used the abbr Name in function name , otherwise  the function name is too long. For example:
Sec for Section.
StrTbl for StringTable.
Load for Loader.





================
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.
+
----------------
jhenderson wrote:
> 
thanks


================
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()));
----------------
jhenderson wrote:
> Do these two calculations need checking to make sure they are within the file (in case of malformed input)?
I added check code for it . thanks 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135887