[llvm-objdump] Fix unstable disassembly output for sections with same address

[jhenderson]

Summary

When two sections shared the same address, the disassembly code was using pointer values to break ties when sorting. Since those values aren't guaranteed to have a specific order, this meant the disassembly code would sometimes change which section to pick when finding symbols targeted by calls in fully linked objects.

This change fixes the non-determinism, so that the same section is always picked. This might have a negative impact in that now a section without any symbol might be picked over a section with symbols, but this will be addressed in a later commit.

Fixes https://bugs.llvm.org/show_bug.cgi?id=45411.

[grimar]

llvm/test/tools/llvm-objdump/X86/disassemble-same-section-addr.test

26

Would be more natural to have VAs encoded in hex: 0x0 here and 0x5 below.

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

1231

stable_sort->llvm::stable_sort I think (while you are here) as usually (probably always)
algorithm invocations like this include std:: or llvm:: prefixes.

[grimar]

LGTM with 2 nits mentioned.

[jhenderson]

Address nits. I'll wait until either I get another thumbs up or tomorrow morning, whichever comes earlier, to give others a chance to chime in.

[MaskRay]

Thanks.

Please mention that the non-determinism comes from SectionRef::operator<(const SectionRef &) const comparing the pointers.

llvm/test/tools/llvm-objdump/X86/disassemble-same-section-addr.test

2

More of a question: shall we place generic disassembly behavior to test/tools/llvm-objdump/ELF/ ?

I know that we mostly place disassembly tests under X86/, so this patch is consistent with the current practice and does not need a change.

[MaskRay]

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

1231–1234

llvm::stable_sort(SectionAddresses, llvm::less_first())

[jhenderson]

llvm/test/tools/llvm-objdump/X86/disassemble-same-section-addr.test

2

I noticed that the llvm-objdump testing is a bit of a mess, because there are two unrelated axes of testing - targets and file formats. Consequently, we have both ELF and X86 folders independently of each other. It's also not always clear to me which axis is important to the test. In this case, I think the answer is actually "neither" (the principle is target and format agnostic, I believe) and it's only in the X86 folder for convenience, due to the need for an X86 disassembler.

As to what we should do elsewhere, maybe we need a "Generic" folder alongside ELF etc, and in that folder, put the X86 etc folders as required? I don't know really.

llvm/test/tools/llvm-objdump/X86/disassemble-same-section-addr.test

+## This test shows that llvm-objdump can handle sections with the same address
+## when symbols in those sections are referenced. In the past, the section
+## picked was non-deterministic, resulting in different symbols being found for
+## the section. The test uses YAML for the input, as we need a fully linked ELF
+## to reproduce the original failure.
+
+# RUN: yaml2obj %s -o %t1 -D SECTION=.second
+# RUN: llvm-objdump -d %t1 | FileCheck %s
+# RUN: yaml2obj %s -o %t2 -D SECTION=.first
+## FIXME: this case should print "<target>" too.
+# RUN: llvm-objdump -d %t2 | FileCheck %s --check-prefix=FAIL
+
+# CHECK: callq 0x5 <target>
+# FAIL:  callq 0x5{{$}}
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .caller
+    Type:    SHT_PROGBITS
+    Flags:   [SHF_ALLOC, SHF_EXECINSTR]
+    Address: 0
+    Content: e800000000 # Call instruction to next address.
+  - Name:    .first
+    Type:    SHT_PROGBITS
+    Flags:   [SHF_ALLOC, SHF_EXECINSTR]
+    Address: 5
+  - Name:    .second
+    Type:    SHT_PROGBITS
+    Flags:   [SHF_ALLOC, SHF_EXECINSTR]
+    Address: 5
+Symbols:
+  - Name:    target
+    Section: [[SECTION]]
+    Value:   5

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

static void disassembleObject(const Target *TheTarget, const ObjectFile *Obj,
   addPltEntries(Obj, AllSymbols, Saver);
 
   // Create a mapping from virtual address to section. An empty section can
   // cause more than one section at the same address. Use a stable sort to
   // stabilize the output.
   std::vector<std::pair<uint64_t, SectionRef>> SectionAddresses;
   for (SectionRef Sec : Obj->sections())
     SectionAddresses.emplace_back(Sec.getAddress(), Sec);
-  stable_sort(SectionAddresses);
+  stable_sort(SectionAddresses, [](const std::pair<uint64_t, SectionRef> &LHS,
+                                   const std::pair<uint64_t, SectionRef> &RHS) {
+    return LHS.first < RHS.first;
+  });
 
   // Linked executables (.exe and .dll files) typically don't include a real
   // symbol table but they might contain an export table.
   if (const auto *COFFObj = dyn_cast<COFFObjectFile>(Obj)) {
     for (const auto &ExportEntry : COFFObj->export_directories()) {
       StringRef Name;
       if (std::error_code EC = ExportEntry.getSymbolName(Name))
         reportError(errorCodeToError(EC), Obj->getFileName());