[ValueTracking] Handle opaque types in isDereferenceableAndAlignedPointer.

[mzolotukhin]

Summary

While we do check that type Ty is sized, an opaque type still can slip thru
this check to isAligned function and hit an assertion there. The fix is to
check type of value after stripping constant offsets. Does it look good?

[reames]

lib/Analysis/ValueTracking.cpp

3302–3303

Why are opaque types considered sized? If they're opaque and we don't yet have a body, how can they be sized? That seems like the root of the problem here.

[mzolotukhin]

lib/Analysis/ValueTracking.cpp

3302–3303

The type of V isn't sized and isn't opaque. However, the type of BV is opaque, and we don't check if it's sized.

I could replace the new check with isSized(), that might be better.

[reames]

lib/Analysis/ValueTracking.cpp

3302–3303

The isSized check would be a lot more general.

I really wonder though if the isSized and isNonNegative checks shouldn't be pushed inside the two callees though.

[mzolotukhin]

• Check 'isSized' instead of 'isOpaque'.

[mzolotukhin]

lib/Analysis/ValueTracking.cpp

3302–3303

I updated the check to isSized.

I'm not well familiar with this code, so I don't have strong opinion on whether these checks are worth pulling in or not. It seems useful though.

[apilipenko]

lib/Analysis/ValueTracking.cpp

3302–3303

I don't think that you have to sink isNonNegative check because it's checking a contract of stripAndAccumulateInBoundsConstantOffsets function. But it makes sense to sink isSized check to isDereferenceableFromAttribute.

[mzolotukhin]

lib/Analysis/ValueTracking.cpp

3302–3303

The issue is with isAligned, not with isDereferenceableFromAttribute.

isDereferenceableFromAttribute (ValueTracking.cpp:3118) has an assert

assert(Ty->isSized() && "must be sized");

But we pass Ty there, not BV->getType()->getPointerElementType(), that's why the assert doesn't fire.

isAligned doesn't have a check or an assert, and we don't pass a type there, so it derives the type from the value we pass (BV):

Type *Ty = Base->getType()->getPointerElementType();
BaseAlign = DL.getABITypeAlignment(Ty);

This type Ty happens to be opaque, and somewhere in getABITypeAlignment we crash.

Will it make sense to add the isSized check to isAligned?

[Gerolf]

Caveat: I'm not familiar with this code either. It would be great if someone could double check removing the Ty parameter from isDeferenceableFromAttribute does not cause problems. But I would be really surprised if Ty didn't have to match BV's type....

lib/Analysis/ValueTracking.cpp

3296

if Ty were opaque here wouldn't there be an assertion in that call already? I guess I don't see why a non-opaque type is guaranteed here. The safe fix would be to check for isSized(Ty) here as well. Since this is orthogonal to the current assert a FIXME should do for now.

3304

The call to isDeferenceableFromAttribute does not look correct. Should Ty match BV's type. I think the invocation without the Ty parameter is the one needed here. Accidentally this would fix the assertion bug, too, since the subsequent call to isAligned would not happen.

3305

I think the isAligned code is not correct either. The following should be changed to
03178 APInt BaseAlign(Offset.getBitWidth(), getAlignment(Base, DL));
03179
03180 if (!BaseAlign) {
03181 Type *Ty = Base->getType()->getPointerElementType();
03182 BaseAlign = DL.getABITypeAlignment(Ty);
03183 }
Type *Ty = Base->getTyoe()->getPointerElementType();
if (!isSized(Ty)) return false;
APInt ...
But again, just to fix the assertion, this is not necessary. A FIXME would do for now.

[mzolotukhin]

Hi Philip, Artur, Gerolf,

Thanks for the helpful replies, please find my comments inline.

Michael

lib/Analysis/ValueTracking.cpp

3304

If I'm not mistaken, the invocation without the Ty parameter goes without the Offset parameter as well, which is required in this case. So, we can't use another version of isDeferenceableFromAttribute here.

That said, the case when BV's type doesn't match V's type is indeed a special case - should we maybe just bailout in this case, or is it expected, and we know how to properly handle it?

3305

The problem with fixing it in isAligned is that we have two versions of that function:

static bool isAligned(const Value *Base, unsigned Align, const DataLayout &DL)

and

static bool isAligned(const Value *Base, APInt Offset, unsigned Align, const DataLayout &DL)

The first calls the second, both of them need the check, and in our case we call directly the second one.
Thus, if we sink isSized check to these functions, we'll do this check twice if the first version is called. Considering this, I think the best fix is to check isSized before calling isAligned, as we do in the current patch. Does it sound right?

[apilipenko]

isAligned indeed requires base pointer to be sized. You either make it a contract of the function and add an assertion in isAligned, or support non-sized base pointers and conservatively return false for them. I'd prefer the latter even if it results in two checks for one of the isAligned versions. Alternatively you will need to verify that any other place where isAligned is called base pointer can't be non-sized.

lib/Analysis/ValueTracking.cpp

3304

Ty other than BV type is a perfectly correct and supported case. You might reference to a field of a struct for example. In this case you will have a struct as a base value (BV) and a field type as Ty.

[mzolotukhin]

• Sink 'isSized' to 'isAligned'.

[mzolotukhin]

Hi Artur,

That makes sense to me, I sank the isSized checks to isAligned routines. Is the patch good to commit?

Thanks,
Michael

