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

[Digger via Phabricator via llvm-commits]

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


================
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)
----------------
hubert.reinterpretcast wrote:
> 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.
changed the comment as suggestion


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:126
+
+  return make_error<GenericBinaryError>("Symbol Name parse failed.",
+                                        object_error::parse_failed);
----------------
hubert.reinterpretcast wrote:
> 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.
the period gone now


================
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!");
+
----------------
hubert.reinterpretcast wrote:
> Suggestion: `Access to symbolic debugger stabstrings from .debug section not yet implemented!`
> Also, reminder re: https://reviews.llvm.org/D61532?id=198073#inline-550220.
fixed. thanks


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532