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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:111
+  assert(SymEntPtr->SectionNumber != XCOFF::N_DEBUG &&
+         "Debug symbol name's length excess 8 bytes not yet implemented!");
+
----------------
Suggestion: `Access to symbolic debugger stabstrings from .debug section not yet implemented!`
Also, reminder re: https://reviews.llvm.org/D61532?id=198073#inline-550220.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:119
+  // or .debug section.
+  // A byte offset value less than 4 is a null or zero-length symbol name.
+  if (Offset < 4)
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > > A byte offset value of 0 is a null or zero-length symbol name.
> > Is the "less than 4" an invention of this patch? A byte offset of 1, 2, or 3 points into the length field. It is well-formed as claimed by the comment in the patch, or is it erroneous?
> in the xcoff document, it describe as  " A byte offset value of 0 is a null or zero-length symbol name."
> 
> A byte offset of 1, 2, or 3 points into the length field , the document do not talk about anything on it. we look it as "zero-length symbol name" . If we can treated  the "A byte offset of 1, 2, or 3" as error format, and return as
> make_error<GenericBinaryError>("Symbol Name offset error",object_error::parse_failed); 
> But I think looking  "A byte offset of 1, 2, or 3" as "zero-length symbol name" will make obj2yaml more compatible。
The comment should be clear as to what is part of the specification and what is part of the implementation. Suggestion:
A byte offset value of 0 is a null or zero-length symbol name. A byte offset in the range 1 to 3 (inclusive) points into the length field; as a soft-error recovery mechanism, we treat such cases as having an offset of 0.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:126
+
+  return make_error<GenericBinaryError>("Symbol Name parse failed.",
+                                        object_error::parse_failed);
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > It seems that there should be no period in the string:
> > ```
> > llvm/lib/Object/WasmObjectFile.cpp:        make_error<StringError>("Bad magic number", object_error::parse_failed);
> > ```
> deleted the period.
I still see the period.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:34
   dumpHeader();
+  if ((EC = dumpSymbols()))
+    return EC;
----------------
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`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532