[PATCH] D71960: [Attributor] AAUndefinedBehavior: Use AAValueSimplify in memory accessing instructions.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

One last nit with one of the tests, then I'll commit this.

In D71960#1877265 <https://reviews.llvm.org/D71960#1877265>, @baziotis wrote:

> In D71960#1877001 <https://reviews.llvm.org/D71960#1877001>, @xbolva00 wrote:
>
> > I have a similar case how to exploit UB to simplify code:
> >
> >   int process(char *p) __attribute__((nonnull));
> >  
> >  
> >   void doit(bool b, char *d) {
> >       char *p = 0;
> >       if (b)
> >           p = d;
> >       process(p);
> >   }
> >
> >
> > https://godbolt.org/z/eYcApm
>
>
>   In this case, it is invalid to call process with null pointer since args are nonnull, so we can remove branch and call process(d) directly.


Nit: In the godbold example you don't even need the `__attribute__((nonnull))` for this, the access is sufficient ;)

> Thanks for the proposal! At first glance, deleting the branch is not straightforward and may not make a lot of sense. Reaching the `call`, you have a `phi` node basically saying:
> 
> - If you got into the branch, the value is `d`.
> - If not, the value is `p` (which with constant propagation, let's say `null`).

This is where AAIsDead needs to talk to AAUnreachable at some point but we have simpler cases to tackle first.

> Deleting the branch first of all seems to require somehow going "backwards", tracking where this (or these) value came from. Second, I can't find a reasoning on how to generalize this (except since we have UB, do whatever)
>  because the previous code is not invalid, only the `call`.

I was always hoping for backwards propagation of UB. Let's start with propagating unreachable backwards. There is already a TODO in the code btw:
`// TODO: look for (assumed) UB to backwards propagate "deadness".`

A good first step would be to check the MustBeExecutedContext either backwards from known UB, e.g. hitting an unreachable, or forward after a control dependence change (e.g, after a conditional branch).
The backwards propagation is not cleaned up and integrated yet (for MustBeExecutedContext, it's somewhere in a review though).
One could maybe work on that or go with the forward approach. This should be done in AAIsDeadFunction.

> That said, personally I think it's a very good idea to check if null arguments are passed to `nonnull` parameters and consider that UB. Then, we can make the `call` unreachable.

I generally agree but we have to be a bit careful. People don't like "obviously harmless" UB to be exploited, e.g., passing `null` to a `nonnull` argument that is unused should not cause an `unreachable`



================
Comment at: llvm/test/Transforms/Attributor/undefined_behavior.ll:80
+  call void @load_null_propagated2(i32* null)
+  ret void
 }
----------------
You have to return the value or sth. As it is, the call has no side-effects and is deleted either way.


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

https://reviews.llvm.org/D71960