[PATCH] D81585: [AIX][XCOFF] 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/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;
----------------
DiggerLin wrote:
> 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. 
If it's matching an existing spec, you don't need to follow LLVM style. Matching the spec is more important for helping people who know the spec to follow the code. See for example how ELF data structures are named (they match the ELF spec).


================
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
----------------
DiggerLin wrote:
> 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  ?
I'd expect to see the whole name on one line, not broken up over multiple. If you feel it's important to have the characters on the same line, you should either consider reorganising how the bytes are printed, so that each relevant field is on its own line, or put it all either on the line where the name starts or where it finishes. Personally, I think it would make more sense to display the output in a more structure manner, with each field displayed on a separate line. For an example of what I mean, take a look at what llvm-dwarfdump does when dumping the debug line table in verbose mode. There, the offset of each instruction is printed, along with the value of the opcode, and its interpretation.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:147
+
+  XCOFFTracebackTable Tb(const_cast<unsigned char *>(Bytes.data() + Index));
+  uint32_t Value;
----------------
DiggerLin wrote:
> 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;
>   };
> 
I'm not sure you should be using the `ref` type. That seems to me for creating references to the value, rather than just a copy of the value, which is probably all you want.

It seems to me like you're just not using the right API. You should probably be using the `DataExtractor` API to read your data, or at the very list the `read32` etc functions in `Endian.h`.


================
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);
----------------
DiggerLin wrote:
> 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. 
You seem to have ignored the first half of my reasoning - it allows more focused unit testing, rather than trying to write all the tests in lit tests, especially given you currently have no way of generating the input files at test time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585