[InstSimplify] Don't fold comparisons of non-inbounds GEPs

[LemonBoy]

Summary

The logic employed by the constant folder assumes the GEPs to be inbounds, when applied to other GEPs the result can be potentially incorrect and cause miscompilations.

[LemonBoy]

llvm/lib/IR/ConstantFold.cpp

1837

Are non-inbounds GEP common enough to implement this?

[lebedev.ri]

This seems to be correct?
https://alive2.llvm.org/ce/z/JyimAB

[nikic]

@nlopes Can you please take a look at the alive result? I don't think the transform is correct, but alive says it is. The result of the GEP is a pointer of value zero. It has different provenance from the null pointer, but icmp eq should be a pure value comparison, independent of provenance, right?

llvm/lib/IR/ConstantFold.cpp

1837

I'd generally say that we should handle inbounds precisely just for the sake of clarity -- however, after reviewing all the folds below, it looks like all of them either require inbounds, or can only work on GEPs for which we would infer inbounds anyway (e.g. the gep GV1, 0 icmp gep GV2, 0 case). Given that, the early out here should be fine, and I think you can drop the TODO as well.

1854

Unrelated to this patch, but all the isSigned handling in this code looks wrong to me. This is effectively saying that GlobalValues can only appear in the lower half of the address space and not in the upper (negative) half, which is not true.

[aqjune]

I don't have a clear model for the semantics of pointer comparison ATM; Pointer comparison *sometimes* needs to take provenance into consideration because LLVM folds p1 == p2 where p1 and p2 are pointing to two different zero-size objects having the same address into false.
Also, considering provenance into account allows aggressively folding pointer comparisons. It isn't clear how *frequently* the provenance should be considered.

But, I'm rather curious about how the miscompilation happened from this optimization. A gep with such offset isn't common, unless a programmer writes a code that subtracts a pointer from null (which is already fishy)?
It would be great if I can see the input that causes miscompilation.

[aqjune]

I don't have a clear model for the semantics of pointer comparison ATM; Pointer comparison *sometimes* needs to take provenance into consideration because LLVM folds p1 == p2 where p1 and p2 are pointing to two different zero-size objects having the same address into false.
Also, considering provenance into account allows aggressively folding pointer comparisons. It isn't clear how *frequently* the provenance should be considered.

FWIW, these are a few examples and relevant links:

• A ptr comparison folding to false regardless of addresses of two possibly overlapping stack allocations: https://godbolt.org/z/ok4vFJ , bugzilla report: https://bugs.llvm.org/show_bug.cgi?id=44342
• Another example: https://godbolt.org/z/bhrsK1 (well it wasn't a zero-size object, but simply merging two identical global objects. Anyway it shows that defining ptr comparison is something non-trivial), reduced from: https://bugs.llvm.org/show_bug.cgi?id=47090 , https://github.com/rust-lang/rust/issues/73722

[nikic]

In D97665#2601852, @aqjune wrote:

I don't have a clear model for the semantics of pointer comparison ATM; Pointer comparison *sometimes* needs to take provenance into consideration because LLVM folds p1 == p2 where p1 and p2 are pointing to two different zero-size objects having the same address into false.
Also, considering provenance into account allows aggressively folding pointer comparisons. It isn't clear how *frequently* the provenance should be considered.

But, I'm rather curious about how the miscompilation happened from this optimization. A gep with such offset isn't common, unless a programmer writes a code that subtracts a pointer from null (which is already fishy)?
It would be great if I can see the input that causes miscompilation.

I'm not sure why zig generates this code, but the context here is https://github.com/ziglang/zig/issues/6408. The issue was exposed by D93820, as we previously simply folded away the GEP to null, losing provenance.

My understanding was that the current resolution on pointer comparison issues is that pointer comparison is value-only, and provenance-losing equality replacements in GVN are what we consider incorrect. But I haven't followed all the discussions.

The original case here was actually not "gep == null", but "ptrtoint gep == 0", in which case it's obviously a pure value comparison, but InstSimplify looks through that. Do you think that's the real issue?

[LemonBoy]

But, I'm rather curious about how the miscompilation happened from this optimization. A gep with such offset isn't common, unless a programmer writes a code that subtracts a pointer from null (which is already fishy)?
It would be great if I can see the input that causes miscompilation.

As stated here the gep is being generated by the SCEV pass.
A minimal test case that shows the gep being created (but not this miscompilation) is here.

llvm/lib/IR/ConstantFold.cpp

1837

Some trivial geps can still be folded such as all-zero ones vs constant or unsigned comparisons with zero. But yes, I'll drop the TODO as those cases are pretty rare.

1854

Yes, that looks wrong. I guess this is unlikely to cause problems as most (every?) pointer comparisons against null are of eq/neq type.

[lebedev.ri]

In D97665#2602726, @LemonBoy wrote:

But, I'm rather curious about how the miscompilation happened from this optimization. A gep with such offset isn't common, unless a programmer writes a code that subtracts a pointer from null (which is already fishy)?
It would be great if I can see the input that causes miscompilation.

As stated here the gep is being generated by the SCEV pass.
A minimal test case that shows the gep being created (but not this miscompilation) is here.

