llvm-objdump: ELF: Handle code and data mix in all scenarios

[khemant]

Summary

The patch handls behavior of the disassembler when code and data are mixed in a text section.
When code an data are mixed in a section, the GNU disassembler acts in an interesting manner.
If told to disassemble text only (-d), it will interpret mixed data as data
$cat 1.c
int myInt = 1;
char myChar = 'b';
float myFloat = 1.2;
double myComm;

int main () {

return myInt;

}

$cat 1.t
SECTIONS {
.text : { *(.text)
*(.data.my* )
*(.bss*) }
}

$clang -c 1.c -fdata-sections
$ld 1.o -T 1.t
$objdump -d a.out

a.out: file format elf64-x86-64

Disassembly of section .text:

0000000000000040 <main>:

40:   55                      push   %rbp
41:   48 89 e5                mov    %rsp,%rbp
44:   c7 45 fc 00 00 00 00    movl   $0x0,-0x4(%rbp)
4b:   8b 04 25 54 00 00 00    mov    0x54,%eax
52:   5d                      pop    %rbp
53:   c3                      retq

0000000000000054 <myInt>:

54:   01 00 00 00                                         ....

0000000000000058 <myChar>:

58:   62 0f 1f 00                                         b...

000000000000005c <myFloat>:

5c:   9a 99 99 3f                                         ...?

ARM variant has one more twist to it. When told to disassemble text only, it will interpret data if data is a standalone symbol, if it is part of a function, it marks it as a word/short etc.
$arm-none-eabi-gcc -c 1.c -fdata-sections
$arm-none-eabi-ld 1.o -t 1.t

$arm-none-eabi-objdump -d a.out

a.out: file format elf32-littlearm

Disassembly of section .text:

00008000 <main>:

