[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/lib/Object/XCOFFObjectFile.cpp:882
+
+XCOFFTracebackTable::XCOFFTracebackTable(const uint8_t *Ptr, uint64_t Size,
+                                         Error &Err)
----------------
DiggerLin wrote:
> jasonliu wrote:
> > 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. 
> yes. we do not want to  planning to treat extra bytes at the end of Traceback Table as an error(the Caller need to deal it , the caller can decide to look it a warning or error. the api also provide API). 
> 
> if return the parsed traceback table size to the caller through the construct, it means the caller can not construct the XCOFFTracebackTable from a constant . and If the caller do not want the size to be modified, the caller has to has local copy variable first  and use the 
> 
> And I think we already to document here 
>  uint64_t TBTSize; // The size of the XCOFFTracebackTable.
The concern I have with using a member function is that this functionality is very easy to get ignored. Caller would not know such API is provided for this purpose until they actually read though all the member functions and what they do. Requesting pass a non-const pointer to a size would make it very obvious that the callee would modify the size to provide feedback.
I don't think a local copy of variable to be a big problem though. In fact, saving it as a data member means you would copy it every time in the callee side, no matter caller wants it or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585