[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/BinaryFormat/XCOFF.h:348
+
+  // Using for the fixed or float point parameter type identification.
+  static constexpr uint32_t FixedParaTypeBit = 0x8000'0000;
----------------
Using -> Used

(?)


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:408
+                                              uint64_t Size);
+  XCOFFTracebackTable(const uint8_t *Ptr, uint64_t Size, Error &Err);
+
----------------
Make this private so that all users must use the `create` function.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:883
+XCOFFTracebackTable::XCOFFTracebackTable(const uint8_t *Ptr,
+                                         const uint64_t Size, Error &Err)
+    : TBPtr(Ptr), Size(Size) {
----------------
Delete the `const` for `Size`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:886
+  ErrorAsOutParameter EAO(&Err);
+  DataExtractor DE(ArrayRef<uint8_t>(Ptr, Size), /* IsLittleEndian */ false,
+                   /* AddressSize */ 0);
----------------
Preferred style is `/* IsLittleEndian=*/false` (same with `AddressSize` below). This plays well with clang-format too.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:888
+                   /* AddressSize */ 0);
+  uint64_t OffsetPtr = 0;
+
----------------
This isn't a pointer, so remove the `Ptr`!


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:890
+
+  DE.getU64(&OffsetPtr, &Err);
+
----------------
Not sure if you've come across it yet, but you could simplify all these calls using `DataExtractor::Cursor` instead. This stores the error internally, whilst also allowing you to continue parsing to the end, with no harmful effects (because nothing is read if `Cursor` is in an error state), similar to `Err` being passed in everywhere. It also tracks the offset. Usage is:

```
DataExtractor::Cursor C(/*Offset=*/0);
DE.getU64(C); // By the way, what is this value for?
...
// No need for ifs here.
CodeLen = DE.getU32(C);
HandlerMask = DE.getU32(C);
...

Err = C.takeError();
```


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:96
+  EXPECT_TRUE(TT1.getParaType());
+  EXPECT_STREQ(TT1.getParaType().getValue().data(), "i, i, f, f, d");
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Does `EXPECT_EQ(TT1.getParaType().getValue(), "i, i, f, f, d");` not work?
> 
> according to  https://github.com/google/googletest/blob/master/googletest/docs/primer.md
> 
> EXPECT_EQ(val1, val2);	compare two values. val1 == val2
> 
> EXPECT_STREQ(str1,str2);  compare two C strings	the two C strings have the same content
> 
Emphasis on "C strings". You're not using a C string on the left, so can use standard C-string to `std::string` implicit conversion on the right instead of calling `c_str` or `data`, so `EXPECT_EQ` is better (simplfies code).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585