[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 a comment.

I'm out of time to look at the tests today. I'll come back to them, hopefully later in the week.



================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:320
+  static constexpr uint32_t IsInterruptHandlerMask = 0x0000'0080;
+  static constexpr uint32_t HasFunctionNamePresentMask = 0x0000'0040;
+  static constexpr uint32_t IsAllocaUsedMask = 0x0000'0020;
----------------
Does this name reflect a name from a specification? I'd expect it to be `IsFunctionNamePresentMask` or possibly `HasFunctionNameMask` otherwise ("is" expects an adjective, i.e. "present", whereas has would expect a noun, i.e. "name").


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:350
+  static constexpr uint32_t ParmTypeIsFloatingBit = 0x8000'0000;
+  static constexpr uint32_t ParmTypeFloatingIsDoubleBit = 0x4000'0000;
+};
----------------
I don't have any XCOFF knowledge, but should this be `ParmTypeIsFloatingDoubleBit`?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:420-421
+  ///    If XCOFF Traceback Table is not parsed successfully or there are extra
+  ///    bytes that are not recognized, \a Size will be updated to reflect the
+  ///    last successfully parsed field XCOFF Traceback Table.
+  static Expected<XCOFFTracebackTable> create(const uint8_t *Ptr,
----------------
I think the following would be slightly clearer:

"\a Size will be updated to be the size up to the end of the last succsessfully parsed field of the table."


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:437
+  bool isInterruptHandler() const;
+  bool hasFuncNamePresent() const;
+  bool isAllocaUsed() const;
----------------
Equivalent comment to mask name.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:877
+    return std::move(Err);
+  return std::move(TBT);
+}
----------------
Do you actually need the `std::move` here?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:933
+
+uint8_t XCOFFTracebackTable::getVersion() const {
+  return GETBITWITHMASKSHIFT(0, VersionMask, VersionShift);
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > hubert.reinterpretcast wrote:
> > > I am not sure that functions that perform a read via a pointer to non-owned memory should be named "get" functions as opposed to "read" functions.
> > I think maybe better to keep name "get" functions . 
> > if the function has argument as 
> > getVersion(void* prt, uint64_t size), it maybe better to change to readVersion(void* prt, uint64_t size).
> > 
> > but there is no argument here, keep getVersion myabe better.
> Another possibility for the name is "extract". @DiggerLin, please see if @jhenderson has an opinion on this naming aspect.
I don't have any particular opinions on this. I think `extractVersion` or `readVersion` might be slightly better than `getVersion` etc because it shows that it's not just a simple getter of a private member or similar.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:886
+  ErrorAsOutParameter EAO(&Err);
+  DataExtractor DE(ArrayRef<uint8_t>(Ptr, Size), /* IsLittleEndian */ false,
+                   /* AddressSize */ 0);
----------------
jhenderson wrote:
> Preferred style is `/* IsLittleEndian=*/false` (same with `AddressSize` below). This plays well with clang-format too.
I made a typo in my original comment. The style for this should be `/*IsLittleEndian=*/false` and `/*AddressSize=*/0` without any spaces.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:890
+
+  DE.getU64(&OffsetPtr, &Err);
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > 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();
> > ```
> I considered to use Cursor before, but most of our data member are Optianl I think it maybe  better to keep current implement, using Cursor, it still get a value(for example zero), and the CodeLen, ParaType, FunctionName etc are Optional type. it will do  Optional &operator=(T &&y)  for all the fields(CodeLen HandlerMask etc,)  even if there is error before and if will always call parseParaType().   I do not think it is efficient.
I don't think the `return 0` behaviour of `Cursor` in the event of an error stops you using it. You can still do `if (C)` to check whether you should do a thing, just like you do with `if (!Err)`. For example:

```
  DataExtractor::Cursor C(Offset);
  DE.getU64(C);
  if (C)
    ParmNum = getNumberOfFixedParms() + getNumberOfFPParms();
  if (C && ParmNum > 0 && !hasVectorInfo())
    ParmsType = parseParmsType(DE.getU32(C), ParmNum);
  if (C && hasTraceBackTableOffset())
    TraceBackTableOffset = DE.getU32(C);

  ...
  Offset = C.tell();
  Err = C.takeError();
```

It doesn't save much, but I find the get* functions easier to read that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585