[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/lib/Object/XCOFFObjectFile.cpp:792
 
-bool XCOFFSymbolRef::isFunction() const {
-  if (OwningObjectPtr->is64Bit())
-    report_fatal_error("64-bit support is unimplemented yet.");
+Expected<XCOFFCsectAuxRef> XCOFFSymbolRef::getXCOFFCsectAuxRef() const {
+  if (!isCsectSymbol())
----------------
DiggerLin wrote:
> not all the symbol has Csect entry.
> what about to return 
> Optional<XCOFFCsectAuxRef>XCOFFSymbolRef::getXCOFFCsectAuxRef() 
I returned Expected<XCOFFCsectAuxRef> partly because of the original comment on this function:
// TODO: The function needs to return an error if there is no csect auxiliary
// entry.

I believe that's a change from your previous commit. Is there any reason that you changed your mind?
In 32 bit mode, you could have a csect symbol, but without any auxiliary entry. That should return an error (which I haven't detected here, but I should). 
Also, in 64 bit mode, it's possible that you have a csect symbol that has auxiliary entries, but do not have a csect auxiliary entry, that should be an error situation right? So it also makes sense to return error in that case. 
I think the interface would be too complicated if we return an Expected wrap with an Optional, or the other way around.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:796
+        object_error::parse_failed,
+        "this symbol entry type could not have a csect auxiliary entry");
 
----------------
DiggerLin wrote:
> I think assert(isCsectSymbol()) myabe better.
> sometime maybe our developer call getXCOFFCsectAuxRef()  at a no CsectSymbol . it is not a object file parse failed.
Sure.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:49
                                 const SymbolRef &Sym) {
-  XCOFFSymbolRef SymRef(Sym.getRawDataRefImpl(), Obj);
+  const XCOFFSymbolRef SymRef = Obj->toSymbolRef(Sym.getRawDataRefImpl());
 
----------------
DiggerLin wrote:
> I can not see benefit to change 
> from  
> XCOFFSymbolRef SymRef(Sym.getRawDataRefImpl(), Obj); 
> to 
> XCOFFSymbolRef SymRef = Obj->toSymbolRef(Sym.getRawDataRefImpl());
I did it for consistency reason, i.e: always get XCOFFSymbolRef via toSymbolRef.


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

https://reviews.llvm.org/D85774