8000:       e52db004        push    {fp}            ; (str fp, [sp, #-4]!)
8004:       e28db000        add     fp, sp, #0
8008:       e59f3010        ldr     r3, [pc, #16]   ; 8020 <main+0x20>
800c:       e5933000        ldr     r3, [r3]
8010:       e1a00003        mov     r0, r3
8014:       e24bd000        sub     sp, fp, #0
8018:       e49db004        pop     {fp}            ; (ldr fp, [sp], #4)
801c:       e12fff1e        bx      lr
8020:       00008024        .word   0x00008024  <=====marked as data

00008024 <myInt>:

8024:       00000001                                ....    <====interpreted as data

00008028 <myChar>:

8028:       00000062                                b...

0000802c <myFloat>:

802c:       3f99999a                                ...?

If told to disassemble all - all objdump variants really disassemble the data as instructions:
$arm-none-eabi-objdump -D a.out

a.out: file format elf32-littlearm

Disassembly of section .text:

00008000 <main>:

8000:       e52db004        push    {fp}            ; (str fp, [sp, #-4]!)
8004:       e28db000        add     fp, sp, #0
8008:       e59f3010        ldr     r3, [pc, #16]   ; 8020 <main+0x20>
800c:       e5933000        ldr     r3, [r3]
8010:       e1a00003        mov     r0, r3
8014:       e24bd000        sub     sp, fp, #0
8018:       e49db004        pop     {fp}            ; (ldr fp, [sp], #4)
801c:       e12fff1e        bx      lr
8020:       00008024        andeq   r8, r0, r4, lsr #32

00008024 <myInt>:

8024:       00000001        andeq   r0, r0, r1

00008028 <myChar>:

8028:       00000062        andeq   r0, r0, r2, rrx

0000802c <myFloat>:

802c:       3f99999a        svccc   0x0099999a

Disassembly of section .bss:

00010030 <__bss_start>:

...

[compnerd]

test/tools/llvm-objdump/X86/disassemble-code-data-mix.test

15 ↗

(On Diff #68398)

Please uniformly choose a style (space or no space between the comma and type specifier).

18 ↗

(On Diff #68398)

Are the sizes really needed?

19 ↗

(On Diff #68398)

Whitespace is ignored, so it makes it easier to read if you align the address labels.

tools/llvm-objdump/llvm-objdump.cpp

1105 ↗

(On Diff #68398)

Join this and the previous line?

1108 ↗

(On Diff #68398)

I wonder how much more expensive it is to keep a std::vector<SymbolRef> instead and query the Address/Name/Type from the symbol.

[khemant]

test/tools/llvm-objdump/X86/disassemble-code-data-mix.test

15 ↗

(On Diff #68398)

Good catch, will fix this.

18 ↗

(On Diff #68398)

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.

tools/llvm-objdump/llvm-objdump.cpp

1105 ↗

(On Diff #68398)

Yes.

1108 ↗

(On Diff #68398)

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.

[khemant]

Incorporated suggestions from previous diff.
Added ARM test.

[compnerd]

tools/llvm-objdump/llvm-objdump.cpp

1107 ↗

(On Diff #68559)

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.

1194 ↗

(On Diff #68559)

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.)

1230 ↗

(On Diff #68559)

Really? clang-format did this?

1313 ↗

(On Diff #68559)

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.

1326 ↗

(On Diff #68559)

Similar via llvm::ulittle16_t.

1359 ↗

(On Diff #68559)

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.

1365 ↗

(On Diff #68559)

Id probably collapse this into a ternary operation.

1380 ↗

(On Diff #68559)

reinterpret_cast<char *> please.

[khemant]

tools/llvm-objdump/llvm-objdump.cpp

1107 ↗

(On Diff #68559)

Looks like this is a moot point now. Please check this commit: https://reviews.llvm.org/rL278921

1313 ↗

(On Diff #68559)

Good point. Will do this.

[khemant]

tools/llvm-objdump/llvm-objdump.cpp

1359 ↗

(On Diff #68559)

We cannot hoist this outside, because the byte stream is broken into 8 character byte sequences and then printer with printable characters. Then the carriage must return and then show the next offset and then start. Example:

0:        00 7e 00 00 01 40 00 00         .~...@..
 8:        89 42 00 00 cd 45 00 00         .B...E..
10:        89 42 00 00 89 42 00 00         .B...B..
18:        89 42 00 00 00 00 00 00         .B......

Hoisting it outside will only print first address, it will make finding a byte say 0x12 difficult.

[khemant]

Updated diff with latest tip, made few changes as recommended.

[compnerd]

LGTM with at least the cast on the use of format to avoid cross-endian issues.

tools/llvm-objdump/llvm-objdump.cpp

1315 ↗

(On Diff #69043)

Is there a real reason to limit this to ARM ELF? Shouldn't all ELF have this behavior?

1332 ↗

(On Diff #69043)

auto would be nicer since the cast spells out the type.

1342 ↗

(On Diff #69043)

Add an explicit cast to unsigned before passing this through format, otherwise you will be breaking cross-endianness.

1349 ↗

(On Diff #69043)

auto would be nicer since the cast spells out the large type.

[khemant]

tools/llvm-objdump/llvm-objdump.cpp

1315 ↗

(On Diff #69043)

ARM is the only one that has these mapping symbols that mix inside the function (from beginning of a@function type and within the size of that symbol). All other architectures there needs to be an explicit data symbol (local or global) in which case the disassembler uses the second kind - the general way of interpreting that bytes are data.

1332 ↗

(On Diff #69043)

Yep.

1342 ↗

(On Diff #69043)

Data is a unsigned int (uint16_t). I think that is enough for format.

1349 ↗

(On Diff #69043)

Got it.

llvm/trunk/test/tools/llvm-objdump/ARM/disassemble-code-data-mix.s

+@RUN: llvm-mc -triple arm-unknown-linux -filetype=obj %s | llvm-objdump -d - | FileCheck %s
+
+  .cpu arm7tdmi
+  .global myInt
+  .data
+  .align  2
+  .type myInt, %object
+  .size myInt, 4
+myInt:
+  .word 1
+  .text
+  .align  2
+  .global main
+  .type main, %function
+main:
+  str fp, [sp, #-4]!
+  add fp, sp, #0
+  ldr r3, .L3
+  ldr r3, [r3]
+  mov r0, r3
+  sub sp, fp, #0
+  ldr fp, [sp], #4
+  bx  lr
+.L4:
+  .align  2
+.L3:
+  .word myInt
+  .size main, .-main
+        .global myStr
+        .type myStr, %object
+myStr:
+        .string "test string"
+
+
+@CHECK:     .word   0x00000000
+@CHECK-DAG: 74 65 73 74 20 73 74 72         test str

llvm/trunk/test/tools/llvm-objdump/X86/disassemble-code-data-mix.s

+// RUN: llvm-mc %s -filetype=obj -triple=x86_64-pc-linux | llvm-objdump -d - | FileCheck %s
+        .text
+        .globl  foo
+        .type   foo, @function
+foo:
+        pushq   %rbp
+        movq    %rsp, %rbp
+        movl    $0, %eax
+        popq    %rbp
+        ret
+
+        .globl bar
+        .type bar, @object
+bar:
+        .string "test string"
+
+// CHECK:        b:        74 65 73 74 20 73 74 72         test str
+// CHECK-NEXT:  13:        69 6e 67 00                     ing.
+

llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp

if (FileBuffer != SourceCache.end()) {
         // Vector begins at 0, line numbers are non-zero
         OS << Delimiter << LineBuffer->second[LineInfo.Line - 1].ltrim()
            << "\n";
     }
   }
   OldLineInfo = LineInfo;
 }
 
+static bool isArmElf(const ObjectFile *Obj) {
+  return (Obj->isELF() &&
+          (Obj->getArch() == Triple::aarch64 ||
+           Obj->getArch() == Triple::aarch64_be ||
+           Obj->getArch() == Triple::arm || Obj->getArch() == Triple::armeb ||
+           Obj->getArch() == Triple::thumb ||
+           Obj->getArch() == Triple::thumbeb));
+}
+
 class PrettyPrinter {
 public:
   virtual ~PrettyPrinter(){}
   virtual void printInst(MCInstPrinter &IP, const MCInst *MI,
                          ArrayRef<uint8_t> Bytes, uint64_t Address,
                          raw_ostream &OS, StringRef Annot,
                          MCSubtargetInfo const &STI, SourcePrinter *SP) {
     if (SP && (PrintSource || PrintLines))
for (const SymbolRef &Symbol : Obj->symbols()) {
     if (Name->empty())
       continue;
 
     Expected<section_iterator> SectionOrErr = Symbol.getSection();
     error(errorToErrorCode(SectionOrErr.takeError()));
     section_iterator SecI = *SectionOrErr;
     if (SecI == Obj->section_end())
       continue;
-    
-    // For AMDGPU we need to track symbol types
     uint8_t SymbolType = ELF::STT_NOTYPE;
-    if (Obj->isELF() && Obj->getArch() == Triple::amdgcn) {
+    if (Obj->isELF())
       SymbolType = getElfSymbolType(Obj, Symbol);
-    }
 
     AllSymbols[*SecI].emplace_back(Address, *Name, SymbolType);
 
   }
 
   // Create a mapping from virtual address to section.
   std::vector<std::pair<uint64_t, SectionRef>> SectionAddresses;
   for (SectionRef Sec : Obj->sections())
for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     uint64_t SectSize = Section.getSize();
     if (!SectSize)
       continue;
 
     // Get the list of all the symbols in this section.
     SectionSymbolsTy &Symbols = AllSymbols[Section];
     std::vector<uint64_t> DataMappingSymsAddr;
     std::vector<uint64_t> TextMappingSymsAddr;
-    if (Obj->isELF() && Obj->getArch() == Triple::aarch64) {
+    if (isArmElf(Obj)) {
       for (const auto &Symb : Symbols) {
         uint64_t Address = std::get<0>(Symb);
         StringRef Name = std::get<1>(Symb);
         if (Name.startswith("$d"))
           DataMappingSymsAddr.push_back(Address - SectionAddr);
         if (Name.startswith("$x"))
           TextMappingSymsAddr.push_back(Address - SectionAddr);
+        if (Name.startswith("$a"))
+          TextMappingSymsAddr.push_back(Address - SectionAddr);
+        if (Name.startswith("$t"))
+          TextMappingSymsAddr.push_back(Address - SectionAddr);
       }
     }
 
     std::sort(DataMappingSymsAddr.begin(), DataMappingSymsAddr.end());
     std::sort(TextMappingSymsAddr.begin(), TextMappingSymsAddr.end());
 
     // Make a list of all the relocations for this section.
     std::vector<RelocationRef> Rels;
for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     error(Section.getName(name));
     outs() << "Disassembly of section ";
     if (!SegmentName.empty())
       outs() << SegmentName << ",";
     outs() << name << ':';
 
     // If the section has no symbol at the start, just insert a dummy one.
     if (Symbols.empty() || std::get<0>(Symbols[0]) != 0) {
-      Symbols.insert(Symbols.begin(), std::make_tuple(SectionAddr, name, ELF::STT_NOTYPE));
+      Symbols.insert(Symbols.begin(),
+                     std::make_tuple(SectionAddr, name, Section.isText()
+                                                            ? ELF::STT_FUNC
+                                                            : ELF::STT_OBJECT));
     }
 
     SmallString<40> Comments;
     raw_svector_ostream CommentStream(Comments);
 
     StringRef BytesStr;
     error(Section.getContents(BytesStr));
     ArrayRef<uint8_t> Bytes(reinterpret_cast<const uint8_t *>(BytesStr.data()),
#else
       raw_ostream &DebugOut = nulls();
 #endif
 
       for (Index = Start; Index < End; Index += Size) {
         MCInst Inst;
 
         // AArch64 ELF binaries can interleave data and text in the
         // same section. We rely on the markers introduced to
-        // understand what we need to dump.
-        if (Obj->isELF() && Obj->getArch() == Triple::aarch64) {
+        // understand what we need to dump. If the data marker is within a
+        // function, it is denoted as a word/short etc
+        if (isArmElf(Obj) && std::get<2>(Symbols[si]) != ELF::STT_OBJECT &&
+            !DisassembleAll) {
           uint64_t Stride = 0;
 
           auto DAI = std::lower_bound(DataMappingSymsAddr.begin(),
                                       DataMappingSymsAddr.end(), Index);
           if (DAI != DataMappingSymsAddr.end() && *DAI == Index) {
             // Switch to data.
             while (Index < End) {
               outs() << format("%8" PRIx64 ":", SectionAddr + Index);
               outs() << "\t";
               if (Index + 4 <= End) {
                 Stride = 4;
                 dumpBytes(Bytes.slice(Index, 4), outs());
-                outs() << "\t.word";
+                outs() << "\t.word\t";
+                uint32_t Data = 0;
+                if (Obj->isLittleEndian()) {
+                  const auto Word =
+                      reinterpret_cast<const support::ulittle32_t *>(
+                          Bytes.data() + Index);
+                  Data = *Word;
+                } else {
+                  const auto Word = reinterpret_cast<const support::ubig32_t *>(
+                      Bytes.data() + Index);
+                  Data = *Word;
+                }
+                outs() << "0x" << format("%08" PRIx32, Data);
               } else if (Index + 2 <= End) {
                 Stride = 2;
                 dumpBytes(Bytes.slice(Index, 2), outs());
-                outs() << "\t.short";
+                outs() << "\t\t.short\t";
+                uint16_t Data = 0;
+                if (Obj->isLittleEndian()) {
+                  const auto Short =
+                      reinterpret_cast<const support::ulittle16_t *>(
+                          Bytes.data() + Index);
+                  Data = *Short;
+                } else {
+                  const auto Short =
+                      reinterpret_cast<const support::ubig16_t *>(Bytes.data() +
+                                                                  Index);
+                  Data = *Short;
+                }
+                outs() << "0x" << format("%04" PRIx16, Data);
               } else {
                 Stride = 1;
                 dumpBytes(Bytes.slice(Index, 1), outs());
-                outs() << "\t.byte";
+                outs() << "\t\t.byte\t";
+                outs() << "0x" << format("%02" PRIx8, Bytes.slice(Index, 1)[0]);
               }
               Index += Stride;
               outs() << "\n";
               auto TAI = std::lower_bound(TextMappingSymsAddr.begin(),
                                           TextMappingSymsAddr.end(), Index);
               if (TAI != TextMappingSymsAddr.end() && *TAI == Index)
                 break;
             }
           }
         }
 
+        // If there is a data symbol inside an ELF text section and we are only
+        // disassembling text (applicable all architectures),
+        // we are in a situation where we must print the data and not
+        // disassemble it.
+        if (Obj->isELF() && std::get<2>(Symbols[si]) == ELF::STT_OBJECT &&
+            !DisassembleAll && Section.isText()) {
+          // print out data up to 8 bytes at a time in hex and ascii
+          uint8_t AsciiData[9] = {'\0'};
+          uint8_t Byte;
+          int NumBytes = 0;
+
+          for (Index = Start; Index < End; Index += 1) {
+            if (NumBytes == 0) {
+              outs() << format("%8" PRIx64 ":", SectionAddr + Index);
+              outs() << "\t";
+            }
+            Byte = Bytes.slice(Index)[0];
+            outs() << format(" %02x", Byte);
+            AsciiData[NumBytes] = isprint(Byte) ? Byte : '.';
+
+            uint8_t IndentOffset = 0;
+            NumBytes++;
+            if (Index == End - 1 || NumBytes > 8) {
+              // Indent the space for less than 8 bytes data.
+              // 2 spaces for byte and one for space between bytes
+              IndentOffset = 3 * (8 - NumBytes);
+              for (int Excess = 8 - NumBytes; Excess < 8; Excess++)
+                AsciiData[Excess] = '\0';
+              NumBytes = 8;
+            }
+            if (NumBytes == 8) {
+              AsciiData[8] = '\0';
+              outs() << std::string(IndentOffset, ' ') << "         ";
+              outs() << reinterpret_cast<char *>(AsciiData);
+              outs() << '\n';
+              NumBytes = 0;
+            }
+          }
+        }
         if (Index >= End)
           break;
 
+        // Disassemble a real instruction or a data when disassemble all is
+        // provided
         bool Disassembled = DisAsm->getInstruction(Inst, Size, Bytes.slice(Index),
                                                    SectionAddr + Index, DebugOut,
                                                    CommentStream);
         if (Size == 0)
           Size = 1;
 
         PIP.printInst(*IP, Disassembled ? &Inst : nullptr,
                       Bytes.slice(Index, Size), SectionAddr + Index, outs(), "",