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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:842
+bool doesXCOFFTracebackTableBegin(ArrayRef<uint8_t> Bytes) {
+  assert(Bytes.size() == 4 && "Traceback table started with 4 bytes zero.");
+  return support::endian::read32be(Bytes.data()) == 0;
----------------
jasonliu wrote:
> How does callers know that they have to pass an ArrayRef that has exactly 4 bytes to this function?
> What if they have an empty array? What if they pass in a 8 byte array?
> I don't think it's callers' responsibility to ensure that. This function should still return a correct answer for the above situation.
This is exactly the point I made earlier, but seems to have been ignored... :(


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:846
+
+static std::string parseParaType(uint32_t Value, unsigned int ParaNum) {
+  std::string ParaType;
----------------
Any particular reason you're using `unsigned int` here instead of just `unsigned` like you do below? Should it actually be a `size_t` in both cases?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:885
+  DataExtractor DE(ArrayRef<uint8_t>(Ptr, S), false, 4);
+  uint64_t offset_ptr = 0;
+
----------------
Please use LLVM style for variable names.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:85
+      XCOFFTracebackTable::create(V1, sizeof(V1));
+  ASSERT_TRUE(!!TTOrErr1) << "Parse error";
+  XCOFFTracebackTable TT1 = TTOrErr1.get();
----------------
Here and in the equivalent cases elsewhere, use `ASSERT_THAT_EXPECTED(TTOrErr1, Succeeded());`


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:86
+  ASSERT_TRUE(!!TTOrErr1) << "Parse error";
+  XCOFFTracebackTable TT1 = TTOrErr1.get();
+  EXPECT_EQ(TT1.getVersion(), 1);
----------------
`XCOFFTracebackTable TT1 = *TTOrErr1;` is more traditional usage.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:95
+
+  EXPECT_TRUE(TT1.getParaType());
+  EXPECT_STREQ(TT1.getParaType().getValue().data(), "i, i, f, f, d");
----------------
`ASSERT_TRUE` or the next check will crash if it ever fails.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:96
+  EXPECT_TRUE(TT1.getParaType());
+  EXPECT_STREQ(TT1.getParaType().getValue().data(), "i, i, f, f, d");
+
----------------
Does `EXPECT_EQ(TT1.getParaType().getValue(), "i, i, f, f, d");` not work?


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:101-103
+  ASSERT_TRUE(!!TTOrErr2) << "Parse error";
+  XCOFFTracebackTable TT2 = TTOrErr2.get();
+  EXPECT_STREQ(TT2.getParaType().getValue().data(), "f, f, d, i, i");
----------------
For this and the ones below, same comments as above, but you also need an `ASSERT_TRUE(TT2.getParaType())` to avoid a crash in case the `Optional` is empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585