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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

In D71960#1877393 <https://reviews.llvm.org/D71960#1877393>, @jdoerfert wrote:

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


Ok thanks, I'll address it. Looking at the repaired tests, I can't see something that this patch exposed
that is not tested in `undefined_behavior.ll` test.

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

May I add: You don't need it //because of the Attributor// (which puts the `nonnull` since there is an access).

> 
> 
>> 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.

It seems that what you're describing is similar to: https://reviews.llvm.org/D71974 when checking for known UB.

> 
> 
>> 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`

Right, I hadn't thought of that.


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

https://reviews.llvm.org/D71960