On Sun, Sep 3, 2017 at 12:19 PM, Dmitry Vyukov <dvyu...@google.com> wrote: > On Sun, Sep 3, 2017 at 12:01 PM, Jakub Jelinek <ja...@redhat.com> wrote: >> On Sun, Sep 03, 2017 at 10:50:16AM +0200, Dmitry Vyukov wrote: >>> What we instrument in LLVM is _comparisons_ rather than control >>> structures. So that would be: >>> _4 = x_8(D) == 98; >>> For example, result of the comparison can be stored into a bool struct >>> field, and then used in branching long time after. We still want to >>> intercept this comparison. >> >> Then we need to instrument not just GIMPLE_COND, which is the stmt >> where the comparison decides to which of the two basic block successors to >> jump, but also GIMPLE_ASSIGN with tcc_comparison class >> gimple_assign_rhs_code (the comparison above), and maybe also >> GIMPLE_ASSIGN with COND_EXPR comparison code (that is say >> _4 = x_1 == y_2 ? 23 : _3; >> ). >> >>> > Perhaps for -fsanitize-coverage= it might be a good idea to force >>> > LOGICAL_OP_NON_SHORT_CIRCUIT/BRANCH_COST or whatever affects GIMPLE >>> > decisions mentioned above so that the IL is closer to what the user wrote. >>> >>> If we recurse down to comparison operations and instrument them, this >>> will not be so important, right? >> >> Well, if you just handle tcc_comparison GIMPLE_ASSIGN and not GIMPLE_COND, >> then you don't handle many comparisons from the source code. And if you >> handle both, some of the GIMPLE_CONDs might be just artificial comparisons. >> By pretending small branch cost for the tracing case you get fewer >> artificial comparisons. > > > Are these artificial comparisons on BOOLEAN_TYPE? I think BOOLEAN_TYPE > needs to be ignored entirely, there is just like 2 combinations of > possible values. > If not, then what it is? Is it a dup of previous comparisons? > > I am not saying these modes should not be enabled. You know much > better. I just wanted to point that that integer comparisons is what > we should be handling. > > Your example: > > _1 = x_8(D) == 21; > _2 = x_8(D) == 64; > _3 = _1 | _2; > if (_3 != 0) > > raises another point. Most likely we don't want to see speculative > comparisons. At least not yet (we will see them once we get through > the first comparison). So that may be another reason to enable these > modes (make compiler stick closer to original code).
Wait, it is not speculative in this case as branch is on _1 | _2. But still, it just makes it harder for fuzzer to get through as it needs to guess both values at the same time rather then doing incremental progress.
