[PATCH] D24636: llvm-objdump Allow disassembly of ARM and thumb code mix in ELF object file.

[Renato Golin via llvm-commits]

rengolin requested changes to this revision.
rengolin added reviewers: rengolin, zatrazz, peter.smith.
rengolin added a comment.
This revision now requires changes to proceed.

This is an interesting functionality, but I'm not well versed in objdump to know if the code is what's expected.

The first thing I would say is that it really needs a clang-format pass as well as a lot more tests, since the change in functionality is not small.

>From a quick look, it seems that you went for the "whatever works" approach, which makes the whole thing extremely complicated, and harder to maintain. I'm not sure all other architectures will be happy with the added context, complexity and conditional blocks for the sake of one arch (ARM).

I honestly think this needs a better design and it would be better to get that discussion to the llvm-dev list first, as an RFC.

cheers,
--renato


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1092
@@ +1091,3 @@
+    size_t Offset = 1;
+    const uint8_t *Data = AttributeSection.data();
+    // Find the aeabi Vendor sub-section
----------------
You're converting a pointer to an array ref to another pointer, this is really confusing. Please, pick one style and stick with it.

even better, isn't there a way to decode the attributes into a nice structure?

@compnerd, you spent more time in the attributes code than I did, any ideas?

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1102
@@ +1101,3 @@
+
+      if (std::string("aeabi") !=
+          reinterpret_cast<const char *>(Data + SubOffset)) {
----------------
StringRef.startsWith("aeabi")?

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1119
@@ +1118,3 @@
+        SubOffset += 4;
+        while (SubOffset < Length) {
+          unsigned TagLen;
----------------
there has to be a better way to deal with attributes...

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1362
@@ -1219,3 +1361,3 @@
         if (Name.startswith("$x"))
-          TextMappingSymsAddr.push_back(Address - SectionAddr);
+          AArch64MappingSymsAddr.push_back(Address - SectionAddr);
         if (Name.startswith("$a"))
----------------
AArch64 code doesn't mix with AArch32, so this is conceptually wrong.

If this is just to reduce code duplication in this file, I understand, but there could be an error. It'd be clearer, though, if they were in separate blocks, one for ARM and one for AArch64.


Repository:
  rL LLVM

https://reviews.llvm.org/D24636