[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/include/llvm/Object/XCOFFObjectFile.h:396-398
+/// This class provides methods to extract traceback table data from a buffer
+/// and that the various accessors may continue to reference the buffer provided
+/// via the constructor.
----------------



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:898
+    // indicates the presence of vector parameters.
+    if (Cur && ParmNum > 0) {
+      uint32_t ParamsTypeValue = DE.getU32(Cur);
----------------
Can you drop the `Cur &&` from here? It looks like there's no way it could be invalid at this point.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:146
+
+  EXPECT_EQ(Disp[0], 0x05050000u);
+  EXPECT_EQ(Disp[1], 0x06060000u);
----------------
You need an `ASSERT_EQ(Disp.size(), 2)` before this check.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:150-159
+TEST(XCOFFObjectFileTest, XCOFFTracebackTableAPIHasVectorInfo) {
+  uint64_t Size = 40;
+  Expected<XCOFFTracebackTable> TTOrErr =
+      XCOFFTracebackTable::create(V_T, Size);
+  ASSERT_THAT_EXPECTED(TTOrErr, Succeeded());
+  XCOFFTracebackTable TT = *TTOrErr;
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > The inputs to this test are identical to the previous case above. Could you simply merge the two together?
> the two test case has different test case name, One to  test  XCOFFTracebackTableAPI for HasVectorInfo , Other to test XCOFFTracebackTableAPI for ControlledStorageInfoDisp.  If merge, it is difficult to have a good test case name here. 
I guess when vector info parsing is expanded, this second test will get expanded, but the first won't be?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585