[PATCH] D89049: [AIX][XCOFF] print out the traceback info

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1437
       for (uint32_t I = 0; I < NumOfCtlAnchors && Cur; ++I)
-        Disp.push_back(DE.getU32(Cur));
+        Disp.push_back(Is64BitObj ? DE.getU64(Cur) : DE.getU32(Cur));
       if (Cur)
----------------
stephenpeckham wrote:
> The controlled storage offsets are always 4 bytes, even for 64-bit binaries
thanks ,fixed it.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:271
+      OS << "\t  ControlledStorageInfoDisp[" << I << "] = " << Disp[I];
+      // Print another 4 bytes for 64 bit.
+      if (Is64Bit) {
----------------
stephenpeckham wrote:
> The controlled storage offsets are always 4 bytes, even for 64-bit binaries.
thanks, fixed it.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:283
+
+    assert(FunctionNameLen > 0 &&
+           "The length of the function name must be greater than zero.");
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Should this assertion be an actual error? In other words, if the function name length stored was actually 0 (in a malformed object), would this code get here?
> > I think I can keep  the assert here, If we want to check the "FunctionNameLen==0   and  isFuncNamePresent is true" malform. we can implement it in function XCOFFTracebackTable::XCOFFTracebackTable() as a new patch.(Current patch is big patch now).
> > 
> > https://github.com/llvm/llvm-project/blob/main/llvm/lib/Object/XCOFFObjectFile.cpp#L1443 
> > 
> > 
> >   if (Cur && isFuncNamePresent()) {
> >     uint16_t FunctionNameLen = DE.getU16(Cur);
> >     if (Cur)
> >       FunctionName = DE.getBytes(Cur, FunctionNameLen);
> >   }
> > 
> We shouldn't land known broken code, and an assertion that can be hit by user code is broken. Whether that means adding the check in a prerequisite patch or this one depends on the solution you choose to go with.
thanks


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1736
+          (Symbols[SI - 1].XCOFFSymInfo.StorageMappingClass.value() ==
+           XCOFF::XMC_PR);
+
----------------
stephenpeckham wrote:
> Programs have glink code which also has traceback tables, so you should check for XMC_GL as well as XMC_PR.
since the patch is a big patch and last for about 3 years,  if I added the "XMC_GL" now, I also need to add a test case for "XMC_GL", it will let the patch bigger. Can I have a separate patch to deal with "XMC_GL"?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1793
+            dumpTracebackTable(Bytes.slice(Index),
+                               SectionAddr + Index + VMAAdjustment, FOS, End,
+                               *STI, &Obj);
----------------
stephenpeckham wrote:
> Programs are usually linked with a non-zero .text origin, so I think you need SectionAddr + End instead of End.  
in the line 1629 ,End already includes the SectionAddr

 uint64_t End = std::min<uint64_t>(**SectionAddr + SectSize,** StopAddress);


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:3231-3232
 
   if (DisassembleAll || PrintSource || PrintLines ||
       !DisassembleSymbols.empty())
     Disassemble = true;
----------------
jhenderson wrote:
> I'd be okay with moving the "if traceback table, enable --disassemble" logic to here, like the other cases, and dropping the "if XCOFF" check. It will simplify the code slightly, as you can then drop the `!TracebackTable` check from the below if clause. It also means that disassembly is printed for both XCOFF and non-XCOFF objects in the same invocation of llvm-objdump, which is good for consistency.
> 
> (If you do this, it might be worth adding a trivial test case showing that disassembly is enabled for non-XCOFF objects with --traceback-table).
Sorry that I can not understand the comment, I do not change code above your comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049