[PATCH] D61532: implement of the parsing symbol table for xcoffobjfile and output as yaml format

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:71
+      support::ubig32_t Offset;
+    } NameInStrTbl;
+  };
----------------
daltenty wrote:
> Building with this section with -DLLVM_ENABLE_WERROR=ON results in "error: anonymous types declared in an anonymous union are an extension"
I have fixed the problem


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:83
+    } CFileLanguageIdAndTypeId;
+  };
+
----------------
daltenty wrote:
> Same problem with anonymous types with this union
I build success as 
bash-4.2$ g++ -std=c++11 -I/scratch/zhijian/llvm/src/llvm/include -I/scratch/zhijian/llvm/build/include -c llvm/include/llvm/Object/XCOFFObjectFile.h


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:83
+    } CFileLanguageIdAndTypeId;
+  };
+
----------------
DiggerLin wrote:
> daltenty wrote:
> > Same problem with anonymous types with this union
> I build success as 
> bash-4.2$ g++ -std=c++11 -I/scratch/zhijian/llvm/src/llvm/include -I/scratch/zhijian/llvm/build/include -c llvm/include/llvm/Object/XCOFFObjectFile.h
I have fixed the problem .


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:115
+  if (SymEntPtr->StorageClass & 0x80)
+    return make_error<StringError>(
+        "XCOFF object file with debug information not yet supported",
----------------
hubert.reinterpretcast wrote:
> sfertile wrote:
> > hubert.reinterpretcast wrote:
> > > I'm not sure what asserting on a asserts-enabled build but generating a hard-error on a release build buys us. If we are okay with a hard error, then we can just go with it for all builds. If we are not okay with a hard error, then we don't want the debug build to trip either (and a TODO comment with some recovery path is the right answer).
> > > 
> > > Note that `llvm/test/tools/llvm-readobj/Inputs/xcoff-basic.o` has such symbols.
> > > 
> > >Note that llvm/test/tools/llvm-readobj/Inputs/xcoff-basic.o has such symbols.
> > 
> > We need to  commit to implementing the debug name handling either in the same patch that enables dumping symbols from readobj or before that. It would be useless if we have to tip-toe around object files with debug info. Until then I think the assert alone is fine.
> Printing unrelated strings or reporting spurious errors in release is not fine in my book. The hard-error alone in fine (no need for an assert).
1. I deleted the assert 
2. print symbol in as "Unimplemented Debug Name" after I discussed with Sean


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61532