[PATCH] D23621: llvm-objdump: ELF: Handle code and data mix in all scenarios

[Saleem Abdulrasool via llvm-commits]

compnerd added inline comments.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1107
@@ -1100,2 +1106,3 @@
+      SectionSymbolsTy;
   std::map<SectionRef, SectionSymbolsTy> AllSymbols;
   for (const SymbolRef &Symbol : Obj->symbols()) {
----------------
IIRC the Expected wrappers are not as intrusive as the Error wrappers.  So, it may end up being reasonable still.  I would agree that this would go beyond what is needed here, and is possibly a good follow up cleanup.

However, would you mind using a typedef (or the C++11 equivalent with using) for the tuple type?  Even better would be to have a private enumeration so we can semi-symbolically expand the tuple.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1194
@@ -1179,2 +1193,3 @@
+          TextMappingSymsAddr.push_back(Address - SectionAddr);
       }
     }
----------------
And that just leaves jezelle (`$z`).  (No, Im not recommending that you add that, LLVM can't even generate that I believe, just excited for the additional coverage here.)

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1230
@@ -1213,1 +1229,3 @@
+                                     ? SymbolRef::ST_Function
+                                     : SymbolRef::ST_Data));
 
----------------
Really?  clang-format did this?

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1313
@@ +1312,3 @@
+                         Bytes.slice(Index, 4)[2] << 8 |
+                         Bytes.slice(Index, 4)[3];
+                outs() << "0x" << format("%08" PRIx32, Data);
----------------
I wonder if we can use `llvm::ulittle32_t` to translate the value.  It would require a specific cast going through format, but would be easier to understand.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1326
@@ +1325,3 @@
+                  Data =
+                      Bytes.slice(Index, 2)[0] << 8 | Bytes.slice(Index, 2)[1];
+                outs() << "0x" << format("%04" PRIx16, Data);
----------------
Similar via `llvm::ulittle16_t`.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1359
@@ +1358,3 @@
+              outs() << "\t";
+            }
+            Byte = Bytes.slice(Index)[0];
----------------
Cant this if be hoisted outside of the for loop?  `Index` should be `Start` at that point, and we can do this unconditionally as we know that `NumBytes` is zero prior to the for loop.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1365
@@ +1364,3 @@
+            else
+              AsciiData[NumBytes] = '.';
+
----------------
Id probably collapse this into a ternary operation.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1380
@@ +1379,3 @@
+              outs() << std::string(IndentOffset, ' ') << "         ";
+              outs() << (char *)AsciiData;
+              outs() << '\n';
----------------
`reinterpret_cast<char *>` please.


Repository:
  rL LLVM

https://reviews.llvm.org/D23621