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

1127

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

1128

Join this and the previous line?

[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

1127

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.

1128

Yes.

[khemant]

Incorporated suggestions from previous diff.
Added ARM test.

[compnerd]

tools/llvm-objdump/llvm-objdump.cpp

1127

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.

1215

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

1251

Really? clang-format did this?

1340

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.

1353

Similar via llvm::ulittle16_t.

1390

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.

1396

Id probably collapse this into a ternary operation.

1411

reinterpret_cast<char *> please.

[khemant]

tools/llvm-objdump/llvm-objdump.cpp

1127

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

1340

Good point. Will do this.

[khemant]

tools/llvm-objdump/llvm-objdump.cpp

1390

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

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

1332

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

1342

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

1349

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

[khemant]

tools/llvm-objdump/llvm-objdump.cpp

1315

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

Yep.

1342

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

1349

Got it.

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

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

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 SectionRef &Section : ToolSectionFilter(*Obj)) {
     section_iterator Sec2 = Section.getRelocatedSection();
     if (Sec2 != Obj->section_end())
       SectionRelocMap[*Sec2].push_back(Section);
   }
 
   // Create a mapping from virtual address to symbol name.  This is used to
   // pretty print the symbols while disassembling.
   typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;
   std::map<SectionRef, SectionSymbolsTy> AllSymbols;
   for (const SymbolRef &Symbol : Obj->symbols()) {
     Expected<uint64_t> AddressOrErr = Symbol.getAddress();
     error(errorToErrorCode(AddressOrErr.takeError()));
     uint64_t Address = *AddressOrErr;
 
     Expected<StringRef> Name = Symbol.getName();
     error(errorToErrorCode(Name.takeError()));
     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;
     if (InlineRelocs) {
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 support::ulittle32_t *Word =
+                      reinterpret_cast<const support::ulittle32_t *>(
+                          Bytes.data() + Index);
+                  Data = *Word;
+                } else {
+                  const support::ubig32_t *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 support::ulittle16_t *Word =
+                      reinterpret_cast<const support::ulittle16_t *>(
+                          Bytes.data() + Index);
+                  Data = *Word;
+                } else {
+                  const support::ubig16_t *Word =
+                      reinterpret_cast<const support::ubig16_t *>(Bytes.data() +
+                                                                  Index);
+                  Data = *Word;
+                }
+                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(), "",