[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 ↗

(On Diff #43081)

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 ↗

(On Diff #43081)

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 ↗

(On Diff #43081)

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 ↗

(On Diff #43090)

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 ↗

(On Diff #43090)

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 ↗

(On Diff #43090)

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 ↗

(On Diff #43090)

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 ↗

(On Diff #43090)

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 ↗

(On Diff #43090)

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 ↗

(On Diff #43090)

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 ↗

(On Diff #43090)

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 ↗

(On Diff #43090)

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

3195–3196 ↗

(On Diff #43255)

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

llvm/trunk/lib/Analysis/ValueTracking.cpp

 }
 
 static bool isAligned(const Value *Base, APInt Offset, unsigned Align,
                       const DataLayout &DL) {
   APInt BaseAlign(Offset.getBitWidth(), getAlignment(Base, DL));
 
   if (!BaseAlign) {
     Type *Ty = Base->getType()->getPointerElementType();
+    if (!Ty->isSized())
+      return false;
     BaseAlign = DL.getABITypeAlignment(Ty);
   }
 
   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);
+  Type *Ty = Base->getType();
+  assert(Ty->isSized() && "must be sized");
+  APInt Offset(DL.getTypeStoreSizeInBits(Ty), 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,

llvm/trunk/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 = !{}