[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:110
+  uint32_t Offset = SymEntPtr->NameInStrTbl.Offset;
+  // The byte offset is relative to the start of the string table
+  // or .debug section.
----------------
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`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:57
 
+static StringRef generateStringRef(const char *Name, uint64_t size) {
+  auto NulCharPtr = static_cast<const char *>(memchr(Name, '\0', size));
----------------
Use `Size` instead of `size` to match `Name` and also the corresponding parameter in `checkSize`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:108
+
+  if (SymEntPtr->NameInStrTbl.Magic != 0)
+    return generateStringRef(SymEntPtr->SymbolName, XCOFF::SymbolNameSize);
----------------
Hardcoded `0` should be replaced with something that indicates the semantic: `XCOFFSymbolEntry::NAME_IN_STR_TBL_MAGIC` or at least `0x00000000`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:112
+  if (SymEntPtr->SectionNumber == XCOFF::N_DEBUG) {
+    llvm_unreachable("Not yet implemented!");
+    return StringRef(nullptr, 0);
----------------
If an error recovery path is thought to be necessary, then `llvm_unreachable` is the wrong answer. Since the error recovery seems reasonable, use `assert` instead.


================
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)
----------------
> 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?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:124
+  if (StringTable.Data != nullptr)
+    return (StringTable.Data + Offset);
+
----------------
Check that the `Offset` is within the string table.


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532