[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/lib/Object/XCOFFObjectFile.cpp:106-107
+    const LoaderSectionHeader32 *LDHeader32) const {
+  if (NameInStrTbl.Magic != XCOFFSymbolRef::NAME_IN_STR_TBL_MAGIC)
+    return generateXCOFFFixedNameStringRef(SymbolName);
+
----------------
jhenderson wrote:
> 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.
thanks for point out.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:214
+void XCOFFDumper::printLoaderSectionSymbols(uintptr_t LoaderSectionAddr) {
+
+  DictScope DS(W, "Loader Section Symbols");
----------------
jhenderson wrote:
> 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.
sorry about  that. Actually I always take a careful review when I uploaded the code.  But sorry that there are still some trivial error. I will put more work on the review when uploaded. 


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:246
+
+  } else {
+    const LoaderSectionHeader32 *LoaderSec32 =
----------------
jhenderson wrote:
> 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.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135887