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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:41-44
+## Above section data are generated using the compiler command:
+##   xlc -o test.o -c test.c
+## We also modified the binary content of traceback table in the object file to add vector Information
+## for function "foo",including ControlledStorageInfo, AllocaRegister, and ExtensionTable.
----------------
(Strictly, you could use "data are" and be grammatically correct, but it sounds weird to a native speaker)


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:354
+    OS << "\n";
+    // The size of displacement(address) is 4 bytes in 32-bit object files, and
+    // 8 bytes in 64-bit object files.
----------------



================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:416
+
+      // Print four bytes which has non-zero value.
+      if (Print4BytesOrLess())
----------------
jhenderson wrote:
> I'm not sure this comment makes sense (some of the bytes can be zero, just not the first one), but regardless, I don't think it's really needed.
Ping this comment?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:419
+        return;
+      if (Print4BytesOrLess())
+        return;
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Is this supposed to be duplicated?
> yes, the code will make the output as 
> 
> 
> ```
> # CHECK-NEXT:      5b: 00               # Padding
> # CHECK-NEXT:        ...
> # CHECK-NEXT:      68: 01 00 00 00
> # CHECK-NEXT:      72: 00 00 00 00
> # CHECK-NEXT:        ...
> # CHECK-NEXT:      90: 01 00 00 00
> ```
> 
> without the duplication code, above  will be changed  to
> 
> ```
> # CHECK-NEXT:      5b: 00               # Padding
> # CHECK-NEXT:        ...
> # CHECK-NEXT:      68: 01 00 00 00
> # CHECK-NEXT:        ...
> # CHECK-NEXT:      90: 01 00 00 00
> ```
The logic here is fairly intertwined, so I might be misunderstanding something, but if the aim is to ensure that there is always a set of zeros before the "...", I'm not convinced that this code achieves that. As far as I can tell, the first four bytes are printed and definitely have at least one non-zero byte involved. The second call though could print non-zero bytes too, so you'd end up with, e.g.:
```
5b: 00               # Padding
  ...
68: 01 00 00 00
72: 00 00 00 01
  ...
90: 01 00 00 00
```

If the next 12 bytes are then all 0, the ... will still be printed. I think?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:419
+        return;
+      if (Print4BytesOrLess())
+        return;
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Is this supposed to be duplicated?
> > yes, the code will make the output as 
> > 
> > 
> > ```
> > # CHECK-NEXT:      5b: 00               # Padding
> > # CHECK-NEXT:        ...
> > # CHECK-NEXT:      68: 01 00 00 00
> > # CHECK-NEXT:      72: 00 00 00 00
> > # CHECK-NEXT:        ...
> > # CHECK-NEXT:      90: 01 00 00 00
> > ```
> > 
> > without the duplication code, above  will be changed  to
> > 
> > ```
> > # CHECK-NEXT:      5b: 00               # Padding
> > # CHECK-NEXT:        ...
> > # CHECK-NEXT:      68: 01 00 00 00
> > # CHECK-NEXT:        ...
> > # CHECK-NEXT:      90: 01 00 00 00
> > ```
> The logic here is fairly intertwined, so I might be misunderstanding something, but if the aim is to ensure that there is always a set of zeros before the "...", I'm not convinced that this code achieves that. As far as I can tell, the first four bytes are printed and definitely have at least one non-zero byte involved. The second call though could print non-zero bytes too, so you'd end up with, e.g.:
> ```
> 5b: 00               # Padding
>   ...
> 68: 01 00 00 00
> 72: 00 00 00 01
>   ...
> 90: 01 00 00 00
> ```
> 
> If the next 12 bytes are then all 0, the ... will still be printed. I think?
I've gone back over the logic again and I'm finding it really hard to figure out. I'd like to summarise what my understanding of the goals of this code actually are, to make sure I understand things correctly:

1) Print a representation of the padding bytes from the end of the parsed data to the end of the full data.
2) Data is printed out in blocks of 4 bytes, aligned to the word boundary.
3) The first line may have fewer than 4 bytes, if the parsed data didn't end on a word boundary.
4) Consecutive blocks of 12 zeros or more are skipped and replaced with "...".
5) Any such skipped block must be preceded by a block of 4 bytes that are all zero.

Is this correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049