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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 26 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:122
   void moveSectionNext(DataRefImpl &Sec) const override;
-  Expected<StringRef> getSectionName(DataRefImpl Sec) const override;
+  std::error_code getSectionName(DataRefImpl Sec,
+                                 StringRef &Res) const override;
----------------
MaskRay wrote:
> You need to rebase. The signature is `Expected<StringRef> getSectionName(DataRefImpl Sec) const override;` now. 
I rebased it, thanks


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:155
 
+public:
   XCOFFObjectFile(MemoryBufferRef Object, std::error_code &EC);
----------------
sfertile wrote:
> Why is this needed?
I added addition public to separate the functions which are inherited from objectfile class from the functions api which only in xcoffobjectfile. it more readable.


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


================
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.
----------------
sfertile wrote:
> 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.
changed comment as suggestion


================
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;
----------------
sfertile wrote:
> maybe: `// Sanitized value, useable as an index into the symbol table.` instead.
fixed


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:97
+  const XCOFFSymbolEntry *SymEntPtr = toSymbolEntry(Symb);
+
+  SymEntPtr += SymEntPtr->NumberOfAuxEntries + 1;
----------------
sfertile wrote:
> I would get rid of this white space.
I always put blank line between a local variable define(declare) and function code, I think it maybe more readable.


================
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.
----------------
sfertile wrote:
> 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.
I am agred with you. we do not support the debug related functionality this moment.  I think I need to modify the function getSymbolName() when we support debug functionality. the symbol name of the debug symbol are in the dbx Stabstrings of debug section.


================
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);
+
----------------
sfertile wrote:
> Why not just `return toSymbolEntry(Symb)->Value;`
good idea, changed as suggestion.


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


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:438
+  StringTable.Size =
+      *reinterpret_cast<const support::ubig32_t *>(base() + CurPtr);
+
----------------
MaskRay wrote:
> Use `support::endian::read32be` in `llvm/Support/Endian.h`
change as suggestion


================
Comment at: llvm/test/tools/obj2yaml/aix_xcoff.test:17
+# CHECK-NEXT:     Section: N_DEBUG
+# CHECK-NEXT:     Type:      0x0003
+# CHECK-NEXT:     StorageClass:    C_FILE
----------------
sfertile wrote:
> 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.
for the N_ABS, N_DEBUG, N_UNDEF, which do not has section related to it, It not  show section number here.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:22
   void dumpHeader();
+  std::error_code dumpSymbols();
 
----------------
MaskRay wrote:
> For new code, prefer `Error` `Expected<T>` to `std::error_code`. The former enforces error checking and provides better error messages. Avoid `errorToErrorCode` if possible, it is very common to lost messages after converting to an `Error`.
if the dumpSymbols return a value other than std::error_code, I can try to use Expected<T>, what T should be in the this case?
same situation for the function std::error_code XCOFFDumper::dump() .

the even I have Expected<T> XCOFFDumper::dump() , the error information will still lose in function std::error_code xcoff2yaml(raw_ostream &Out,

const object::XCOFFObjectFile &Obj) {
and static std::error_code dumpObject(const ObjectFile &Obj) {


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:50
 
+std::error_code XCOFFDumper::dumpSymbols() {
+  std::vector<XCOFFYAML::Symbol> &Symbols = YAMLObj.Symbols;
----------------
MaskRay wrote:
> You may want to use `Expected<...>` `Error` instead of `std::error_code`, then you can avoid conversion like `errorToErrorCode()`
if the dumpSymbols return a value other than std::error_code, I can try to use Expected<T>,  what T should be in the this case?
same situation for the function  std::error_code XCOFFDumper::dump() .

the even I have Expected<T>  XCOFFDumper::dump()  , the error information will still lose in function std::error_code xcoff2yaml(raw_ostream &Out,
                           const object::XCOFFObjectFile &Obj) {
and static std::error_code dumpObject(const ObjectFile &Obj) {


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:69
+    if (!SectionNameRefOrErr) {
+      return errorToErrorCode(SectionNameRefOrErr.takeError());
+    }
----------------
MaskRay wrote:
> redundant braces.
deleted them


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532