[llvm] r369789 - [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null

Tue Aug 27 16:57:50 PDT 2019

OK, I think I understand now. I've been able to make some source fixes.

One case, above, is a custom data structure that can apparently exist
either on the stack or heap (AFAICT), and implements lookups using
"base+offset", where, when it's on the heap, base == <nullptr> and offset
== <where the data really is>, but when it's on the stack, base ==
<location of the base>, offset == <4, 8, or whatever> to index into that.
The easy fix is to translate "base + offset" to "base ? base + offset :
reinterpret_cast<const char*>(offset)" -- I think that works around the UB
you mentioned. (I'm not sure it's *completely* UB-free, but it's at least
less UB...)

And then there's also this glory:
https://github.com/google/filament/blob/772af1e8971e65bbff314cf696a9f32da79e3485/filament/backend/include/private/backend/CommandStream.h#L102
(hint: there's a loop elsewhere that needs that to be nullptr to
terminate...)

Anyway, horrifying stuff, but seems to be not a miscompile after all. I'm
surprised sanitizers didn't catch this, but I guess this kind of check just
hasn't been implemented yet. Sorry for the false alarm!

On Tue, Aug 27, 2019 at 12:39 PM Jordan Rupprecht <rupprecht at google.com>
wrote:

>
>
> On Tue, Aug 27, 2019 at 12:29 PM Philip Reames <listmail at philipreames.com>
> wrote:
>
>> Well, your problem is that your code is undefined.
>>
>> All inbounds pointers derived from a non-null base are non-null.  (By
>> assumption, they can't wrap.)  And if you have a null base, then there are
>> no inbounds derived pointers other than null itself.  So, in the case
>> below, the result of isValid is very likely to be poison which means
>> branching on it is UB.  The only non-poison result would be if %1 !=
>> nullptr.
>>
>> Philip
>>
>> So the only case which can fail is when %1 is null.
>>
> Turns out that's the case -- when I print out the corresponding var for
> %1, it's nullptr. So the pointer is calculated as base+offset, but
> IsValid() is essentially only looking at base.
>
> I'm not sure I understand the rest -- I'll try distilling the test case
> into some more.
>
>> On 8/27/19 11:19 AM, Jordan Rupprecht wrote:
>>
>> Yes, it's inbounds.
>>
>> Still narrowing it down (I spent all day yesterday just finding the file
>> with the suspected miscompile). I'm snipping out a ton, but the
>> relevant bits in the .ll seems to be:
>>
>> define doSomething() {
>> entry:
>>   %1 = load i8*, i8** %<internal/mangled name of foo.x>, align 8
>>   %8 = load i64, i64* %offset31.i, align 1
>> ...
>> while.body.lr.ph.i:
>>   br label %while.body.i
>> while.body.i:
>>   %9 = phi i64 [ %8, %while.body.lr.ph.i ], [ %11, %if.end.i ]
>>   %add.ptr.i = getelementptr inbounds i8, i8* %1, i64 %9
>> ...
>> if.end.i:
>>   %11 = load i64, i64* %offset.i, align 1
>> ...
>>   %tobool.i15 = icmp eq i8* %add.ptr.i, null  ; <-- relevant inlined
>> IsValid() check
>> }
>>
>> => that last check is now:
>>   %tobool.i15 = icmp eq i8* %1, null
>>
>> (the "valid pointer" is a base+offset, which I guess is why it got the
>> name %add.ptr.i, if that's at all relevant)
>>
>> I've also confirmed it's these two patches causing the failure, and not
>> any regressions in the broken window -- i.e. the test fails at r369795, but
>> passes if I take r369788 as the baseline and then apply r369790 - r369794.
>>
>> On Mon, Aug 26, 2019 at 9:53 PM Philip Reames <listmail at philipreames.com>
>> wrote:
>>
>>> Is there any chance <valid_pointer> is a inbounds geps? Without that, I
>>> can't see the connection to the patch.
>>>
>>> Philip
>>> On 8/26/19 5:05 PM, Jordan Rupprecht wrote:
>>>
>>> btw, we're seeing something that looks like miscompiles with this patch
>>> (& after the fix in r369795). The code looks pretty well defined (passes
>>> asan/msan/etc.).
>>>
>>> I'm trying to reduce it into something that actually triggers the bug,
>>> but it vaguely looks like:
>>> struct Foo {
>>>  bool IsValid() const { return x; }
>>>  Foo(char *y): x(y) {}
>>>  char *x;
>>> }
>>>
>>> Foo get() {
>>>   return some_condition ? Foo(<valid pointer>) : Foo(nullptr);
>>> }
>>>
>>> void doSomething() {
>>>   Foo foo = get();
>>>   if (foo.IsValid()) { .. }
>>> }
>>>
>>> What I'm observing is get() go down the branch of some_condition being
>>> true and returning Foo(<valid pointer>), but foo.IsValid() is returning
>>> false.
>>>
>>> Given that context, is there anything you might see is a subtle bug with
>>> this patch?
>>>
>>> On Fri, Aug 23, 2019 at 10:57 AM Philip Reames via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Author: reames
>>>> Date: Fri Aug 23 10:58:58 2019
>>>> New Revision: 369789
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=369789&view=rev
>>>> Log:
>>>> [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P,
>>>> null
>>>>
>>>> This generalizes the isGEPKnownNonNull rule from ValueTracking to apply
>>>> when we do not know if the base is non-null, and thus need to replace one
>>>> condition with another.
>>>>
>>>> The core notion is that since an inbounds GEP can only form null if the
>>>> base pointer is null and the offset is zero. However, if the offset is
>>>> non-zero, the the "inbounds" marker makes the result poison. Thus, we're
>>>> free to ignore the case where the offset is non-zero. Similarly, there's no
>>>> case under which a non-null base can result in a null result without
>>>> generating poison.
>>>>
>>>> Differential Revision: https://reviews.llvm.org/D66608
>>>>
>>>>
>>>> Added:
>>>>     llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll
>>>> Modified:
>>>>     llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>>>>
>>>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=369789&r1=369788&r2=369789&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>>>> (original)
>>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Fri
>>>> Aug 23 10:58:58 2019
>>>> @@ -894,6 +894,27 @@ Instruction *InstCombiner::foldGEPICmp(G
>>>>        Offset = EmitGEPOffset(GEPLHS);
>>>>      return new ICmpInst(ICmpInst::getSignedPredicate(Cond), Offset,
>>>>                          Constant::getNullValue(Offset->getType()));
>>>> +  } else if (GEPLHS->isInBounds() && ICmpInst::isEquality(Cond) &&
>>>> +             GEPLHS->getType()->isPointerTy() && // TODO: extend to
>>>> vector geps
>>>> +             isa<Constant>(RHS) && cast<Constant>(RHS)->isNullValue()
>>>> &&
>>>> +             !NullPointerIsDefined(I.getFunction(),
>>>> +
>>>>  RHS->getType()->getPointerAddressSpace())) {
>>>> +    // For most address spaces, an allocation can't be placed at null,
>>>> but null
>>>> +    // itself is treated as a 0 size allocation in the in bounds
>>>> rules.  Thus,
>>>> +    // the only valid inbounds address derived from null, is null
>>>> itself.
>>>> +    // Thus, we have four cases to consider:
>>>> +    // 1) Base == nullptr, Offset == 0 -> inbounds, null
>>>> +    // 2) Base == nullptr, Offset != 0 -> poison as the result is out
>>>> of bounds
>>>> +    // 3) Base != nullptr, Offset == (-base) -> poison (crossing
>>>> allocations)
>>>> +    // 4) Base != nullptr, Offset != (-base) -> nonnull (and possibly
>>>> poison)
>>>> +    //
>>>> +    // (Note if we're indexing a type of size 0, that simply collapses
>>>> into one
>>>> +    //  of the buckets above.)
>>>> +    //
>>>> +    // In general, we're allowed to make values less poison (i.e.
>>>> remove
>>>> +    //   sources of full UB), so in this case, we just select between
>>>> the two
>>>> +    //   non-poison cases (1 and 4 above).
>>>> +    return new ICmpInst(Cond, GEPLHS->getPointerOperand(), RHS);
>>>>    } else if (GEPOperator *GEPRHS = dyn_cast<GEPOperator>(RHS)) {
>>>>      // If the base pointers are different, but the indices are the
>>>> same, just
>>>>      // compare the base pointer.
>>>>
>>>> Added: llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll?rev=369789&view=auto
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll (added)
>>>> +++ llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll Fri Aug
>>>> 23 10:58:58 2019
>>>> @@ -0,0 +1,184 @@
>>>> +; NOTE: Assertions have been autogenerated by
>>>> utils/update_test_checks.py
>>>> +; RUN: opt -S < %s -instcombine | FileCheck %s
>>>> +
>>>> +;; Start by showing the results of constant folding (which doesn't use
>>>> +;; the poison implied by gep for the nonnull cases).
>>>> +
>>>> +define i1 @test_ne_constants_null() {
>>>> +; CHECK-LABEL: @test_ne_constants_null(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    ret i1 false
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8* null, i64 0
>>>> +  %cnd = icmp ne i8* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +define i1 @test_ne_constants_nonnull() {
>>>> +; CHECK-LABEL: @test_ne_constants_nonnull(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    ret i1 true
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8* null, i64 1
>>>> +  %cnd = icmp ne i8* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +define i1 @test_eq_constants_null() {
>>>> +; CHECK-LABEL: @test_eq_constants_null(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    ret i1 true
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8* null, i64 0
>>>> +  %cnd = icmp eq i8* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +define i1 @test_eq_constants_nonnull() {
>>>> +; CHECK-LABEL: @test_eq_constants_nonnull(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    ret i1 false
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8* null, i64 1
>>>> +  %cnd = icmp eq i8* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +;; Then show the results for non-constants.  These use the inbounds
>>>> provided
>>>> +;; UB fact to ignore the possible overflow cases.
>>>> +
>>>> +define i1 @test_ne(i8* %base, i64 %idx) {
>>>> +; CHECK-LABEL: @test_ne(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    [[CND:%.*]] = icmp ne i8* [[BASE:%.*]], null
>>>> +; CHECK-NEXT:    ret i1 [[CND]]
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8* %base, i64 %idx
>>>> +  %cnd = icmp ne i8* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +define i1 @test_eq(i8* %base, i64 %idx) {
>>>> +; CHECK-LABEL: @test_eq(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    [[CND:%.*]] = icmp eq i8* [[BASE:%.*]], null
>>>> +; CHECK-NEXT:    ret i1 [[CND]]
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8* %base, i64 %idx
>>>> +  %cnd = icmp eq i8* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +;; TODO: vectors not yet handled
>>>> +define <2 x i1> @test_vector_base(<2 x i8*> %base, i64 %idx) {
>>>> +; CHECK-LABEL: @test_vector_base(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    [[GEP:%.*]] = getelementptr inbounds i8, <2 x i8*>
>>>> [[BASE:%.*]], i64 [[IDX:%.*]]
>>>> +; CHECK-NEXT:    [[CND:%.*]] = icmp eq <2 x i8*> [[GEP]],
>>>> zeroinitializer
>>>> +; CHECK-NEXT:    ret <2 x i1> [[CND]]
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, <2 x i8*> %base, i64 %idx
>>>> +  %cnd = icmp eq <2 x i8*> %gep, zeroinitializer
>>>> +  ret <2 x i1> %cnd
>>>> +}
>>>> +
>>>> +define <2 x i1> @test_vector_index(i8* %base, <2 x i64> %idx) {
>>>> +; CHECK-LABEL: @test_vector_index(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    [[GEP:%.*]] = getelementptr inbounds i8, i8*
>>>> [[BASE:%.*]], <2 x i64> [[IDX:%.*]]
>>>> +; CHECK-NEXT:    [[CND:%.*]] = icmp eq <2 x i8*> [[GEP]],
>>>> zeroinitializer
>>>> +; CHECK-NEXT:    ret <2 x i1> [[CND]]
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8* %base, <2 x i64> %idx
>>>> +  %cnd = icmp eq <2 x i8*> %gep, zeroinitializer
>>>> +  ret <2 x i1> %cnd
>>>> +}
>>>> +
>>>> +;; These two show instsimplify's reasoning getting to the non-zero
>>>> offsets
>>>> +;; before instcombine does.
>>>> +
>>>> +define i1 @test_eq_pos_idx(i8* %base) {
>>>> +; CHECK-LABEL: @test_eq_pos_idx(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    ret i1 false
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8* %base, i64 1
>>>> +  %cnd = icmp eq i8* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +define i1 @test_eq_neg_idx(i8* %base) {
>>>> +; CHECK-LABEL: @test_eq_neg_idx(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    ret i1 false
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8* %base, i64 -1
>>>> +  %cnd = icmp eq i8* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +;; Show an example with a zero sized type since that's
>>>> +;; a cornercase which keeps getting mentioned.  The GEP
>>>> +;; produces %base regardless of the value of the index
>>>> +;; expression.
>>>> +define i1 @test_size0({}* %base, i64 %idx) {
>>>> +; CHECK-LABEL: @test_size0(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    [[CND:%.*]] = icmp ne {}* [[BASE:%.*]], null
>>>> +; CHECK-NEXT:    ret i1 [[CND]]
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds {}, {}* %base, i64 %idx
>>>> +  %cnd = icmp ne {}* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +define i1 @test_size0_nonzero_offset({}* %base) {
>>>> +; CHECK-LABEL: @test_size0_nonzero_offset(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    [[CND:%.*]] = icmp ne {}* [[BASE:%.*]], null
>>>> +; CHECK-NEXT:    ret i1 [[CND]]
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds {}, {}* %base, i64 15
>>>> +  %cnd = icmp ne {}* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +
>>>> +;; Finally, some negative tests for sanity checking.
>>>> +
>>>> +define i1 @neq_noinbounds(i8* %base, i64 %idx) {
>>>> +; CHECK-LABEL: @neq_noinbounds(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    [[GEP:%.*]] = getelementptr i8, i8* [[BASE:%.*]], i64
>>>> [[IDX:%.*]]
>>>> +; CHECK-NEXT:    [[CND:%.*]] = icmp ne i8* [[GEP]], null
>>>> +; CHECK-NEXT:    ret i1 [[CND]]
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr i8, i8* %base, i64 %idx
>>>> +  %cnd = icmp ne i8* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>> +
>>>> +define i1 @neg_objectatnull(i8 addrspace(2)* %base, i64 %idx) {
>>>> +; CHECK-LABEL: @neg_objectatnull(
>>>> +; CHECK-NEXT:  entry:
>>>> +; CHECK-NEXT:    [[GEP:%.*]] = getelementptr inbounds i8, i8
>>>> addrspace(2)* [[BASE:%.*]], i64 [[IDX:%.*]]
>>>> +; CHECK-NEXT:    [[CND:%.*]] = icmp eq i8 addrspace(2)* [[GEP]], null
>>>> +; CHECK-NEXT:    ret i1 [[CND]]
>>>> +;
>>>> +entry:
>>>> +  %gep = getelementptr inbounds i8, i8 addrspace(2)* %base, i64 %idx
>>>> +  %cnd = icmp eq i8 addrspace(2)* %gep, null
>>>> +  ret i1 %cnd
>>>> +}
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190827/bc0bc4c5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4849 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190827/bc0bc4c5/attachment.bin>