[PATCH] D82549: [AIX][XCOFF] parsing xcoff object file auxiliary header

[Digger via Phabricator via llvm-commits]

DiggerLin marked 20 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:90
+  support::ubig16_t ModuleType;
+  char CpuFlag;
+  char CpuType;
----------------
jasonliu wrote:
> Why do we use `char` and not `uint8_t`?
thanks


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:139
+const XCOFFAuxiliaryHeader64 *XCOFFObjectFile::AuxiliaryHeader64() const {
+  assert(is64Bit() && "64-bit interface called on a 64-bit object file.");
+  return static_cast<const XCOFFAuxiliaryHeader64 *>(AuxiliaryHeader);
----------------
jasonliu wrote:
> 64-bit interface called on a 32-bit object file.
thanks


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:105
+    const XCOFFAuxiliaryHeader64 *AuxHeader64Prt = Obj.AuxiliaryHeader64();
+    printAuxiliaryHeaders(AuxHeader64Prt);
+  } else {
----------------
jasonliu wrote:
> I don't think you need to define an extra variable here.
thanks


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:491
+  W.print##H(S, T);                                                            \
+  if ((X = X - sizeof(T)) == 0)                                                \
+  return
----------------
hubert.reinterpretcast wrote:
> This strikes me as extremely hazardous. What if we get a length value that is reflective of a partial field?
thanks


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:494
+
+void XCOFFDumper::printAuxiliaryHeaders(const XCOFFAuxiliaryHeader32 *AuxHeader) {
+  if (AuxHeader == nullptr) {
----------------
jasonliu wrote:
> Please consider combine 32 bit and 64 bit version of this function using template, as most of the fields have the same name. 
we print out the information based on the  binary sequence of auxiliary header.
the same field is on different offset between the  32bit and 64 bits. it is difficult to implement in template.

for example. 
o_tsize :  Offset at  4  in 32bits , but Offset at 56 at 64bits. 


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:501
+  DictScope DS(W, "AuxiliaryHeader");
+  PrintAuxMember(Hex, "Magic", AuxHeader->AuxMagic, AuxSize);
+  PrintAuxMember(Hex, "Version", AuxHeader->Version, AuxSize);
----------------
hubert.reinterpretcast wrote:
> jasonliu wrote:
> > Why do you need to pass in `AuxSize` to the macro function when all inputs are the same?
> `AuxSize` is modified by each macro(!) invocation...
for AuxSize is modified , I just make it looks like a function. 


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:178
+  // --auxiliary-headers
+  cl::opt<bool>
+      XCOFFAuxiliaryHeaders("auxiliary-headers",
----------------
jasonliu wrote:
> I'm assuming we need to add it somewhere in the llvm docs about this new option. 
thanks



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82549