[PATCH] D85774: [XCOFF][AIX] Enable tooling support for 64 bit symbol table parsing

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:171
+    return Entry32 ? reinterpret_cast<uintptr_t>(Entry32)
+                   : reinterpret_cast<uintptr_t>(Entry64);
+  }
----------------
DiggerLin wrote:
> what about return reinterpret_cast<uintptr_t>(Entry32 ? Entry32 : Entry64) ?
No, you could not do that. It only works if Entry32 and Entry64 are the same type. But they are not here.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:426
+
+  static uintptr_t getAdvancedSymbolEntryAddress(uintptr_t CurrentAddress,
+                                                 uint32_t Distance);
----------------
DiggerLin wrote:
> not sure we want to Distance to be negative value future? I think change to int32_t Distance, means that we can backward 
I don't see a need to jump backward now. If it's needed in the future, we could always change in the future patch.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:433
+typedef struct {
+  uint8_t LanguageId;
+  uint8_t CpuTypeId;
----------------
DiggerLin wrote:
> not sure  whether we want to define a enum for the LanguageID in this patch.
> The values for this field are defined in the e_lang field in "Exception Section" 
> 
I think we are already doing enum mapping in tools/llvm-readobj/XCOFFDumper.cpp. I don't see a strong need to create an enum for it.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:490
 
-  XCOFF::StorageClass getStorageClass() const;
-  uint8_t getNumberOfAuxEntries() const;
-  const XCOFFCsectAuxEnt32 *getXCOFFCsectAuxEnt32() const;
-  uint16_t getType() const;
-  int16_t getSectionNumber() const;
+  uint64_t getValue() const { return Entry32 ? getValue32() : getValue64(); }
 
----------------
DiggerLin wrote:
> using GETVALUE(Value) for consistent ?
I would prefer to be more explicit here because we are doing a conversion to larger value for 32 bit version, which is different from the rest of GETVALUE(Value).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:775
+  // A function definition should be a label definition.
+  if (CsectAuxRef.isLabel())
+    return false;
----------------
DiggerLin wrote:
> if we not enable  -ffunction-sections , function entry is label.  
Thanks. I brought back the old behavior and added the FIXME to say that this function does not return a correct value if we have -ffunction-sections enabled. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:815
 
-  // A function definition should be a label definition.
-  if ((CsectAuxEnt->SymbolAlignmentAndType & SymTypeMask) != XCOFF::XTY_LD)
-    return false;
+  auto getSymbolAuxType = [this](uintptr_t AuxAddr) {
+    return viewAs<XCOFF::SymbolAuxType>(
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > create  a static function getSymbolAuxType in a file scope maybe  better? 
> > All the Aux symbol of 64bit all has the AuxType .  we can use the function for other type too in other place later ?
> there already has a function on XCOFFCsectAuxRef ::getAuxType64() 
Yes, there is a XCOFFCsectAuxRef ::getAuxType64(), but if you notice, this function is used to create an XCOFFCsectAuxRef object. So you don't have that function available in the creator. 
And I don't think a static function is needed, because when you created XCOFFCsectAuxRef object through this function, then you could call the XCOFFCsectAuxRef ::getAuxType64() to get your type. So this lambda should only exists in this function. 


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

https://reviews.llvm.org/D85774