[PATCH] D81585: [AIX][XCOFF] Decode trace back table information for xcoff object file for llvm-objdump -d

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

According to the premerge bot, some of your tests are failing. Please fix.

Frankly, this patch is way too big to sensibly review. I think you'd be better served breaking it up into several smaller pieces that incrementally build towards what you want. It will also be easier to ensure that each piece is tested properly.

You could, for example, introduce a stub for the class and some parsing code that fills in, for example, the first byte in one patch, skipping the rest initiall,y and incrementally add support for each other byte in a series of patches. You could add dumping support for each byte in the same sort of manner. Finally, you could integrate it with llvm-objdump in the final change in the series.



================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:288
+struct Traceback_Table {
+
+  // Byte 1
----------------
Remove this blank line.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:299
+  static constexpr uint32_t GlobaLinkage_Mask = 0x8000;
+  static constexpr uint32_t Is_Eprol_Mask = 0x4000;
+  static constexpr uint32_t Has_CodeLen_Mask = 0x2000;
----------------
Are these names taken from the spec? If so, that's fine. If not, they need renaming in LLVM style.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:395
+class XCOFFTracebackTable {
+  void *Tb_ptr;
+
----------------
Again, unless this is a name conforming to some spec, it needs renaming in LLVM style (e.g. `TBPtr`).


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:400
+
+  // The 3th bytes fields
+  bool hasFunctionCodeLen();
----------------
These comments are not needed. They are implementation details of the format, which doesn't add anything to the user's knowledge. (Also 3th -> 3rd and missing full stops).


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-tracebacktable.test:60-61
+CHECK-NEXT:       bc: 00 04 6d 61                  # Length of function name = 4
+CHECK-NEXT:                                        # Function Name: ma
+CHECK-NEXT:       c0: 69 6e 00 00                  # in
+CHECK-NEXT:                                        # Padding
----------------
The comment text here looks all messed up.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:147
+
+  XCOFFTracebackTable Tb(const_cast<unsigned char *>(Bytes.data() + Index));
+  uint32_t Value;
----------------
The use of `const_cast` here and below is very dubious. This is a dumping tool. Why do you need to modify the bytes at all?

More likely, you need to fix the signature of your class methods to take the right type.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:232
+    OS << "# Parameter Type: ";
+    for (unsigned i = 0; i < Tb.getNumberOfFixedPara() + Tb.getNumberOfFPPara();
+         ++i) {
----------------
`i` -> `I`
Assuming `Tb.getNumberOfFixedPara() + Tb.getNumberOfFPPara()` is constant during this loop, move it into a new variable initialised at loop start, as per LLVM coding standards.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:336
+
+  assert(End >= Address + Index && "Traceback table parse error.");
+
----------------
If a malformed traceback table can cause this to be hit, it should be a regular error/warning, not an assertion. Assertions are for internal coding errors, not for errors caused by invalid input.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:32-38
+void formatTracebackTableOutput(ArrayRef<uint8_t> Bytes, uint64_t Address,
+                                raw_ostream &OS);
+
+void dumpTracebackTable(ArrayRef<uint8_t> Bytes, uint64_t Address,
+                        raw_ostream &OS, uint64_t End);
+
+bool doesXCOFFTracebackTableBegin(ArrayRef<uint8_t> Bytes);
----------------
It seems to me like this new code would be better off in a library somewhere, e.g. somewhere in the `Object` library. That would allow more focused testing of it (via unit tests), and in the future other tools could be taught to parse the data structure too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585