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

Mon Mar 9 13:30:50 PDT 2020

MaskRay added a comment.

In D75631#1912053 <https://reviews.llvm.org/D75631#1912053>, @jhenderson wrote:

> > By the way it makes me wonder if there isn't some CHECK-NOT or implicit check not relying on the old output which are never going to be triggered even if the output goes wrong
>
> That thought had occurred to me too. @MaskRay, did you do any check to make sure you haven't invalidated any CHECK-NOT/--implicit-check-not patterns?

I wrote a dumb script to update CHECK lines in ~350 tests changed by D75713 <https://reviews.llvm.org/D75713>. I manually fixed ~50 tests. I did not make extra efforts to verify any implicit-check-not= is still kept for the tests updated by my script.

implicit-check-not should be used carefully. Usually, only when a positive pattern also occurs, a negative pattern can be used. Otherwise a negative pattern can easily become stale after the output format changes.

As an example, `--implicit-check-not=somedata:` is a bad pattern.

>> I'm not sure why does it change since I've only seen dump from the CI and haven't been able to reproduce the failure locally. However the dump I get shows "callq </tmp/a.c>" which seems like an arbitrary choice (it's not the first symbol for that address, it's a STT_FILE instead of STT_FUNC)
> 
> I don't know the ins and outs of the LLVM disassembly logic, but it might be that disassembling references just pick an arbitrary symbol with a given address and doesn't pay any attention to the symbol type (in other words, an STT_FILE symbol with addess 0 is just as valid as a function symbol with that address).
> 
> To help investigate the CI issue, is it always the same build configuration, or are there different configurations being used that could have an impact? Did it start changing at some point, or has it always been an issue as far as you know?
> 
> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75631