This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Handle opaque types in isDereferenceableAndAlignedPointer.
ClosedPublic

Authored by mzolotukhin on Dec 16 2015, 4:02 PM.

Details

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?

Diff Detail

Repository
rL LLVM

Event Timeline

mzolotukhin retitled this revision from to [ValueTracking] Handle opaque types in isDereferenceableAndAlignedPointer..
mzolotukhin updated this object.
mzolotukhin added a subscriber: llvm-commits.
reames added inline comments.Dec 16 2015, 4:39 PM
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 added inline comments.Dec 16 2015, 4:51 PM
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 added inline comments.Dec 16 2015, 5:02 PM
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.

  • Check 'isSized' instead of 'isOpaque'.
mzolotukhin added inline comments.Dec 16 2015, 5:26 PM
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 added inline comments.Dec 17 2015, 5:48 AM
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 added inline comments.Dec 17 2015, 10:05 AM
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 added a subscriber: Gerolf.Dec 17 2015, 11:21 AM

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.

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 edited edge metadata.Dec 18 2015, 6:24 AM

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 edited edge metadata.
  • Sink 'isSized' to 'isAligned'.

Hi Artur,

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

Thanks,
Michael

apilipenko accepted this revision.Dec 21 2015, 3:38 AM
apilipenko edited edge metadata.

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.

This revision is now accepted and ready to land.Dec 21 2015, 3:38 AM
This revision was automatically updated to reflect the committed changes.

Thanks, committed in r256192 with the requested change!

Michael