[PATCH] D75631: [test] Fix reliability of disassemble-functions.test

Tue Mar 24 15:42:52 PDT 2020

thopre marked an inline comment as done.
thopre added a comment.

In D75631#1913554 <https://reviews.llvm.org/D75631#1913554>, @echristo wrote:

> >> The change looks good to me regardless, but i'd like to understand the issue causing the variability in behaviour before giving formal approval, as non-determinism like this is bad, and might indicate something like an unitialised variable somewhere or some other bug.
> > 
> > I will also be nervous to approve this change, without understanding how the test fails.
>
> Agreed.
>
> -eric

Sorry for taking so long to come back to this. TL;DR: while I cannot reproduce the test failure on my own computer there's inherent behavior variation due to qsort not guaranteeing any specific order for elements that are equivalents [1]. I think algorithm to determine target name could be improved.

[1] https://en.cppreference.com/w/cpp/algorithm/qsort

Long summary:

Test llvm/test/tools/llvm-objdump/X86/Inputs/simple-executable-x86_64.yaml translates to an executable file. For non relocatable files, llvm-objdump finds the last section in the qsort'ed list of sections whose address is below or equal to the target of the call. On my machine that ends up being .strtab. Since there's no associated symbols with that section, it looks among absolute symbols for the last one with an address less or equal to the target of the call. It finds only 1 absolute symbol: the STT_FILE symbol /tmp/a.c. While I cannot reproduce the test failure on my machine I presume on some system qsort will sort the sections with the same target address (0) differently leading to very different code executing.

Given the nature of the symbol to be found (target of a call) I think it would make sense to restrict the section search to SHT_PROGBITS + SHF_ALLOC|SHF_EXECINSTR. Since there's isText() available in SectionRef I've used that instead so it only checks for SHF_EXECINSTR. That makes the test fail as it should since the call's target in main is foo.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75631/new/

https://reviews.llvm.org/D75631