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

Thu Jul 16 07:54:49 PDT 2020

jasonliu added inline comments.

================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:882
+
+XCOFFTracebackTable::XCOFFTracebackTable(const uint8_t *Ptr, uint64_t Size,
+                                         Error &Err)
----------------
DiggerLin wrote:
> jasonliu wrote:
> > We passed in a pointer and a size to this constructor function and create function, so caller knows how big the traceback table is. 
> > I think in general we need to have a way to tell caller some of the content is not parsed when that happens. 
> > i.e. everything in the traceback looks good, but it has some extra bytes in the end we do not recognized. 
> > So that caller maybe able to just dump the raw bytes out if they want, and print out a general message saying:" Some of the traceback table content is not recognized and here are the raw bytes."
> I added a data member "TBTSize" to store the valid traceback table size and API for it.
> When the caller want  to print out the table content is not recognized , it can not the api the get the valid size.
Sounds like we are not planning to treat extra bytes at the end of Traceback Table as an error, which kind of makes sense as we already parsed all the traceback table entries we know and it looks good. 
Regarding on how we return the parsed traceback table size to the caller, instead of using a member function getTBTSize, have we thought about modify the create/constructor to have `uint64_t *Size` instead of `uint64_t Size`, so that the size could be passed back via the parameter? And we would need extra documentations for that as well. An example would be what we used here for `DataExtractor`: https://llvm.org/doxygen/DataExtractor_8h_source.html#l00100 
Also we would want to have a test to test this behavior. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585