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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:158
+      return Entry32->ParameterHashIndex;
+    return Entry64->ParameterHashIndex;
+  }
----------------
jasonliu wrote:
> DiggerLin wrote:
> > change to 
> > return Entry32 ? Entry32->ParameterHashIndex : Entry64->ParameterHashIndex
> > 
> > and change in the following functions too ?
> I was initially worried about MSVC breakage here: https://github.com/llvm/llvm-project/commit/210314ae8c59bc0a8801c2528eda892cd5960c31
> But after taking a closer look, it seems to be only a problem for conversion from ubig32_t value to a unit64_t, which does not apply here. 
> So I will give it a try.
thanks let me know.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:171
+    return Entry32 ? reinterpret_cast<uintptr_t>(Entry32)
+                   : reinterpret_cast<uintptr_t>(Entry64);
+  }
----------------
what about return reinterpret_cast<uintptr_t>(Entry32 ? Entry32 : Entry64) ?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:204
+      return Entry32->SymbolAlignmentAndType;
+    return Entry64->SymbolAlignmentAndType;
+  }
----------------
ruse GETVALUE(SymbolAlignmentAndType) ?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:426
+
+  static uintptr_t getAdvancedSymbolEntryAddress(uintptr_t CurrentAddress,
+                                                 uint32_t Distance);
----------------
not sure we want to Distance to be negative value future? I think change to int32_t Distance, means that we can backward 


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:433
+typedef struct {
+  uint8_t LanguageId;
+  uint8_t CpuTypeId;
----------------
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" 



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:482
+    assert(OwningObjectPtr && "OwningObjectPtr can not be nullptr!");
+    assert(SymEntDataRef.p != 0 && "Symbol table pointer can not be nullptr!");
+
----------------
"Symbol table pointer can not be nullptr!" --> "Symbol table entry pointer can not be nullptr!"


================
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(); }
 
----------------
using GETVALUE(Value) for consistent ?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:216
 XCOFFObjectFile::getSymbolSection(DataRefImpl Symb) const {
-  const XCOFFSymbolEntry *SymEntPtr = toSymbolEntry(Symb);
-  int16_t SectNum = SymEntPtr->SectionNumber;
+  int16_t SectNum = toSymbolRef(Symb).getSectionNumber();
 
----------------
const int16_t SectNum ? 



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:477
+XCOFFObjectFile::getSymbolSectionName(XCOFFSymbolRef SymEntPtr) const {
+  int16_t SectionNum = SymEntPtr.getSectionNumber();
 
----------------
const int16_t SectionNum ?


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

https://reviews.llvm.org/D85774