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

[Biplob Mishra via Phabricator via llvm-commits]

bipmis 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);
----------------
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
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157740