This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jhenderson on Apr 7 2020, 4:44 AM.

Details

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.

Diff Detail

Event Timeline

jhenderson created this revision.Apr 7 2020, 4:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2020, 4:44 AM
Herald added a subscriber: rupprecht. · View Herald Transcript
grimar added inline comments.Apr 7 2020, 5:02 AM
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 accepted this revision.Apr 7 2020, 5:03 AM

LGTM with 2 nits mentioned.

This revision is now accepted and ready to land.Apr 7 2020, 5:03 AM
jhenderson updated this revision to Diff 255657.Apr 7 2020, 6:19 AM
jhenderson marked an inline comment as done.

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 accepted this revision.Apr 7 2020, 11:12 AM

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 added inline comments.Apr 7 2020, 11:13 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1231–1234

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

jhenderson marked 2 inline comments as done.Apr 8 2020, 2:16 AM
jhenderson added inline comments.
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.

jhenderson marked an inline comment as done.Apr 8 2020, 2:59 AM
This revision was automatically updated to reflect the committed changes.