[apilipenko]

LGTM with one check converted to an assertion.

lib/Analysis/ValueTracking.cpp

3193–3194

There is a difference in this check and the check in the other isAligned function. Here you check that the Base type is sized. This function should be called for pointer only. Pointers are always sized, so you just need to assert that Base is a pointer.

In the other function you check that pointer element type is sized, which is not guaranteed, so the check is required.

[mzolotukhin]

Thanks, committed in r256192 with the requested change!

Michael

lib/Analysis/ValueTracking.cpp

static bool isAligned(const Value *Base, APInt Offset, unsigned Align,
   APInt Alignment(Offset.getBitWidth(), Align);
 
   assert(Alignment.isPowerOf2() && "must be a power of 2!");
   return BaseAlign.uge(Alignment) && !(Offset & (Alignment-1));
 }
 
 static bool isAligned(const Value *Base, unsigned Align, const DataLayout &DL) {
   APInt Offset(DL.getTypeStoreSizeInBits(Base->getType()), 0);
   return isAligned(Base, Offset, Align, DL);
 }
 
 /// Test if V is always a pointer to allocated and suitably aligned memory for
 /// a simple load or store.
 static bool isDereferenceableAndAlignedPointer(
     const Value *V, unsigned Align, const DataLayout &DL,
     const Instruction *CtxI, const DominatorTree *DT,
     const TargetLibraryInfo *TLI, SmallPtrSetImpl<const Value *> &Visited) {
   // Note that it is not safe to speculate into a malloc'd region because
bool llvm::isDereferenceableAndAlignedPointer(const Value *V, unsigned Align,
   // attribute, we know exactly how many bytes are dereferenceable. If we can
   // determine the exact offset to the attributed variable, we can use that
   // information here.
   Type *VTy = V->getType();
   Type *Ty = VTy->getPointerElementType();
 
   // Require ABI alignment for loads without alignment specification
   if (Align == 0)
     Align = DL.getABITypeAlignment(Ty);
 
   if (Ty->isSized()) {
     APInt Offset(DL.getTypeStoreSizeInBits(VTy), 0);
     const Value *BV = V->stripAndAccumulateInBoundsConstantOffsets(DL, Offset);
 
-    if (Offset.isNonNegative())
+    if (BV->getType()->getPointerElementType()->isSized() &&
+        Offset.isNonNegative())
       if (isDereferenceableFromAttribute(BV, Offset, Ty, DL, CtxI, DT, TLI) &&
           isAligned(BV, Offset, Align, DL))
         return true;
   }
 
   SmallPtrSet<const Value *, 32> Visited;
   return ::isDereferenceableAndAlignedPointer(V, Align, DL, CtxI, DT, TLI,
                                               Visited);
 }
 

test/Analysis/ValueTracking/memory-dereferenceable.ll

 ; RUN: opt -print-memderefs -analyze -S <%s | FileCheck %s
 
 ; Uses the print-deref (+ analyze to print) pass to run
 ; isDereferenceablePointer() on many load instruction operands
 
 target datalayout = "e"
 
+%TypeOpaque = type opaque
+
 declare zeroext i1 @return_i1()
 
 declare i32* @foo()
 @globalstr = global [6 x i8] c"hello\00"
 @globali32ptr = external global i32*
 
 %struct.A = type { [8 x i8], [5 x i8] }
 @globalstruct = external global %struct.A
 
 @globalptr.align1 = external global i8, align 1
 @globalptr.align16 = external global i8, align 16
 
+; CHECK-LABEL: 'test'
 define void @test(i32 addrspace(1)* dereferenceable(8) %dparam,
                   i8 addrspace(1)* dereferenceable(32) align 1 %dparam.align1,
                   i8 addrspace(1)* dereferenceable(32) align 16 %dparam.align16)
     gc "statepoint-example" {
 ; CHECK: The following are dereferenceable:
 entry:
 ; CHECK: %globalptr{{.*}}(aligned)
     %globalptr = getelementptr inbounds [6 x i8], [6 x i8]* @globalstr, i32 0, i32 0
; CHECK: %d4_aligned_load{{.*}}(aligned)
     %d4_unaligned_load = load i32*, i32** @globali32ptr, !dereferenceable !0
     %d4_aligned_load = load i32*, i32** @globali32ptr, !dereferenceable !0, !align !{i64 16}
     %load26 = load i32, i32* %d4_unaligned_load, align 16
     %load27 = load i32, i32* %d4_aligned_load, align 16
 
     ret void
 }
 
+; Just check that we don't crash.
+; CHECK-LABEL: 'opaque_type_crasher'
+define void @opaque_type_crasher(%TypeOpaque* dereferenceable(16) %a) {
+entry:
+  %bc = bitcast %TypeOpaque* %a to i8*
+  %ptr8 = getelementptr inbounds i8, i8* %bc, i32 8
+  %ptr32 = bitcast i8* %ptr8 to i32*
+  br i1 undef, label %if.then, label %if.end
+
+if.then:
+  %res = load i32, i32* %ptr32, align 4
+  br label %if.end
+
+if.end:
+  ret void
+}
+
 declare i32 @llvm.experimental.gc.statepoint.p0f_i1f(i64, i32, i1 ()*, i32, i32, ...)
 declare i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32, i32, i32)
 
 !0 = !{i64 4}
 !1 = !{i64 2}
 !2 = !{}