[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/include/llvm/Object/XCOFFObjectFile.h:170
+
+  // Note that return value is signed and might return a negative value.
+  // Negative values are reserved for future use.
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > I would suggest rewording this, maybe
> > > `// The value as encoded in the object file.`
> > > 
> > > I think that + the following line is enough to convey negative values *are* allowed/expected.
> > changed comment as suggestion
> s/Note that return/Returns/;
fixed. thanks


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:174
+	
+  // Note that return value is unsigned,a negative value will return as zero.
+  uint32_t getLogicalNumberOfSymbolTableEntries() const;
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > maybe: `// Sanitized value, useable as an index into the symbol table.` instead.
> > fixed
> s/Return/Returns a/;
fixed , thanks


================
Comment at: llvm/test/tools/obj2yaml/aix_xcoff.test:17
+# CHECK-NEXT:     Section: N_DEBUG
+# CHECK-NEXT:     Type:      0x0003
+# CHECK-NEXT:     StorageClass:    C_FILE
----------------
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);
}



================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:64
+
+    Sym.Value = SymbolEntPtr->Value;
+
----------------
hubert.reinterpretcast wrote:
> Length mismatch.
> ```
> llvm::yaml::Hex16 Value; // Symbol value; storage class-dependent.
> ```
> ```
> support::ubig32_t Value; // Symbol value; storage class-dependent.
> ```
> 
thanks for point out. fixed


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532