[PATCH] D12169: Relax constexpr rules to improve __builtin_object_size's accuracy
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 31 19:47:16 PDT 2015
rsmith added inline comments.
================
Comment at: lib/AST/ExprConstant.cpp:479
@@ +478,3 @@
+ /// that may occur. The intent of this mode is to determine an LValue's
+ /// Offset, so things not ordinarily allowed in constexprs
+ /// (reinterpret_casts, OOB array indices, etc.) are allowed. As such, the
----------------
`constexpr` is a keyword and an adjective. The noun phrase you want here is "constant expressions".
================
Comment at: lib/AST/ExprConstant.cpp:4879-4907
@@ -4753,31 +4878,31 @@
bool Success(const APValue &V, const Expr *E) {
Result.setFrom(Info.Ctx, V);
return true;
}
bool ZeroInitialization(const Expr *E) {
return Success((Expr*)nullptr);
}
bool VisitBinaryOperator(const BinaryOperator *E);
bool VisitCastExpr(const CastExpr* E);
bool VisitUnaryAddrOf(const UnaryOperator *E);
bool VisitObjCStringLiteral(const ObjCStringLiteral *E)
{ return Success(E); }
bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
- { return Success(E); }
+ { return Success(E); }
bool VisitAddrLabelExpr(const AddrLabelExpr *E)
{ return Success(E); }
bool VisitCallExpr(const CallExpr *E);
bool VisitBlockExpr(const BlockExpr *E) {
if (!E->getBlockDecl()->hasCaptures())
return Success(E);
return Error(E);
}
bool VisitCXXThisExpr(const CXXThisExpr *E) {
// Can't look at 'this' when checking a potential constant expression.
if (Info.checkingPotentialConstantExpression())
return false;
if (!Info.CurrentCall->This) {
if (Info.getLangOpts().CPlusPlus11)
Info.Diag(E, diag::note_constexpr_this) << E->isImplicit();
----------------
> We need some way to support `BOS((char*)&Foo.Bar + 1, N)`
Why? This seems like a pretty canonical example of "we do not know which subobject is being referenced any more", and thus we should give up for types 1 and 3 here.
================
Comment at: lib/AST/ExprConstant.cpp:4879-4907
@@ -4753,31 +4878,31 @@
bool Success(const APValue &V, const Expr *E) {
Result.setFrom(Info.Ctx, V);
return true;
}
bool ZeroInitialization(const Expr *E) {
return Success((Expr*)nullptr);
}
bool VisitBinaryOperator(const BinaryOperator *E);
bool VisitCastExpr(const CastExpr* E);
bool VisitUnaryAddrOf(const UnaryOperator *E);
bool VisitObjCStringLiteral(const ObjCStringLiteral *E)
{ return Success(E); }
bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
- { return Success(E); }
+ { return Success(E); }
bool VisitAddrLabelExpr(const AddrLabelExpr *E)
{ return Success(E); }
bool VisitCallExpr(const CallExpr *E);
bool VisitBlockExpr(const BlockExpr *E) {
if (!E->getBlockDecl()->hasCaptures())
return Success(E);
return Error(E);
}
bool VisitCXXThisExpr(const CXXThisExpr *E) {
// Can't look at 'this' when checking a potential constant expression.
if (Info.checkingPotentialConstantExpression())
return false;
if (!Info.CurrentCall->This) {
if (Info.getLangOpts().CPlusPlus11)
Info.Diag(E, diag::note_constexpr_this) << E->isImplicit();
----------------
rsmith wrote:
> > We need some way to support `BOS((char*)&Foo.Bar + 1, N)`
>
> Why? This seems like a pretty canonical example of "we do not know which subobject is being referenced any more", and thus we should give up for types 1 and 3 here.
> [...] we have an interesting feature in the non-patched impl where `B *b = (B*)((A*)&b + 1)` is at an offset of `sizeof(A)`, but Designator says it's at `b[1]`
That certainly seems like a bug. We should have marked the designator as invalid at the pointer arithmetic because it wasn't a pointer to the most derived type. (Are you sure it wasn't your changes to `adjustIndex` that caused this?)
================
Comment at: lib/AST/ExprConstant.cpp:498-502
@@ -499,6 +497,7 @@
/// expression?
bool checkingPotentialConstantExpression() const {
return EvalMode == EM_PotentialConstantExpression ||
- EvalMode == EM_PotentialConstantExpressionUnevaluated;
+ EvalMode == EM_PotentialConstantExpressionUnevaluated ||
+ EvalMode == EM_OffsetFold;
}
----------------
I think you should only have one mode here, and it should not be a "checking potential constant expression" mode. (Then, if evaluation of a `__builtin_object_size` call fails from a "checking potential constant expression" mode, we should treat it as a potential success -- that is, return false with no diagnostic.)
It looks like the only place this would go wrong is the assertion in `CheckLValueConstantExpression`, but you can update that assertion to also allow this one case.
================
Comment at: lib/AST/ExprConstant.cpp:866
@@ +865,3 @@
+ Entries.back().ArrayIndex += N;
+ if (!Info.allowOutOfBoundsIndices() &&
+ Entries.back().ArrayIndex > MostDerivedArraySize) {
----------------
I'm not convinced this is a good idea. There are two cases:
1) We're looking for the complete object (type is 0 or 2). We don't need this change: the right thing to do is discard the subobject designator and just track the offset, which is what we'd do anyway and all we need.
2) We're looking for the specified subobject (type is 1 or 3). This change seems wrong: we no longer know which the specified subobject is, because the code computed an offset that left the subobject we thought we were in.
================
Comment at: lib/AST/ExprConstant.cpp:1983-1986
@@ +1982,6 @@
+
+ // If we allow offsets that aren't on object boundaries, we need to take
+ // into account any additional offsets that aren't fully accounted for
+ // (already) by indices
+ if (Info.allowNonObjectBoundaryOffsets()) {
+ QualType ArrayType = LVal.Designator.MostDerivedType;
----------------
Same comment as for the previous case: I don't think this is a good idea. We can track the offset in this case, but we can't reasonably keep the designator valid.
================
Comment at: lib/AST/ExprConstant.cpp:4003-4008
@@ -3905,2 +4002,8 @@
}
+ // Called when we couldn't evaluate the LValue Base of a member expression,
+ // but the members could be visited properly.
+ bool DerivedInvalidBase(const APValue &V, const Expr *E) {
+ assert(Info.allowInvalidBaseExpr() && "This shouldn't be allowed");
+ return static_cast<Derived*>(this)->InvalidBase(V, E);
+ }
----------------
This doesn't make much sense in `ExprEvaluatorBase`. I think you should only need the change to `LValueExprEvaluatorBase::VisitMemberExpr` -- it's only lvalue member expressions that should get this behavior.
================
Comment at: lib/AST/ExprConstant.cpp:4626-4632
@@ -4625,9 +4752,7 @@
bool LValueExprEvaluator::VisitMemberExpr(const MemberExpr *E) {
- // Handle static data members.
if (const VarDecl *VD = dyn_cast<VarDecl>(E->getMemberDecl())) {
VisitIgnoredValue(E->getBase());
return VisitVarDecl(E, VD);
}
- // Handle static member functions.
if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(E->getMemberDecl())) {
----------------
Why do you hate comments? :)
================
Comment at: lib/AST/ExprConstant.cpp:4980-4987
@@ -4848,10 +4979,10 @@
// also static_casts, but we disallow them as a resolution to DR1312.
- if (!E->getType()->isVoidPointerType()) {
+ if (!Info.allowReinterpretCasts() && !E->getType()->isVoidPointerType()) {
Result.Designator.setInvalid();
if (SubExpr->getType()->isVoidPointerType())
CCEDiag(E, diag::note_constexpr_invalid_cast)
<< 3 << SubExpr->getType();
else
CCEDiag(E, diag::note_constexpr_invalid_cast) << 2;
}
return true;
----------------
You need to set the designator to invalid here, or you break the invariant of the `LValue` and further calculations on it will result in wrong offsets (or maybe assertions). As with the other two cases, I don't think it's reasonable to claim we can track the subobject once this has happened.
================
Comment at: lib/AST/ExprConstant.cpp:6299
@@ -6167,3 +6298,3 @@
/// as used by __builtin_object_size.
-static QualType getObjectType(APValue::LValueBase B) {
+static QualType GetObjectType(APValue::LValueBase B) {
if (const ValueDecl *D = B.dyn_cast<const ValueDecl*>()) {
----------------
Lowercase first letter for function names is preferred in new code.
================
Comment at: lib/AST/ExprConstant.cpp:6331-6336
@@ -6200,8 +6330,8 @@
// MostDerivedType is null if we're dealing with a literal such as nullptr or
// (char*)0x100000. Lower it to LLVM in either case so it can figure out what
// to do with it.
// FIXME(gbiv): Try to do a better job with this in clang.
if (Base.Designator.MostDerivedType.isNull())
return Error(E);
----------------
Not really related to the current patch, but this check seems wrong for the `(Type & 1) == 0` case: if the designator is invalid, we don't care what `MostDerivedType` is, we only care about the type of the `Base` and the `BaseOffset` (and now, having a valid base).
================
Comment at: lib/AST/ExprConstant.cpp:6403
@@ -6264,1 +6402,3 @@
+ EndOffset += SizeOfPointee * AmountToAdd;
+ EndOffset -= CharUnits::fromQuantity(EndOffset % SizeOfPointee);
if (BaseOffset > EndOffset)
----------------
We're supposed to return the number of usable bytes; I don't see why we should care if we can't fit an integral number of the pointee type in those bytes.
http://reviews.llvm.org/D12169
More information about the cfe-commits
mailing list