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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
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.
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > 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.
> > By convention, a storage class value with the high-order bit on indicates that this field is a symbolic debugger stabstring.
> The above means that the check should be for `SectionNumber < 0`.
thanks for the information, using "By convention, a storage class value with the high-order bit on indicates that this field is a symbolic debugger stabstring." is more exact than  assert(SymEntPtr->SectionNumber != XCOFF::N_DEBUG)






================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:34
   dumpHeader();
+  if ((EC = dumpSymbols()))
+    return EC;
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > What gets returned when this is false?
> > if symbol name parse failed . 
> > it return std::error_code which value is 3 . defined in 
> > Error.h 
> > 
> > enum class object_error {
> >   // Error code 0 is absent. Use std::error_code() instead.
> >   arch_not_found = 1,
> >   invalid_file_type,
> >   parse_failed,
> >   unexpected_eof,
> >   string_table_non_null_end,
> >   invalid_section_index,
> >   bitcode_section_not_found,
> >   invalid_symbol_index,
> > };
> Sean is pointing out that there is no return statement in the case where there is no error on the call to `dumpSymbols`.
thanks for clarify the question, fixed it.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:50
 
+std::error_code XCOFFDumper::dumpSymbols() {
+  std::vector<XCOFFYAML::Symbol> &Symbols = YAMLObj.Symbols;
----------------
MaskRay wrote:
> DiggerLin wrote:
> > 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) {
> Return `Error`.
thanks for the information.  But I think I still keep std::error_code XCOFFDumper::dumpSymbols() as it now. I will schedule  a new patch after this patch upstream. 

the new patch will change the high level interface 
static std::error_code dumpObject(const ObjectFile &Obj) {
  if (Obj.isCOFF())
    return coff2yaml(outs(), cast<COFFObjectFile>(Obj));

  if (Obj.isXCOFF())
    return xcoff2yaml(outs(), cast<XCOFFObjectFile>(Obj));

  if (Obj.isELF())
    return elf2yaml(outs(), Obj);
  if (Obj.isWasm())
    return wasm2yaml(outs(), cast<WasmObjectFile>(Obj));

  return obj2yaml_error::unsupported_obj_file_format;
}

to 

Expected<Error>  dumpObject(const ObjectFile &Obj)  as your suggestion.

if change to Expected<Error>  dumpObject(const ObjectFile &Obj). I also need to modified the function 
coff2yaml();
elf2yaml();
wasm2yaml();


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532