[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
Wed Jul 8 01:06:48 PDT 2020
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
More information about the llvm-commits
mailing list