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

[Digger Lin via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:179-180
   // Two of the bits that haven't got used in the mask.
-  if (Flag & 0x06)
+  if (Flag & 0x06 || Flag == 0x0)
     Res += "Unknown ";
 
----------------
jhenderson wrote:
> Is this change tightly linked to the rest of the patch, or could it be done first with an appropriate separate test case?
The function "getExtendedTBTableFlagString" is used the patch, but the Flag == 0x0 we do not deal with before, so add the here to deal with by the way(I think Jason asked to add here if I remember correct). If you strong suggest that I should remove it.  If will remove it.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1463-1464
       VectorParmsNum = VecExt->getNumberOfVectorParms();
+      // Skip two bytes of padding after vector info.
+      DE.skip(Cur, 2);
     }
----------------
jhenderson wrote:
> Do we have testing that these two bytes are now skipped (and which would have failed before because they weren't)?
yes, We tested the modification in "XCOFFTracebackTableAPIHasVectorInfo" of the XCOFFObjectFileTest.cpp, we change 

EXPECT_EQ(Size, 45u);
---> 
EXPECT_EQ(Size, 47u);


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1488
 
+    if (ExtensionTable.value() & ExtendedTBTableFlag::TB_EH_INFO) {
+      // eh_info displacement must be 4-byte aligned.
----------------
jhenderson wrote:
> Noting that there needs to be test cases both with and without this flag.
we has in  llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll 
; OBJ-DIS-NEXT:     13a: 08             # ExtensionTable = TB_EH_INFO
; OBJ-DIS-NEXT:     13b: 00             # Alignment padding for except info displacement
; OBJ-DIS-NEXT:     13c: 00 00 00 08    # Except info displacement

and in disassemble-traceback-table.test
# CHECK-NEXT:       54: c0 00 00 00       # VectorParmsInfoString = vf
# CHECK-NEXT:       58: 00 00             # Padding
# CHECK-NEXT:       5a: 20                # ExtensionTable = TB_SSP_CANARY
# CHECK-NEXT:       5b: 00                # Paddings


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:182
+    // Print all remaining zero as ...
+    if (Size - LastNoZero >= 8)
+      errs() << "\t\t...\n";
----------------
jhenderson wrote:
> Why 8?
if the remainning zero are >=8 , we only print as 
errs() << "\t\t...\n";
just keep it same AIX tool objdump.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:385
+    printRawData(Bytes.slice(Index, 4), Address + Index, OS, STI);
+    OS << "\t# Padding\n";
+    Index += 4;
----------------
jhenderson wrote:
> I'm not sure you really need this comment, given you've already labelled the previous bit with "Padding".
we just want to comment of every line the binary, without "Padding", user maybe not sure the line is not decode this moment or just "padding"


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:12-14
+#include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Object/XCOFFObjectFile.h"
+#include "llvm/Support/FormattedStream.h"
----------------
jhenderson wrote:
> Can you use forward declarations instead of additional includes here?
using forward delcarations here  and include the file in XCOFFDump.cpp ? what is the benefit ?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:162
 } // end namespace llvm
-
 #endif
----------------
jhenderson wrote:
> Why have you deleted this blank line?
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049