[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:1204-1206
+      uint64_t Pos = Cur.tell();
+      if (Pos % 4 != 0)
+        DE.skip(Cur, 4 - (Pos % 4));
----------------
jhenderson wrote:
> I think you should be able to use the alignTo function to do this alignment calculation. However, it looks like the `Cursor` and `DataExtractor` classes are missing any form of `seek` function to set the position directly, without needing to calculate from the current location. I think it would be useful to add this function to one or the other. You could then do something like one of the following:
> ```
> Cur.seek(alignTo(Cur.tell(), 4));
> DE.seek(Cur, alignTo(Cur.tell(), 4));
> ```
> which I'm sure you'll agree is much clearer and less likely to have any issues.
good idea, thanks. I will create a separate patch for it. https://reviews.llvm.org/D109603 . after the patch approve, I will change the code based on the new patch.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll:9
 
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN:     -mattr=+altivec -vec-extabi -xcoff-traceback-table=true -filetype=obj -o %t.o  < %s
----------------
jhenderson wrote:
> Is there any particular reason you can't fold this in with the other cases?
these one generate the object file.  other  generate asm. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll:12
+; RUN: llvm-objdump -D --traceback-table  --symbol-description  %t.o | \
+; RUN:   FileCheck --check-prefix=OBJ_DIS %s
+
----------------
jhenderson wrote:
> Use `-` not `_` in your prefix names, as per the prevailing style in this file.
thanks


================
Comment at: llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll:106
+; OBJ_DIS:           9c: 00 00 00 00    # Traceback table begin
+; OBJ_DIS_NEXT:      a0: 00             # Version = 0
+; OBJ_DIS_NEXT:      a1: 09             # Language = CPlusPlus
----------------
jhenderson wrote:
> This won't work. `NEXT` suffixes must be prefixed by a `-` not `_`.
thanks


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test:1
+# REQUIRES: powerpc-registered-target
+
----------------
jhenderson wrote:
> 1) This test needs additional comments explaining what the test is trying to test. How is the input invalid?
> 2) You should invest time in adding functionality for writing traceback tables to yaml2obj, so you can use it to generate the invalid output, rather than checking in an opaque and impossible to maintain binary (most people won't have access to the xlc compiler, for example, should there be a problem with the test).
agree with you.  but not this moment, the yaml2obj not support the  auxiliary entry for the symbol table, parsing traceback table depend on it. 

in the XCOFFEmitter.cpp : line 332 
"// TODO: Auxiliary entry is not supported yet."


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:7
+## xcoff-traceback-table.o compiled with IBM XL C/C++ for AIX, V16.1.0
+## compiler command: xlc -o xcoff-traceback-table.o -c foo.c
+## foo.c:
----------------
jhenderson wrote:
> DiggerLin wrote:
> > Xiangling_L wrote:
> > > Should we use xlclang here?
> > yes, of course, we can use xlclang here, but  I think using xlc is ok too.
> See my above comments - add traceback table generation to yaml2obj rather than adding canned binaries that people are unable to update.
agree with you. same comment as above.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:174
+
+    // The value of Size has been changed in function XCOFFTracebackTable::create()
+    Size = End - Address;
----------------
jhenderson wrote:
> Remember to put a full stop at the end of sentences.
thanks


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:201
+
+  auto printOutBytes = [&](uint64_t n) {
+    printRawData(Bytes.slice(Index, n), Address + Index, OS, STI);
----------------
jhenderson wrote:
> Shorter and more grammatically correct name. You "print" something usually, not "print out" something.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049