[PATCH] D61532: implement of the parsing symbol table for xcoffobjfile and output as yaml format

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:155
 
+public:
   XCOFFObjectFile(MemoryBufferRef Object, std::error_code &EC);
----------------
Why is this needed?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:162
+  }
+  Expected<StringRef>
+  getSymbolSectionName(const XCOFFSymbolEntry *SymEntPtr) const;
----------------
Add a blank line before this one please.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:165
+  const XCOFFSymbolEntry *toSymbolEntry(DataRefImpl Ref) const;
   uint16_t getMagic() const;
   uint16_t getNumberOfSections() const;
----------------
Another blank line before this one as well please.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:170
+
+  // Note that return value is signed and might return a negative value.
+  // Negative values are reserved for future use.
----------------
I would suggest rewording this, maybe
`// The value as encoded in the object file.`

I think that + the following line is enough to convey negative values *are* allowed/expected.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:174
+	
+  // Note that return value is unsigned,a negative value will return as zero.
+  uint32_t getLogicalNumberOfSymbolTableEntries() const;
----------------
maybe: `// Sanitized value, useable as an index into the symbol table.` instead.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:97
+  const XCOFFSymbolEntry *SymEntPtr = toSymbolEntry(Symb);
+
+  SymEntPtr += SymEntPtr->NumberOfAuxEntries + 1;
----------------
I would get rid of this white space.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:100
+  Symb.p = reinterpret_cast<uintptr_t>(SymEntPtr);
   return;
 }
----------------
this is unneeded.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:110
+  uint32_t Offset = SymEntPtr->NameInStrTbl.Offset;
+  // The byte offset is relative to the start of the string table
+  // or .debug section.
----------------
How do we know when the index is into the string table vs. into  a .debug section? If we have a symbol where the name *is* in the debug section we still return as StringRef to the sting table which is a problem.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:130
 uint64_t XCOFFObjectFile::getSymbolValueImpl(DataRefImpl Symb) const {
-  uint64_t Result = 0;
-  llvm_unreachable("Not yet implemented!");
-  return Result;
+  const XCOFFSymbolEntry *SymEntPtr = toSymbolEntry(Symb);
+
----------------
Why not just `return toSymbolEntry(Symb)->Value;`


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:335
 
+uint16_t XCOFFObjectFile::getMagic() const { return FileHdrPtr->Magic; }
+
----------------
Keep this with the other file header related accessors.


================
Comment at: llvm/test/tools/obj2yaml/aix_xcoff.test:17
+# CHECK-NEXT:     Section: N_DEBUG
+# CHECK-NEXT:     Type:      0x0003
+# CHECK-NEXT:     StorageClass:    C_FILE
----------------
Is the intent to always keep dumping the type as a numeric value? For this tool I think that's reasonable but I'd like to clarify.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61532