llvm dev - Feb 2016 - Possible soundness issue with available_externally (split from "RFC: Add guard intrinsics")

On Thu, Feb 25, 2016 at 9:59 AM, Sanjoy Das
<sanjoy at playingwithpointers.com> wrote:
> Couple of other examples:
>
>   void @foo(i32* %ptr) available_externally {
>     %discard = load i32, i32* %ptr
>   }
>   void bar() {
>     call @foo(i32* %x)
>   }
>
> ==>
>
>   void @foo(i32* %ptr) available_externally {
>   }
>   void bar() {
>     call @foo(i32* %x)
>   }
>
> ==>
>
>   void @foo(i32* %ptr) available_externally {
>   }
>   void bar() {
>     call @foo(i32* undef) ;; non optimized @foo will crash
>   }
>
>   ;; Similar example if @foo was dividing something by an integer
>   ;; argument
>
> We've actually seen the above in our VM (though back then we
> didn't realize that the problem was more general than the one
> case above).
^ This particular case above is already fixed in DeadArgElim:

bool DAE::RemoveDeadArgumentsFromCallers(Function &Fn)
{
  // We cannot change the arguments if this TU does not define the function or
  // if the linker may choose a function body from another TU, even if the
  // nominal linkage indicates that other copies of the function have the same
  // semantics. In the below example, the dead load from %p may not have been
  // eliminated from the linker-chosen copy of f, so replacing %p with undef
  // in callers may introduce undefined behavior.
  //
  // define linkonce_odr void @f(i32* %p) {
  //   %v = load i32 %p
  //   ret void
  // }
  if (!Fn.isStrongDefinitionForLinker())
    return false;

-- Sanjoy

On Fri, Feb 26, 2016 at 7:08 PM Sanjoy Das <sanjoy at
playingwithpointers.com>
wrote:
> On Thu, Feb 25, 2016 at 9:59 AM, Sanjoy Das
> <sanjoy at playingwithpointers.com> wrote:
> > Couple of other examples:
> >
> >   void @foo(i32* %ptr) available_externally {
> >     %discard = load i32, i32* %ptr
> >   }
> >   void bar() {
> >     call @foo(i32* %x)
> >   }
> >
> > ==>
> >
> >   void @foo(i32* %ptr) available_externally {
> >   }
> >   void bar() {
> >     call @foo(i32* %x)
> >   }
> >
> > ==>
> >
> >   void @foo(i32* %ptr) available_externally {
> >   }
> >   void bar() {
> >     call @foo(i32* undef) ;; non optimized @foo will crash
> >   }
> >
> >   ;; Similar example if @foo was dividing something by an integer
> >   ;; argument
> >
> > We've actually seen the above in our VM (though back then we
> > didn't realize that the problem was more general than the one
> > case above).
>
> ^ This particular case above is already fixed in DeadArgElim:
>
Fascinating. So I'm just proposing that we need to use similar logic in
other IPO passes.

We should probably factor it. Maybe we just need to admit mayBeOverridden
in more cases. It's not clear that there is any meaningful delta between
what has been discussed and just putting these cases into that set.

-Chandler

>
> bool DAE::RemoveDeadArgumentsFromCallers(Function &Fn)
> {
>   // We cannot change the arguments if this TU does not define the
> function or
>   // if the linker may choose a function body from another TU, even if the
>   // nominal linkage indicates that other copies of the function have the
> same
>   // semantics. In the below example, the dead load from %p may not have
> been
>   // eliminated from the linker-chosen copy of f, so replacing %p with
> undef
>   // in callers may introduce undefined behavior.
>   //
>   // define linkonce_odr void @f(i32* %p) {
>   //   %v = load i32 %p
>   //   ret void
>   // }
>   if (!Fn.isStrongDefinitionForLinker())
>     return false;
>
> -- Sanjoy
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160227/d7524a33/attachment-0001.html>

On Fri, Feb 26, 2016 at 7:11 PM, Chandler Carruth <chandlerc at
google.com> wrote:
> On Fri, Feb 26, 2016 at 7:08 PM Sanjoy Das <sanjoy at
playingwithpointers.com>
> wrote:
>>
>> On Thu, Feb 25, 2016 at 9:59 AM, Sanjoy Das
>> <sanjoy at playingwithpointers.com> wrote:
>> > Couple of other examples:
>> >
>> >   void @foo(i32* %ptr) available_externally {
>> >     %discard = load i32, i32* %ptr
>> >   }
>> >   void bar() {
>> >     call @foo(i32* %x)
>> >   }
>> >
>> > ==>
>> >
>> >   void @foo(i32* %ptr) available_externally {
>> >   }
>> >   void bar() {
>> >     call @foo(i32* %x)
>> >   }
>> >
>> > ==>
>> >
>> >   void @foo(i32* %ptr) available_externally {
>> >   }
>> >   void bar() {
>> >     call @foo(i32* undef) ;; non optimized @foo will crash
>> >   }
>> >
>> >   ;; Similar example if @foo was dividing something by an integer
>> >   ;; argument
>> >
>> > We've actually seen the above in our VM (though back then we
>> > didn't realize that the problem was more general than the one
>> > case above).
>>
>> ^ This particular case above is already fixed in DeadArgElim:
>
>
> Fascinating. So I'm just proposing that we need to use similar logic in
> other IPO passes.
Yeah, so far I don't see any way around it.  Refinement is everywhere,
and even if we are able to audit it out *now*, keeping it out will be
a major burden.

-- Sanjoy
>
> We should probably factor it. Maybe we just need to admit mayBeOverridden
in
> more cases. It's not clear that there is any meaningful delta between
what
> has been discussed and just putting these cases into that set.
>
> -Chandler
>
>>
>>
>> bool DAE::RemoveDeadArgumentsFromCallers(Function &Fn)
>> {
>>   // We cannot change the arguments if this TU does not define the
>> function or
>>   // if the linker may choose a function body from another TU, even if
the
>>   // nominal linkage indicates that other copies of the function have
the
>> same
>>   // semantics. In the below example, the dead load from %p may not
have
>> been
>>   // eliminated from the linker-chosen copy of f, so replacing %p with
>> undef
>>   // in callers may introduce undefined behavior.
>>   //
>>   // define linkonce_odr void @f(i32* %p) {
>>   //   %v = load i32 %p
>>   //   ret void
>>   // }
>>   if (!Fn.isStrongDefinitionForLinker())
>>     return false;
>>
>> -- Sanjoy


-- 
Sanjoy Das
http://playingwithpointers.com