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

[khemant@codeaurora.org via llvm-commits]

khemant added inline comments.

================
Comment at: test/tools/llvm-objdump/X86/disassemble-code-data-mix.test:15
@@ +14,3 @@
+        .globl bar
+        .type bar, at object
+bar:
----------------
compnerd wrote:
> Please uniformly choose a style (space or no space between the comma and type specifier).
Good catch, will fix this.

================
Comment at: test/tools/llvm-objdump/X86/disassemble-code-data-mix.test:18
@@ +17,3 @@
+        .string "test string"
+        .size bar, .-bar
+// CHECK: b:        74 65 73 74 20 73 74 72         test str
----------------
compnerd wrote:
> Are the sizes really needed?
In case of X86 test not really. In case of ARM test (I will upload it with some changes you recommend) it is needed so objdump can figure out that the data is "inside" a function or just a standalone data symbol in a text section.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1105
@@ +1104,3 @@
+  // data.
+  // This is used to pretty print the symbols while disassembling.
+  typedef std::vector<std::tuple<uint64_t, StringRef, SymbolRef::Type>>
----------------
compnerd wrote:
> Join this and the previous line?
Yes.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1108
@@ -1100,2 +1107,3 @@
+      SectionSymbolsTy;
   std::map<SectionRef, SectionSymbolsTy> AllSymbols;
   for (const SymbolRef &Symbol : Obj->symbols()) {
----------------
compnerd wrote:
> I wonder how much more expensive it is to keep a `std::vector<SymbolRef>` instead and query the Address/Name/Type from the symbol.
Good question for which I do not have any answer. The patch only made the pair into a tuple. 
I see however that types, address and name all return Expected, that would mean every time we query we have to check for all of this and then decide what should be done in case of error(s).

Perhaps someone else may have an answer for this.


Repository:
  rL LLVM

https://reviews.llvm.org/D23621