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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:56
+bool doesXCOFFTracebackTableBegin(ArrayRef<uint8_t> Bytes) {
+  assert(Bytes.size() == 4 && "Traceback table started with 4 bytes zero.");
+  return support::endian::read32be(Bytes.data()) == 0;
----------------
DiggerLin wrote:
> jhenderson wrote:
> > I think `return false` would be more appropriate in this case. That way you could, for example, pass in an empty array. Clearly in that situation, the bytes can't represent the start of a traceback table.
> > 
> > I'd also be inclined to change this name to simply `isXCOFFTracebackTable`. I don't think you need the `Begin` bit, but I don't feel strongly about this, especially since I don't have much context in this area.
> > 
> > I also think this function better belongs down near your other added code in this file to do with the traceback table.
> when the caller call  doesXCOFFTracebackTableBegin(), it should guarantee , there is four bytes . we call assert here , just make sure  there is four bytes.
> 
> if there is  four bytes zero in the text section object file, it means the tracebacktable information will follow the fours bytes zero.
> 
> I keep the function name as  doesXCOFFTracebackTableBegin, if you strong oppose it, I can change.
> 
>   
But why should it be the caller's responsibility to make sure there are 4 bytes available? That requires the caller to know the format of the traceback table, which it shouldn't have to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585