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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:335-338
+.. option:: --traceback-table
+
+  Decode traceback table for disassembly output.
+
----------------
As previously suggested, I'd move all llvm-objdump related changes to a separate patch. This patch should focus on the data structure and API, then another patch can make use of it within llvm-objdump etc.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:288
+struct Traceback_Table {
+
+  // Byte 1
----------------
jhenderson wrote:
> Remove this blank line.
There's a blank line here again. Please remove it.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:406
+public:
+  XCOFFTracebackTable(const uint8_t *Ptr, const uint64_t S);
+
----------------
No real need for `const` in the second argument.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:56
+bool doesXCOFFTracebackTableBegin(ArrayRef<uint8_t> Bytes) {
+  assert(Bytes.size() == 4 && "Traceback table started with 4 bytes zero.");
+  return support::endian::read32be(Bytes.data()) == 0;
----------------
I think `return false` would be more appropriate in this case. That way you could, for example, pass in an empty array. Clearly in that situation, the bytes can't represent the start of a traceback table.

I'd also be inclined to change this name to simply `isXCOFFTracebackTable`. I don't think you need the `Begin` bit, but I don't feel strongly about this, especially since I don't have much context in this area.

I also think this function better belongs down near your other added code in this file to do with the traceback table.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:845
 
+static std::string parseParaType(uint32_t Value, unsigned int ParaNum) {
+  std::string ParaType;
----------------
Whilst this function isn't part of the public API, I think it is complicated enough to deserve its own dedicated unit tests. You could write some code in the unit test to setup a basic table, using a provided value for the bits to do with the `ParaType`, and then test that the expected sequence is produced.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:870
+
+XCOFFTracebackTable::XCOFFTracebackTable(const uint8_t *Ptr, const uint64_t S)
+    : TBPtr(Ptr), Size(S) {
----------------
It may be better to use a [[ https://llvm.org/docs/ProgrammersManual.html#fallible-constructors | fallible constructor pattern ]] here, to do the error handling below in a way that will allow users to pass in malformed inputs, and let the library handle the problems. Also, consider using the `DataExtractor` and `Cursor` classes, here to do the parsing, rather than using `support::endian` directly along with needed to manually track the position.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:28
+
+  EXPECT_EQ(TT.getVersion(), 0);
+
----------------
You need at least a second variant of this unit test with different input values. This allows you to check that the `getVersion()` function doesn't just return 0 always, the `isGlobalLinkage()` function doesn't just always return false, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585