[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
llvm-commits at lists.llvm.org
Thu Aug 13 00:15:51 PDT 2020
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
More information about the llvm-commits
mailing list