[PATCH] D81585: [AIX][XCOFF][Patch1] Provide decoding trace back table information API for xcoff object file for llvm-objdump -d

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:411
+  static Expected<XCOFFTracebackTable> create(const uint8_t *Ptr,
+                                              uint64_t &Size);
+  uint8_t getVersion() const;
----------------
I think a pointer to `Size` is better than reference to `Size` in this case. Reference is too subtle for this scenario and caller might not notice this is taken by reference.
And as mentioned in the other comment, a comment and a test case is needed for this function.

For the comment, I would suggest:
```
/// Parse a XCOFF Traceback Table from \a Ptr with \a Size bytes.
/// Returns a XCOFFTracebackTable upon successful parsing, otherwise an Error is returned.
///
/// \param[in] Ptr
///    A pointer points to the beginning of XCOFF Traceback Table (right after the initial 4 bytes of zeros).
/// \param[in, out] Size
///    The length of the XCOFF Traceback Table. 
///    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 actual length of the XCOFF Traceback Table that get parsed.
```

Also please include a test that the XCOFFTracebackTable have extra content upon a successful parsing, and check if the Size gets modified correctly to the content that we are able to parse. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:857
+      Value <<= 1;
+      continue;
+    } else {
----------------
both `continue` (here and in `else`) seems redundant for the current control flow. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585