[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:421
+  ///    bytes that are not recognized, \a Size will be updated to be the size
+  ///    up to the end of the last succsessfully parsed field of the table.
+  static Expected<XCOFFTracebackTable> create(const uint8_t *Ptr,
----------------
Always check my text for typos :)

succsessfully -> successfully


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:872-874
+                                                          uint64_t *Size) {
+  Error Err = Error::success();
+  XCOFFTracebackTable TBT(Ptr, *Size, Err);
----------------
You pass in the Size as a pointer here, yet it can never be null, and is immediately dereferenced. I think it would make sense to use a reference.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:877
+    return std::move(Err);
+  return std::move(TBT);
+}
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Do you actually need the `std::move` here?
> yes. I think so. there is iImplicitly-declared move constructor for XCOFFTracebackTable , and there is there are several Optional type of data member, for example, SmallString has move constructor. 
In general terms, you should never need to `std::move` a local variable in a return statement. This is because it inihibits return-value optimizations. In return statements, the compiler automatically elides the copy or move, if it can. Explicitly specifying `std::move` prevents the compiler doing this. (See https://stackoverflow.com/questions/17473753/c11-return-value-optimization-or-move and various other resources on the internet). The presence of move constructors (whether implicitly declared or not) is irrelevant here.

That being said, at least with some compilers, when performing an implicit conversion on return (as happens here), you may need a std::move (see https://stackoverflow.com/questions/17481018/when-is-explicit-move-needed-for-a-return-statement). This is something that should disappear at some point, but I suspect our C++14 support might make it impossible.

Basically, try removing it, and building with different compilers, and see what happens.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:18
+  EXPECT_TRUE(doesXCOFFTracebackTableBegin({0, 0, 0, 0}));
+
+  EXPECT_FALSE(doesXCOFFTracebackTableBegin({0, 0, 0, 1}));
----------------
This test is simple enough that I'd just delete this blank line. I'd also show the behaviour where there are too few bytes, and too many bytes.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:101
+  Expected<XCOFFTracebackTable> TTOrErr2 =
+      XCOFFTracebackTable::create(V1, &Size);
+  ASSERT_THAT_EXPECTED(TTOrErr2, Succeeded());
----------------
It's possible that `Size` might have been changed by the previous call to `create`, so it's not safe to reuse it here. It might make more sense to split each of these cases up into a separate test, with a fixture providing the common stuff for reuse.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:142
+  EXPECT_EQ(Disp[1], 0x06060000u);
+}
----------------
You need testing for what happens when the table is truncated/malformed in such a way that the parsing fails. Currently, you do not test the failures. I think you'll need a test case for each point where the reading might fail if the size were too small.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585