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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 16 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:299
+  static constexpr uint32_t GlobaLinkage_Mask = 0x8000;
+  static constexpr uint32_t Is_Eprol_Mask = 0x4000;
+  static constexpr uint32_t Has_CodeLen_Mask = 0x2000;
----------------
jhenderson wrote:
> Are these names taken from the spec? If so, that's fine. If not, they need renaming in LLVM style.
yes, the name is  taken from the "aix RT  follow on linkage" , I will change it to  llvm style. 


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-tracebacktable.test:60-61
+CHECK-NEXT:       bc: 00 04 6d 61                  # Length of function name = 4
+CHECK-NEXT:                                        # Function Name: ma
+CHECK-NEXT:       c0: 69 6e 00 00                  # in
+CHECK-NEXT:                                        # Padding
----------------
jhenderson wrote:
> The comment text here looks all messed up.
"in" is part of function name "main". what format do you suggestion? 
 # Function Name: in
I think it also messed up. or
#Function Name(cont): in  ?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:147
+
+  XCOFFTracebackTable Tb(const_cast<unsigned char *>(Bytes.data() + Index));
+  uint32_t Value;
----------------
jhenderson wrote:
> The use of `const_cast` here and below is very dubious. This is a dumping tool. Why do you need to modify the bytes at all?
> 
> More likely, you need to fix the signature of your class methods to take the right type.
the support::ubig32_t::ref constructor only accept (void*), 

public:
  struct ref {
    explicit ref(void *Ptr) : Ptr(Ptr) {}

    operator value_type() const {
      return endian::read<value_type, endian, alignment>(Ptr);
    }

    void operator=(value_type NewValue) {
      endian::write<value_type, endian, alignment>(Ptr, NewValue);
    }

  private:
    void *Ptr;
  };



================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:336
+
+  assert(End >= Address + Index && "Traceback table parse error.");
+
----------------
jhenderson wrote:
> If a malformed traceback table can cause this to be hit, it should be a regular error/warning, not an assertion. Assertions are for internal coding errors, not for errors caused by invalid input.
thanks


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:32-38
+void formatTracebackTableOutput(ArrayRef<uint8_t> Bytes, uint64_t Address,
+                                raw_ostream &OS);
+
+void dumpTracebackTable(ArrayRef<uint8_t> Bytes, uint64_t Address,
+                        raw_ostream &OS, uint64_t End);
+
+bool doesXCOFFTracebackTableBegin(ArrayRef<uint8_t> Bytes);
----------------
jhenderson wrote:
> It seems to me like this new code would be better off in a library somewhere, e.g. somewhere in the `Object` library. That would allow more focused testing of it (via unit tests), and in the future other tools could be taught to parse the data structure too.
we has discussed whether we need to decode traceback table in other tools in our team  and whether to implement some functionality in the object file. we do not find  other tools now, even if there is later, we can do a NFC patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585