[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:57
 
+static StringRef generateStringRef(const char *Name, uint64_t size) {
+  auto NulCharPtr = static_cast<const char *>(memchr(Name, '\0', size));
----------------
hubert.reinterpretcast wrote:
> Use `Size` instead of `size` to match `Name` and also the corresponding parameter in `checkSize`.
fixed , thanks


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


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:112
+  if (SymEntPtr->SectionNumber == XCOFF::N_DEBUG) {
+    llvm_unreachable("Not yet implemented!");
+    return StringRef(nullptr, 0);
----------------
hubert.reinterpretcast wrote:
> 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.
we do not support xcoff object file which generated by -g currently. we will support it later, and symbol name in the .debug section  later. So I think use  llvm_unreachable maybe more reasonable.


================
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:
> > 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。


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:124
+  if (StringTable.Data != nullptr)
+    return (StringTable.Data + Offset);
+
----------------
hubert.reinterpretcast wrote:
> Check that the `Offset` is within the string table.
Good idea, added  offset check with the string table size. thanks


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


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:443
+  // field
+  if (!checkSize(Data, EC, CurPtr, (StringTable.Size)))
+    return;
----------------
hubert.reinterpretcast wrote:
> Why the extra parentheses around `StringTable.Size`?
deleted parentheses


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:100
+  Symb.p = reinterpret_cast<uintptr_t>(SymEntPtr);
   return;
 }
----------------
hubert.reinterpretcast wrote:
> sfertile wrote:
> > this is unneeded.
> There's still a stray `return` statement here.
deleted


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:20
+#include <cstdint>
+#include <vector>
 
----------------
hubert.reinterpretcast wrote:
> Both of the above includes are included directly by `XCOFFYAML.h` for which this is the implementation file.
good point, I deleted all redundant header files


================
Comment at: llvm/test/tools/obj2yaml/aix_xcoff.test:17
+# CHECK-NEXT:     Section: N_DEBUG
+# CHECK-NEXT:     Type:      0x0003
+# CHECK-NEXT:     StorageClass:    C_FILE
----------------
hubert.reinterpretcast wrote:
> The question for this line is whether printing as a single type field is appropriate. This is a `C_FILE` entry where `n_lang` is `C (0x00)` and `n_cputype` is `COM (0x03)`. That is, perhaps we should have inspected `CFileLanguageIdAndTypeId`?
The field name of yaml output are defined in the function, Once it is defined , we can not change the field name based on the value.

void MappingTraits<XCOFFYAML::Symbol>::mapping(IO &IO, XCOFFYAML::Symbol &S) {

IO.mapRequired("Name", S.SymbolName);
IO.mapRequired("Value", S.Value);
IO.mapRequired("Section", S.SectionName);
IO.mapRequired("Type", S.Type);
IO.mapRequired("StorageClass", S.StorageClass);
IO.mapRequired("NumberOfAuxEntries", S.NumberOfAuxEntries);
}


================
Comment at: llvm/test/tools/obj2yaml/aix_xcoff.test:17
+# CHECK-NEXT:     Section: N_DEBUG
+# CHECK-NEXT:     Type:      0x0003
+# CHECK-NEXT:     StorageClass:    C_FILE
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > DiggerLin wrote:
> > > sfertile wrote:
> > > > Is the intent to always keep dumping the type as a numeric value? For this tool I think that's reasonable but I'd like to clarify.
> > > for the N_ABS, N_DEBUG, N_UNDEF, which do not has section related to it, It not  show section number here.
> > Thanks for Hubert let me I make a wrong answer. 
> >  
> > 1. for the question of " Is the intent to always keep dumping the type as a numeric value? For this tool I think that's reasonable but I'd like to clarify."
> > 
> > Answer:
> > in the xcoff document ,
> > n_type :
> > Bits 0-3
> > Symbol visibility. The SYM_V_MASK macro, with the value 0xF000, can be used to mask off bits in the n_type field that do not specify visibility. The following visibilities are defined:
> > 0x1000	SYM_V_INTERNAL
> > 0x2000	SYM_V_HIDDEN
> > 0x3000	SYM_V_PROTECTED
> > 0x4000	SYM_V_EXPORTED
> > 
> > Bit 10
> > Optionally set to 1 if the symbol is a function. Otherwise, it is set to 0.
> > 
> > I can not use enum to represent the value. I have to display as a value.
> > 
> > 2. For the question "This is a C_FILE entry where n_lang is C (0x00) and n_cputype is COM (0x03). That is, perhaps we should have inspected CFileLanguageIdAndTypeId?"
> > 
> > Answer:  I do not think yaml framework support showing different field name based on the value of the field.  
> > 
> > The field name of yaml output are defined in the function, Once it is defined , we can not change the field name based on the value.
> > 
> > void MappingTraits<XCOFFYAML::Symbol>::mapping(IO &IO, XCOFFYAML::Symbol &S) {
> >   IO.mapRequired("Name", S.SymbolName);
> >   IO.mapRequired("Value", S.Value);
> >   IO.mapRequired("Section", S.SectionName);
> >   IO.mapRequired("Type", S.Type);
> >   IO.mapRequired("StorageClass", S.StorageClass);
> >   IO.mapRequired("NumberOfAuxEntries", S.NumberOfAuxEntries);
> > }
> > 
> What happens with something like:
> ```
> llvm/lib/ObjectYAML/COFFYAML.cpp:  IO.mapOptional("SymbolTableIndex", Rel.SymbolTableIndex);
> ```
> ?
when you did the first comment on it , I think over to use  IO.mapOptional, But I denied  it in final,   "n_type"  is not optional field in the xcoff object file. if I use optional field for the CFileLanguageIdAndTypeId , it means that CFileLanguageIdAndTypeId  is optional field of symbol entry , It may cause confuse, it actually only a different  interpret of the n_type.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532