[PATCH] D12169: Relax constexpr rules to improve __builtin_object_size's accuracy
George Burgess IV via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 4 14:31:23 PDT 2015
george.burgess.iv closed this revision.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
r246877. Thanks for the review!
(Also: Forgot to submit comments with the most recent revision, so you're getting them all now. Sorry. :) )
================
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;
}
----------------
rsmith wrote:
> 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.
Migrated to EM_ConstantFold (if Type=0|2) or EM_ConstantExpressionConstantFold (if Type=1|3)
================
Comment at: lib/AST/ExprConstant.cpp:866
@@ +865,3 @@
+ Entries.back().ArrayIndex += N;
+ if (!Info.allowOutOfBoundsIndices() &&
+ Entries.back().ArrayIndex > MostDerivedArraySize) {
----------------
rsmith wrote:
> 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.
SGTM. Backed it out.
================
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())) {
----------------
rsmith wrote:
> Why do you hate comments? :)
They were far too concise and up-to-date for my taste.
================
Comment at: lib/AST/ExprConstant.cpp:4879-4913
@@ -4753,31 +4878,37 @@
bool Success(const APValue &V, const Expr *E) {
Result.setFrom(Info.Ctx, V);
return true;
}
+
+ bool InvalidBase(const APValue &V, const Expr *E) {
+ Result.setInvalid(E);
+ 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();
----------------
> 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.
SGTM.
> That certainly seems like a bug...
Discussed in person. Was a misunderstanding on my part.
================
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*>()) {
----------------
rsmith wrote:
> Lowercase first letter for function names is preferred in new code.
Thanks for the heads-up :)
================
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);
----------------
rsmith wrote:
> 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).
We now no longer directly deal with this case, so I'm closing this.
================
Comment at: lib/AST/ExprConstant.cpp:6403
@@ -6264,1 +6402,3 @@
+ EndOffset += SizeOfPointee * AmountToAdd;
+ EndOffset -= CharUnits::fromQuantity(EndOffset % SizeOfPointee);
if (BaseOffset > EndOffset)
----------------
rsmith wrote:
> 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.
Note: Code is gone, so this no longer applies. Original response below
The only reason we care is because of how we compute the end offset. Currently, we do `EndOffset = SizeOfPointee * NumberOfArraySlotsUntilTheEnd`. This works fine when we're on an object boundary, but when we're not, the array index = `floor(OffsetFromArrayStart / SizeOfPointee)`. So, without this line, EndOffset = 11 in:
```
int16_t shorts[5];
__builtin_object_size((char*)&shorts + 1, 1);
```
I'm happy to phrase it differently in the code, but if we decide to allow off-object-boundary offsets, then we need to find some way to support it.
================
Comment at: lib/AST/ExprConstant.cpp:499
@@ -496,1 +498,3 @@
+ /// MemberExpr with a base that can't be evaluated.
+ EM_ConstantExpressionOffsetFold,
} EvalMode;
----------------
rsmith wrote:
> Maybe rename to `OffsetFold` or `DesignatorFold`?
DesignatorFold it is -- thanks for the suggestions.
================
Comment at: lib/AST/ExprConstant.cpp:6263-6264
@@ +6262,4 @@
+ auto *SubExpr = Cast->getSubExpr();
+ if (!SubExpr->getType()->hasPointerRepresentation() || !SubExpr->isRValue())
+ return NoParens;
+ return ignorePointerCastsAndParens(SubExpr);
----------------
rsmith wrote:
> I don't think this is quite right: you should only skip past casts that don't change the pointer value. In particular, this check will step past derived-to-base pointer conversions, which may require an adjustment to the pointer.
Thanks for catching that -- I really need to get to know C++ better :)
Updated `if (Cast == nullptr)` to `if (Cast == nullptr || Cast->getCastKind() == CK_DerivedToBase)`.
http://reviews.llvm.org/D12169
More information about the cfe-commits
mailing list