[PATCH] D157740: [WIP] [InstCombine] Fold icmp into phi beyond the same BB.

[Andrew via Phabricator via llvm-commits]

browneee added inline comments.


================
Comment at: compiler-rt/test/dfsan/conditional_callbacks.c:101
   int tainted_cond = ((DataI * DataJ) != 1);
+  dfsan_set_label(LabelIJ, &tainted_cond, sizeof(tainted_cond));
   assert(dfsan_get_label(tainted_cond) == LabelIJ);
----------------
bipmis wrote:
> browneee wrote:
> > bipmis wrote:
> > > browneee wrote:
> > > > Thanks nikic and bipmis for checking this change with me :)
> > > > 
> > > > The point of the test is that tainted_cond should have taint labels propagated from above. This change would invalidate this test, and break the behavior this is testing for.
> > > > 
> > > > I wonder, what is the IR for `((DataI * DataJ) != 1)` before and after this change? I suspect this is where the taint label is being lost.
> > > > 
> > > Ok Thanks for explaining it.
> > > Let me discuss the IR for the original conditional_callbacks.c before and after the change. 
> > > I see the DataI and DataJ are obtained from argv as expected and same in both cases. The result calculation is completely avoided in the mod patch. The phi of the result values is also avoided. Instead it is converted to an i1 cmp based on the value check of DataJ==2. 
> > > {F28800923}
> > > 
> > > //The error I encounter with this is :
> > > /home/bipmis01/llvm_30may/llvm-project/compiler-rt/test/dfsan/conditional_callbacks.c:79:12: error: CHECK: expected string not found in input
> > >  // CHECK: Label 1 used as condition
> > > <stdin>:1:1: note: scanning from here
> > > conditional_callbacks.c.tmp: /home/bipmis01/llvm_30may/llvm-project/compiler-rt/test/dfsan/conditional_callbacks.c:35: void my_dfsan_conditional_callback(dfsan_label, dfsan_origin): Assertion `Label == LabelI' failed.//
> > Thanks. Now that I see the error, my suspicion was wrong (or maybe that is a second problem after the error).
> > 
> > When dfsan is tracking tainted data, this feature allows users to find when the data is used to make a decision (ie, used in a control flow condition).
> > 
> > `  int DataI = (Argv[0][0] != 0) ? 1 : 0;`   // 69: no callback here, because condition is not tainted
> > 
> > `  if (DataI) {`  // 80: DataI was tainted by `dfsan_set_label(LabelI, &DataI, sizeof(DataI));`
> > 
> > Because DataI is tainted, `my_dfsan_conditional_callback` gets called.
> > 
> > ```
> > static int Count = 0;
> >   switch (Count++) {
> > ```
> > The Count in my_dfsan_conditional_callback is a hack to have different logic each time my_dfsan_conditional_callback.
> > 
> > The assert that is failing on 35, when Count==0. So the first call has a tainted condition (otherwise it wouldn't be called) but it is not tainted with LabelI.
> > 
> > The failure of Label != LabelI probably means that my_dfsan_conditional_callback is first called from another location.
> > 
> > `  if (DataI) {` // 80
> > 
> > I think this code still happens (your diff; left 93  (IIUC, the left side of your diff is new?)), but later - so the calls to `my_dfsan_conditional_callback` would happen in a different order - and the assertions would fail.
> > 
> > 
> > I suggest temporarily remove the assertions in the switch in `my_dfsan_conditional_callback`, and see if it prints 3 things for the 3 `// CHECK:`. This would confirm things are still working, and the problem is that the test was not robust to code re-ordering.
> I think the first condition has been optimised away.
> ./conditional_callbacks.c.tmp-orig FooBarBaz
> Label 1 used as condition
> Label 2 used as condition
> Label 3 used as condition
> Label 3 used as condition
> 
> ./conditional_callbacks.c.tmp FooBarBaz
> Label 2 used as condition
> Label 3 used as condition
> 
> This can also be seen from the IR.
> A modification to the test as below now works. Not right but just a proof of the calls taking place
> 
> ```
>   switch (Count++) {
>   case 0:
>     assert(Label == LabelJ);
>     break;
>   case 1:
>     assert(Label == LabelIJ);
>     break;
>   default:
>     break;
>   }
> ```
> And remove the first check from source
> 
> ```
> // CHECK: Label 1 used as condition
> ```
> 
I think first condition being optimised away makes sense as an explanation, and matches that output.

Question is how to fix it?

##### Ideas

1) Mark dfsan_set_label second argument as volatile? I think this would prevent the optimization from removing the if. 
2) Change the code in the test so the optimization won't break the test.  e.g. maybe `result = identity(42);` where identity() is not inlined (I don't have a good sense for when optimizations will [not] occur).
3) Run DFSan transform pass earlier, before these optimizations? (maybe not feasible; probably a fair bit of work to figure out)

##### Re 1):
I think this is a problem here only because the `(Argv[0][0] != 0) ? 1 : 0;` ternary, the dfsan_set_label, and the if are all so close together. If dfsan_set_label was much earlier, then I expect the first ternary would trigger the check and it would work out ok.

Change would be
* https://github.com/llvm/llvm-project/blob/1034688d58783779168d59b47d2b3e897ad869c6/compiler-rt/include/sanitizer/dfsan_interface.h#L46
* https://github.com/llvm/llvm-project/blob/1034688d58783779168d59b47d2b3e897ad869c6/compiler-rt/lib/dfsan/dfsan.h#L31
* https://github.com/llvm/llvm-project/blob/1034688d58783779168d59b47d2b3e897ad869c6/compiler-rt/lib/dfsan/dfsan.cpp#L580
And similar surrounding functions.

This may be the best option, if it works.

##### Re 2):
It wouldn't surprise me if there are already other cases where a use in a condition is optimized away, and we just don't have a test pointing to it.  Would prefer to keep the if condition (modified to avoid optimization) rather than remove it.

I'm OK with this option.

##### Re 3): 
Probably much more effort than the other two. May create other problems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157740