[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:100
+  Symb.p = reinterpret_cast<uintptr_t>(SymEntPtr);
   return;
 }
----------------
sfertile wrote:
> this is unneeded.
There's still a stray `return` statement here.


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


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:20
+#include <cstdint>
+#include <vector>
 
----------------
Both of the above includes are included directly by `XCOFFYAML.h` for which this is the implementation file.


================
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:
> 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);
```
?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532