[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:119
+  // or .debug section.
+  // A byte offset value less than 4 is a null or zero-length symbol name.
+  if (Offset < 4)
----------------
sfertile wrote:
> DiggerLin wrote:
> > 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
> Could we have a valid offset of less than 4 if the name is in the debug section? I'm not suggesting we need to change in this patch, just considering the future.
Great question.

In `llvm/test/tools/llvm-readobj/Inputs/xcoff-basic.o`:
```
0000620:                          0000 0000 0000
0000630: 0002 0000 0000 fffe 0000 8c00
```

The offset into the debug section is 2.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:115
+  if (SymEntPtr->StorageClass & 0x80)
+    return make_error<StringError>(
+        "XCOFF object file with debug information not yet supported",
----------------
sfertile wrote:
> hubert.reinterpretcast wrote:
> > I'm not sure what asserting on a asserts-enabled build but generating a hard-error on a release build buys us. If we are okay with a hard error, then we can just go with it for all builds. If we are not okay with a hard error, then we don't want the debug build to trip either (and a TODO comment with some recovery path is the right answer).
> > 
> > Note that `llvm/test/tools/llvm-readobj/Inputs/xcoff-basic.o` has such symbols.
> > 
> >Note that llvm/test/tools/llvm-readobj/Inputs/xcoff-basic.o has such symbols.
> 
> We need to  commit to implementing the debug name handling either in the same patch that enables dumping symbols from readobj or before that. It would be useless if we have to tip-toe around object files with debug info. Until then I think the assert alone is fine.
Printing unrelated strings or reporting spurious errors in release is not fine in my book. The hard-error alone in fine (no need for an assert).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532