[PATCH] D65240: [XCOFF][AIX] Generate symbol table entries with llvm-readobj

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:162
+  if (SymEntPtr->NameInStrTbl.Magic != XCOFFSymbolEntry::NAME_IN_STR_TBL_MAGIC)
+    return generateStringRef(SymEntPtr->SymbolName);
+
----------------
hubert.reinterpretcast wrote:
> `generateStringRef` got modified in an NFC patch I believe. The uses in this patch make it more obvious that the one-argument version of that function should not be named so generically. Perhaps `generateXCOFFFixedNameStringRef`?
Agree. The name should give more information on what it is trying to do. 


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:310
+    StatAuxEntPtr =
+        reinterpret_cast<const XCOFFSectAuxEntForStat *>(SymbolEntPtr + 1);
+    printSectAuxEntForStat(StatAuxEntPtr);
----------------
hubert.reinterpretcast wrote:
> jasonliu wrote:
> > hubert.reinterpretcast wrote:
> > > This is unsafe. There is no statement I could find indicating that such an auxiliary entry is required, and `xlc` will generate such symbol table entries with no auxiliary entry.
> > > 
> > > ```
> > > [16]    m   0x00000094     .data     1  unamex                    _$STATIC
> > > [17]    a4  0x00000004       0    0     SD       RW    0    0
> > > [18]    m   0x00000094     .data     0  static                    x
> > > ```
> > We have 
> > 
> > ```
> > if (NumberOfAuxEntries == 0)
> >     return;
> > ```
> > 
> > before this switch statement. If there is no aux entry, we would exit already before getting here. 
> Ah, yes. As a further consideration, I think we should do something about having more than one auxiliary entry here.
Could C_STAT symbol have more than 1 aux entry? I added an assertion for that.


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

https://reviews.llvm.org/D65240