(i've already been looking at the scev problem, maybe will post patch soon-ish)

[nikic]

Resigning here, based on past experience trying to challenge the Word Of God. It really doesn't help that someone opened an alive issue for this without cross-referencing it, so the same discussion is repeated twice.

[aqjune]

In D97665#2602696, @nikic wrote:

The original case here was actually not "gep == null", but "ptrtoint gep == 0", in which case it's obviously a pure value comparison, but InstSimplify looks through that. Do you think that's the real issue?

Actually this is indeed a problematic transformation; you said a very valid point.
Removing it has large impact (it leaves ptrtoint to an object which is conservatively considered to escape the object), so simply removing the transformation is hard ATM, but I believe it should be removed at some point.

It is bad that there were two discussions with the same topic; maybe the link was not shared here because the Alive2's side did not have a very clear solution about ptr comparison as well. If I could answer anything clear about the pointer comparison, the link must have been shared :(

In D97665#2602738, @lebedev.ri wrote:

(i've already been looking at the scev problem, maybe will post patch soon-ish)

+1, Fixing SCEV seems to be the easiest solution at this point.

[nikic]

Alive pointer icmp semantics have recently been fixed and now agree that this is a miscompile, so I think we should move forward with this patch. It will need a rebase as some additional tests have been added in the meantime. This should also fix https://bugs.llvm.org/show_bug.cgi?id=50208.

[LemonBoy]

Rebased. I've moved the test to icmp-null.ll and made it a bit nicer.

[nikic]

llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll

203 ↗

(On Diff #344148)

For the tests that changed, could you please add a copy that has the inbounds keyword, so that the folding case is still covered?

[LemonBoy]

llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll

203 ↗

(On Diff #344148)

There's something weird going on, SimplifyGEPInst in InstructionSimplify.cpp is silently dropping the inbounds flag thus preventing the fold from happening.

[LemonBoy]

Ping?

[nlopes]

Since we settled on pointer comparison being equivalent to address comparison (i.e., provenance is not taken into account), then the current code is correct.
See table 2 here, for example: https://web.ist.utl.pt/nuno.lopes/pubs/alive2-mem-cav21.pdf#page=10

Therefore I suggest we drop this patch.

The optimization that is wrong under this model is one in InstSimplify that folds pointer comparison between pointers of different objects to false. That is not correct for geps that aren't inbounds.

llvm/lib/IR/ConstantFold.cpp

case Instruction::SExt:
         return evaluateICmpRelation(CE1Op0,
                                     Constant::getNullValue(CE1Op0->getType()),
                                     isSigned);
       }
       break;
 
     case Instruction::GetElementPtr: {
       GEPOperator *CE1GEP = cast<GEPOperator>(CE1);
+
+      // The foldings below assume the GEP to be inbounds.
+      // TODO: Some of the logic can be applied for non-inbounds GEPs as well.
+      if (!CE1GEP->isInBounds())
+        return ICmpInst::BAD_ICMP_PREDICATE;
+
       // Ok, since this is a getelementptr, we know that the constant has a
       // pointer type.  Check the various cases.
       if (isa<ConstantPointerNull>(V2)) {
         // If we are comparing a GEP to a null pointer, check to see if the base
         // of the GEP equals the null pointer.
         if (const GlobalValue *GV = dyn_cast<GlobalValue>(CE1Op0)) {
           if (GV->hasExternalWeakLinkage())
             // Weak linkage GVals could be zero or not. We're comparing that
             // to null pointer so its greater-or-equal
             return isSigned ? ICmpInst::ICMP_SGE : ICmpInst::ICMP_UGE;
           else
             // If its not weak linkage, the GVal must have a non-zero address
             // so the result is greater-than
             return isSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
         } else if (isa<ConstantPointerNull>(CE1Op0)) {
           // If we are indexing from a null pointer, check to see if we have any
           // non-zero indices.
           for (unsigned i = 1, e = CE1->getNumOperands(); i != e; ++i)
             if (!CE1->getOperand(i)->isNullValue())
               // Offsetting from null, must not be equal.
               return isSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
           // Only zero indexes from null, must still be zero.

llvm/test/Transforms/InstSimplify/icmp.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -instsimplify -S | FileCheck %s
 
 target datalayout = "e-p:64:64:64-p1:16:16:16-p2:32:32:32-p3:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
 
+@g = internal unnamed_addr constant [2 x i8] c".\00", align 1
+
 declare void @usei8ptr(i8* %ptr)
 
 ; Ensure that we do not crash when looking at such a weird bitcast.
 define i1 @bitcast_from_single_element_pointer_vector_to_pointer(<1 x i8*> %ptr1vec, i8* %ptr2) {
 ; CHECK-LABEL: @bitcast_from_single_element_pointer_vector_to_pointer(
 ; CHECK-NEXT:    [[PTR1:%.*]] = bitcast <1 x i8*> [[PTR1VEC:%.*]] to i8*
 ; CHECK-NEXT:    call void @usei8ptr(i8* [[PTR1]])
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[PTR1]], [[PTR2:%.*]]
 ; CHECK-NEXT:    ret i1 [[CMP]]
 ;
   %ptr1 = bitcast <1 x i8*> %ptr1vec to i8*
   call void @usei8ptr(i8* %ptr1)
   %cmp = icmp eq i8* %ptr1, %ptr2
   ret i1 %cmp
 }
 
+; Ensure the comparison is not incorrectly folded into `false`.
+define i1 @compare_non_inbounds_gep_zero() {
+; CHECK-LABEL: @compare_non_inbounds_gep_zero(
+; CHECK-NEXT:    ret i1 icmp eq (i8* getelementptr ([2 x i8], [2 x i8]* @g, i64 0, i64 sub (i64 0, i64 ptrtoint ([2 x i8]* @g to i64))), i8* null)
+;
+  %cmp = icmp eq i8* getelementptr ([2 x i8], [2 x i8]* @g, i64 0, i64 sub (i64 0, i64 ptrtoint ([2 x i8]* @g to i64))), null
+  ret i1 %cmp
+}
+
 ; TODO: these should return poison
 
 define i1 @poison(i32 %x) {
 ; CHECK-LABEL: @poison(
 ; CHECK-NEXT:    ret i1 undef
 ;
   %v = icmp eq i32 %x, poison
   ret i1 %v