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

Jordan Rupprecht via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 28 22:14:16 PDT 2019


Any attempt at reducing in such a way that it produces test failures has
failed so far. Even things like adding a printf causes the tests to
suddenly pass. I'll still keep plugging at it, but I'm becoming less
hopeful that I'll have a full working example of a failure.

Still, for someone familiar with how these things work, there might be some
generic diagnostics/sanitizers/etc. for cases of doing `if (ptr + offset)`
if you can somehow conclude the behavior is UB & optimizations would be
unexpected (to people not intricately familiar with C++ rules), e.g. (ptr,
offset) = (0, <non-zero), or ptr == -offset (in which case integer
arithmetic says ptr+offset = 0, but this patch allows optimizing `if
(ptr+offset)` to `if (ptr)`).

On Wed, Aug 28, 2019 at 2:15 AM Roman Lebedev <lebedev.ri at gmail.com> wrote:

> On Wed, Aug 28, 2019 at 2:58 AM Jordan Rupprecht via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> >
> > 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!
> Do you have a small standalone reproducer for this, please?
> I'd be interested to take a look from sanitizer perspective.
>
> Roman.
>
> > 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
> >
> > _______________________________________________
> > 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/20190828/f6d23876/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/20190828/f6d23876/attachment.bin>


More information about the llvm-commits mailing list