From 7ecad2e4ced180b4fdebc6b7bf6d26d83b454318 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Thu, 24 Dec 2020 17:04:40 +0100 Subject: [PATCH] [InstSimplify] Don't fold gep p, -p to null This is a partial fix for https://bugs.llvm.org/show_bug.cgi?id=44403. Folding gep p, q-p to q is only legal if p and q have the same provenance. This fold should probably be guarded by something like getUnderlyingObject(p) == getUnderlyingObject(q). This patch is a partial fix that removes the special handling for gep p, 0-p, which will fold to a null pointer, which would certainly not pass an underlying object check (unless p is also null, in which case this would fold trivially anyway). Folding to a null pointer is particularly problematic due to the special handling it receives in many places, making end-to-end miscompiles more likely. Differential Revision: https://reviews.llvm.org/D93820 --- llvm/lib/Analysis/InstructionSimplify.cpp | 24 +++++++++++++------- llvm/test/Transforms/InstSimplify/gep.ll | 37 ++++++++++++++++++++++++++----- 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp index 96a3ada..2ae4228 100644 --- a/llvm/lib/Analysis/InstructionSimplify.cpp +++ b/llvm/lib/Analysis/InstructionSimplify.cpp @@ -4270,9 +4270,7 @@ static Value *SimplifyGEPInst(Type *SrcTy, ArrayRef Ops, // doesn't truncate the pointers. if (Ops[1]->getType()->getScalarSizeInBits() == Q.DL.getPointerSizeInBits(AS)) { - auto PtrToIntOrZero = [GEPTy](Value *P) -> Value * { - if (match(P, m_Zero())) - return Constant::getNullValue(GEPTy); + auto PtrToInt = [GEPTy](Value *P) -> Value * { Value *Temp; if (match(P, m_PtrToInt(m_Value(Temp)))) if (Temp->getType() == GEPTy) @@ -4280,10 +4278,14 @@ static Value *SimplifyGEPInst(Type *SrcTy, ArrayRef Ops, return nullptr; }; + // FIXME: The following transforms are only legal if P and V have the + // same provenance (PR44403). Check whether getUnderlyingObject() is + // the same? + // getelementptr V, (sub P, V) -> P if P points to a type of size 1. if (TyAllocSize == 1 && match(Ops[1], m_Sub(m_Value(P), m_PtrToInt(m_Specific(Ops[0]))))) - if (Value *R = PtrToIntOrZero(P)) + if (Value *R = PtrToInt(P)) return R; // getelementptr V, (ashr (sub P, V), C) -> Q @@ -4292,7 +4294,7 @@ static Value *SimplifyGEPInst(Type *SrcTy, ArrayRef Ops, m_AShr(m_Sub(m_Value(P), m_PtrToInt(m_Specific(Ops[0]))), m_ConstantInt(C))) && TyAllocSize == 1ULL << C) - if (Value *R = PtrToIntOrZero(P)) + if (Value *R = PtrToInt(P)) return R; // getelementptr V, (sdiv (sub P, V), C) -> Q @@ -4300,7 +4302,7 @@ static Value *SimplifyGEPInst(Type *SrcTy, ArrayRef Ops, if (match(Ops[1], m_SDiv(m_Sub(m_Value(P), m_PtrToInt(m_Specific(Ops[0]))), m_SpecificInt(TyAllocSize)))) - if (Value *R = PtrToIntOrZero(P)) + if (Value *R = PtrToInt(P)) return R; } } @@ -4317,15 +4319,21 @@ static Value *SimplifyGEPInst(Type *SrcTy, ArrayRef Ops, Ops[0]->stripAndAccumulateInBoundsConstantOffsets(Q.DL, BasePtrOffset); + // Avoid creating inttoptr of zero here: While LLVMs treatment of + // inttoptr is generally conservative, this particular case is folded to + // a null pointer, which will have incorrect provenance. + // gep (gep V, C), (sub 0, V) -> C if (match(Ops.back(), - m_Sub(m_Zero(), m_PtrToInt(m_Specific(StrippedBasePtr))))) { + m_Sub(m_Zero(), m_PtrToInt(m_Specific(StrippedBasePtr)))) && + !BasePtrOffset.isNullValue()) { auto *CI = ConstantInt::get(GEPTy->getContext(), BasePtrOffset); return ConstantExpr::getIntToPtr(CI, GEPTy); } // gep (gep V, C), (xor V, -1) -> C-1 if (match(Ops.back(), - m_Xor(m_PtrToInt(m_Specific(StrippedBasePtr)), m_AllOnes()))) { + m_Xor(m_PtrToInt(m_Specific(StrippedBasePtr)), m_AllOnes())) && + !BasePtrOffset.isOneValue()) { auto *CI = ConstantInt::get(GEPTy->getContext(), BasePtrOffset - 1); return ConstantExpr::getIntToPtr(CI, GEPTy); } diff --git a/llvm/test/Transforms/InstSimplify/gep.ll b/llvm/test/Transforms/InstSimplify/gep.ll index e6670e4..8fff9e9 100644 --- a/llvm/test/Transforms/InstSimplify/gep.ll +++ b/llvm/test/Transforms/InstSimplify/gep.ll @@ -40,9 +40,16 @@ define i64* @test3(i64* %b, i64* %e) { ret i64* %gep } +; The following tests should not be folded to null, because this would +; lose provenance of the base pointer %b. + define %struct.A* @test4(%struct.A* %b) { ; CHECK-LABEL: @test4( -; CHECK-NEXT: ret %struct.A* null +; CHECK-NEXT: [[B_PTR:%.*]] = ptrtoint %struct.A* [[B:%.*]] to i64 +; CHECK-NEXT: [[SUB:%.*]] = sub i64 0, [[B_PTR]] +; CHECK-NEXT: [[SDIV:%.*]] = sdiv exact i64 [[SUB]], 7 +; CHECK-NEXT: [[GEP:%.*]] = getelementptr [[STRUCT_A:%.*]], %struct.A* [[B]], i64 [[SDIV]] +; CHECK-NEXT: ret %struct.A* [[GEP]] ; %b_ptr = ptrtoint %struct.A* %b to i64 %sub = sub i64 0, %b_ptr @@ -53,7 +60,11 @@ define %struct.A* @test4(%struct.A* %b) { define %struct.A* @test4_inbounds(%struct.A* %b) { ; CHECK-LABEL: @test4_inbounds( -; CHECK-NEXT: ret %struct.A* null +; CHECK-NEXT: [[B_PTR:%.*]] = ptrtoint %struct.A* [[B:%.*]] to i64 +; CHECK-NEXT: [[SUB:%.*]] = sub i64 0, [[B_PTR]] +; CHECK-NEXT: [[SDIV:%.*]] = sdiv exact i64 [[SUB]], 7 +; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds [[STRUCT_A:%.*]], %struct.A* [[B]], i64 [[SDIV]] +; CHECK-NEXT: ret %struct.A* [[GEP]] ; %b_ptr = ptrtoint %struct.A* %b to i64 %sub = sub i64 0, %b_ptr @@ -64,7 +75,10 @@ define %struct.A* @test4_inbounds(%struct.A* %b) { define i8* @test5(i8* %b) { ; CHECK-LABEL: @test5( -; CHECK-NEXT: ret i8* null +; CHECK-NEXT: [[B_PTR:%.*]] = ptrtoint i8* [[B:%.*]] to i64 +; CHECK-NEXT: [[SUB:%.*]] = sub i64 0, [[B_PTR]] +; CHECK-NEXT: [[GEP:%.*]] = getelementptr i8, i8* [[B]], i64 [[SUB]] +; CHECK-NEXT: ret i8* [[GEP]] ; %b_ptr = ptrtoint i8* %b to i64 %sub = sub i64 0, %b_ptr @@ -74,7 +88,10 @@ define i8* @test5(i8* %b) { define i8* @test5_inbounds(i8* %b) { ; CHECK-LABEL: @test5_inbounds( -; CHECK-NEXT: ret i8* null +; CHECK-NEXT: [[B_PTR:%.*]] = ptrtoint i8* [[B:%.*]] to i64 +; CHECK-NEXT: [[SUB:%.*]] = sub i64 0, [[B_PTR]] +; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds i8, i8* [[B]], i64 [[SUB]] +; CHECK-NEXT: ret i8* [[GEP]] ; %b_ptr = ptrtoint i8* %b to i64 %sub = sub i64 0, %b_ptr @@ -84,7 +101,11 @@ define i8* @test5_inbounds(i8* %b) { define i64* @test6(i64* %b) { ; CHECK-LABEL: @test6( -; CHECK-NEXT: ret i64* null +; CHECK-NEXT: [[B_PTR:%.*]] = ptrtoint i64* [[B:%.*]] to i64 +; CHECK-NEXT: [[SUB:%.*]] = sub i64 0, [[B_PTR]] +; CHECK-NEXT: [[ASHR:%.*]] = ashr exact i64 [[SUB]], 3 +; CHECK-NEXT: [[GEP:%.*]] = getelementptr i64, i64* [[B]], i64 [[ASHR]] +; CHECK-NEXT: ret i64* [[GEP]] ; %b_ptr = ptrtoint i64* %b to i64 %sub = sub i64 0, %b_ptr @@ -95,7 +116,11 @@ define i64* @test6(i64* %b) { define i64* @test6_inbounds(i64* %b) { ; CHECK-LABEL: @test6_inbounds( -; CHECK-NEXT: ret i64* null +; CHECK-NEXT: [[B_PTR:%.*]] = ptrtoint i64* [[B:%.*]] to i64 +; CHECK-NEXT: [[SUB:%.*]] = sub i64 0, [[B_PTR]] +; CHECK-NEXT: [[ASHR:%.*]] = ashr exact i64 [[SUB]], 3 +; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds i64, i64* [[B]], i64 [[ASHR]] +; CHECK-NEXT: ret i64* [[GEP]] ; %b_ptr = ptrtoint i64* %b to i64 %sub = sub i64 0, %b_ptr -- 2.7.4