r293595 - Improve fix for PR28739
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Mon Jan 30 18:23:02 PST 2017
Author: rsmith
Date: Mon Jan 30 20:23:02 2017
New Revision: 293595
URL: http://llvm.org/viewvc/llvm-project?rev=293595&view=rev
Log:
Improve fix for PR28739
Don't try to map an APSInt addend to an int64_t in pointer arithmetic before
bounds-checking it. This gives more consistent behavior (outside C++11, we
consistently use 2s complement semantics for both pointer and integer overflow
in constant expressions) and fixes some cases where in C++11 we would fail to
properly check for out-of-bounds pointer arithmetic (if the 2s complement
64-bit overflow landed us back in-bounds).
In passing, also fix some cases where we'd perform possibly-overflowing
arithmetic on CharUnits (which have a signed underlying type) during constant
expression evaluation.
Modified:
cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/test/Sema/const-eval.c
cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td?rev=293595&r1=293594&r2=293595&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td Mon Jan 30 20:23:02 2017
@@ -44,7 +44,8 @@ def note_constexpr_non_global : Note<
def note_constexpr_uninitialized : Note<
"%select{|sub}0object of type %1 is not initialized">;
def note_constexpr_array_index : Note<"cannot refer to element %0 of "
- "%select{array of %2 elements|non-array object}1 in a constant expression">;
+ "%select{array of %2 element%plural{1:|:s}2|non-array object}1 "
+ "in a constant expression">;
def note_constexpr_float_arithmetic : Note<
"floating point arithmetic produces %select{an infinity|a NaN}0">;
def note_constexpr_pointer_subtraction_not_same_array : Note<
Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=293595&r1=293594&r2=293595&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Mon Jan 30 20:23:02 2017
@@ -350,36 +350,48 @@ namespace {
MostDerivedArraySize = 2;
MostDerivedPathLength = Entries.size();
}
- void diagnosePointerArithmetic(EvalInfo &Info, const Expr *E, uint64_t N);
+ void diagnosePointerArithmetic(EvalInfo &Info, const Expr *E, APSInt N);
/// Add N to the address of this subobject.
- void adjustIndex(EvalInfo &Info, const Expr *E, uint64_t N) {
- if (Invalid) return;
+ void adjustIndex(EvalInfo &Info, const Expr *E, APSInt N) {
+ if (Invalid || !N) return;
+ uint64_t TruncatedN = N.extOrTrunc(64).getZExtValue();
if (isMostDerivedAnUnsizedArray()) {
// Can't verify -- trust that the user is doing the right thing (or if
// not, trust that the caller will catch the bad behavior).
- Entries.back().ArrayIndex += N;
- return;
- }
- if (MostDerivedPathLength == Entries.size() &&
- MostDerivedIsArrayElement) {
- Entries.back().ArrayIndex += N;
- if (Entries.back().ArrayIndex > getMostDerivedArraySize()) {
- diagnosePointerArithmetic(Info, E, Entries.back().ArrayIndex);
- setInvalid();
- }
+ // FIXME: Should we reject if this overflows, at least?
+ Entries.back().ArrayIndex += TruncatedN;
return;
}
+
// [expr.add]p4: For the purposes of these operators, a pointer to a
// nonarray object behaves the same as a pointer to the first element of
// an array of length one with the type of the object as its element type.
- if (IsOnePastTheEnd && N == (uint64_t)-1)
- IsOnePastTheEnd = false;
- else if (!IsOnePastTheEnd && N == 1)
- IsOnePastTheEnd = true;
- else if (N != 0) {
- diagnosePointerArithmetic(Info, E, uint64_t(IsOnePastTheEnd) + N);
+ bool IsArray = MostDerivedPathLength == Entries.size() &&
+ MostDerivedIsArrayElement;
+ uint64_t ArrayIndex =
+ IsArray ? Entries.back().ArrayIndex : (uint64_t)IsOnePastTheEnd;
+ uint64_t ArraySize =
+ IsArray ? getMostDerivedArraySize() : (uint64_t)1;
+
+ if (N < -(int64_t)ArrayIndex || N > ArraySize - ArrayIndex) {
+ // Calculate the actual index in a wide enough type, so we can include
+ // it in the note.
+ N = N.extend(std::max<unsigned>(N.getBitWidth() + 1, 65));
+ (llvm::APInt&)N += ArrayIndex;
+ assert(N.ugt(ArraySize) && "bounds check failed for in-bounds index");
+ diagnosePointerArithmetic(Info, E, N);
setInvalid();
+ return;
}
+
+ ArrayIndex += TruncatedN;
+ assert(ArrayIndex <= ArraySize &&
+ "bounds check succeeded for out-of-bounds index");
+
+ if (IsArray)
+ Entries.back().ArrayIndex = ArrayIndex;
+ else
+ IsOnePastTheEnd = (ArrayIndex != 0);
}
};
@@ -1050,16 +1062,16 @@ bool SubobjectDesignator::checkSubobject
}
void SubobjectDesignator::diagnosePointerArithmetic(EvalInfo &Info,
- const Expr *E, uint64_t N) {
+ const Expr *E, APSInt N) {
// If we're complaining, we must be able to statically determine the size of
// the most derived array.
if (MostDerivedPathLength == Entries.size() && MostDerivedIsArrayElement)
Info.CCEDiag(E, diag::note_constexpr_array_index)
- << static_cast<int>(N) << /*array*/ 0
+ << N << /*array*/ 0
<< static_cast<unsigned>(getMostDerivedArraySize());
else
Info.CCEDiag(E, diag::note_constexpr_array_index)
- << static_cast<int>(N) << /*non-array*/ 1;
+ << N << /*non-array*/ 1;
setInvalid();
}
@@ -1275,14 +1287,24 @@ namespace {
void clearIsNullPointer() {
IsNullPtr = false;
}
- void adjustOffsetAndIndex(EvalInfo &Info, const Expr *E, uint64_t Index,
+ void adjustOffsetAndIndex(EvalInfo &Info, const Expr *E, APSInt Index,
CharUnits ElementSize) {
- // Compute the new offset in the appropriate width.
- Offset += Index * ElementSize;
- if (Index && checkNullPointer(Info, E, CSK_ArrayIndex))
+ // An index of 0 has no effect. (In C, adding 0 to a null pointer is UB,
+ // but we're not required to diagnose it and it's valid in C++.)
+ if (!Index)
+ return;
+
+ // Compute the new offset in the appropriate width, wrapping at 64 bits.
+ // FIXME: When compiling for a 32-bit target, we should use 32-bit
+ // offsets.
+ uint64_t Offset64 = Offset.getQuantity();
+ uint64_t ElemSize64 = ElementSize.getQuantity();
+ uint64_t Index64 = Index.extOrTrunc(64).getZExtValue();
+ Offset = CharUnits::fromQuantity(Offset64 + ElemSize64 * Index64);
+
+ if (checkNullPointer(Info, E, CSK_ArrayIndex))
Designator.adjustIndex(Info, E, Index);
- if (Index)
- clearIsNullPointer();
+ clearIsNullPointer();
}
void adjustOffset(CharUnits N) {
Offset += N;
@@ -1411,6 +1433,16 @@ static bool EvaluateAsRValue(EvalInfo &I
// Misc utilities
//===----------------------------------------------------------------------===//
+/// Negate an APSInt in place, converting it to a signed form if necessary, and
+/// preserving its value (by extending by up to one bit as needed).
+static void negateAsSigned(APSInt &Int) {
+ if (Int.isUnsigned() || Int.isMinSignedValue()) {
+ Int = Int.extend(Int.getBitWidth() + 1);
+ Int.setIsSigned(true);
+ }
+ Int = -Int;
+}
+
/// Produce a string describing the given constexpr call.
static void describeCall(CallStackFrame *Frame, raw_ostream &Out) {
unsigned ArgIndex = 0;
@@ -1458,21 +1490,6 @@ static bool EvaluateIgnoredValue(EvalInf
return true;
}
-/// Sign- or zero-extend a value to 64 bits. If it's already 64 bits, just
-/// return its existing value.
-static bool getExtValue(EvalInfo &Info, const Expr *E, const APSInt &Value,
- int64_t &Result) {
- if (Value.isSigned() ? Value.getMinSignedBits() > 64
- : Value.getActiveBits() > 64) {
- Info.FFDiag(E);
- return false;
- }
-
- Result = Value.isSigned() ? Value.getSExtValue()
- : static_cast<int64_t>(Value.getZExtValue());
- return true;
-}
-
/// Should this call expression be treated as a string literal?
static bool IsStringLiteralCall(const CallExpr *E) {
unsigned Builtin = E->getBuiltinCallee();
@@ -2228,7 +2245,7 @@ static bool HandleSizeof(EvalInfo &Info,
/// \param Adjustment - The adjustment, in objects of type EltTy, to add.
static bool HandleLValueArrayAdjustment(EvalInfo &Info, const Expr *E,
LValue &LVal, QualType EltTy,
- int64_t Adjustment) {
+ APSInt Adjustment) {
CharUnits SizeOfPointee;
if (!HandleSizeof(Info, E->getExprLoc(), EltTy, SizeOfPointee))
return false;
@@ -2237,6 +2254,13 @@ static bool HandleLValueArrayAdjustment(
return true;
}
+static bool HandleLValueArrayAdjustment(EvalInfo &Info, const Expr *E,
+ LValue &LVal, QualType EltTy,
+ int64_t Adjustment) {
+ return HandleLValueArrayAdjustment(Info, E, LVal, EltTy,
+ APSInt::get(Adjustment));
+}
+
/// Update an lvalue to refer to a component of a complex number.
/// \param Info - Information about the ongoing evaluation.
/// \param LVal - The lvalue to be updated.
@@ -3192,11 +3216,9 @@ struct CompoundAssignSubobjectHandler {
return false;
}
- int64_t Offset;
- if (!getExtValue(Info, E, RHS.getInt(), Offset))
- return false;
+ APSInt Offset = RHS.getInt();
if (Opcode == BO_Sub)
- Offset = -Offset;
+ negateAsSigned(Offset);
LValue LVal;
LVal.setFrom(Info.Ctx, Subobj);
@@ -5158,10 +5180,7 @@ bool LValueExprEvaluator::VisitArraySubs
if (!EvaluateInteger(E->getIdx(), Index, Info))
return false;
- int64_t Offset;
- if (!getExtValue(Info, E, Index, Offset))
- return false;
- return HandleLValueArrayAdjustment(Info, E, Result, E->getType(), Offset);
+ return HandleLValueArrayAdjustment(Info, E, Result, E->getType(), Index);
}
bool LValueExprEvaluator::VisitUnaryDeref(const UnaryOperator *E) {
@@ -5427,15 +5446,11 @@ bool PointerExprEvaluator::VisitBinaryOp
if (!EvaluateInteger(IExp, Offset, Info) || !EvalPtrOK)
return false;
- int64_t AdditionalOffset;
- if (!getExtValue(Info, E, Offset, AdditionalOffset))
- return false;
if (E->getOpcode() == BO_Sub)
- AdditionalOffset = -AdditionalOffset;
+ negateAsSigned(Offset);
QualType Pointee = PExp->getType()->castAs<PointerType>()->getPointeeType();
- return HandleLValueArrayAdjustment(Info, E, Result, Pointee,
- AdditionalOffset);
+ return HandleLValueArrayAdjustment(Info, E, Result, Pointee, Offset);
}
bool PointerExprEvaluator::VisitUnaryAddrOf(const UnaryOperator *E) {
@@ -7931,6 +7946,18 @@ bool DataRecursiveIntBinOpEvaluator::
return true;
}
+static void addOrSubLValueAsInteger(APValue &LVal, APSInt Index, bool IsSub) {
+ // Compute the new offset in the appropriate width, wrapping at 64 bits.
+ // FIXME: When compiling for a 32-bit target, we should use 32-bit
+ // offsets.
+ assert(!LVal.hasLValuePath() && "have designator for integer lvalue");
+ CharUnits &Offset = LVal.getLValueOffset();
+ uint64_t Offset64 = Offset.getQuantity();
+ uint64_t Index64 = Index.extOrTrunc(64).getZExtValue();
+ Offset = CharUnits::fromQuantity(IsSub ? Offset64 - Index64
+ : Offset64 + Index64);
+}
+
bool DataRecursiveIntBinOpEvaluator::
VisitBinOp(const EvalResult &LHSResult, const EvalResult &RHSResult,
const BinaryOperator *E, APValue &Result) {
@@ -7977,13 +8004,7 @@ bool DataRecursiveIntBinOpEvaluator::
// Handle cases like (unsigned long)&a + 4.
if (E->isAdditiveOp() && LHSVal.isLValue() && RHSVal.isInt()) {
Result = LHSVal;
- int64_t Offset;
- if (!getExtValue(Info, E, RHSVal.getInt(), Offset))
- return false;
- if (E->getOpcode() == BO_Add)
- Result.getLValueOffset() += CharUnits::fromQuantity(Offset);
- else
- Result.getLValueOffset() -= CharUnits::fromQuantity(Offset);
+ addOrSubLValueAsInteger(Result, RHSVal.getInt(), E->getOpcode() == BO_Sub);
return true;
}
@@ -7991,10 +8012,7 @@ bool DataRecursiveIntBinOpEvaluator::
if (E->getOpcode() == BO_Add &&
RHSVal.isLValue() && LHSVal.isInt()) {
Result = RHSVal;
- int64_t Offset;
- if (!getExtValue(Info, E, LHSVal.getInt(), Offset))
- return false;
- Result.getLValueOffset() += CharUnits::fromQuantity(Offset);
+ addOrSubLValueAsInteger(Result, LHSVal.getInt(), /*IsSub*/false);
return true;
}
Modified: cfe/trunk/test/Sema/const-eval.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/const-eval.c?rev=293595&r1=293594&r2=293595&view=diff
==============================================================================
--- cfe/trunk/test/Sema/const-eval.c (original)
+++ cfe/trunk/test/Sema/const-eval.c Mon Jan 30 20:23:02 2017
@@ -138,14 +138,9 @@ void PR24622();
struct PR24622 {} pr24622;
EVAL_EXPR(52, &pr24622 == (void *)&PR24622); // expected-error {{must have a constant size}}
-// Reject cases that would require more than 64 bits of pointer offset to
-// represent.
-// FIXME: We don't check for all offset overflow, just immediate adjustments
-// that don't fit in 64 bits. We should consistently either check all offset
-// adjustments or truncate to 64 bits everywhere.
-void *PR28739a = (__int128)(unsigned long)-1 + &PR28739a; // expected-error {{not a compile-time constant}}
-void *PR28739b = &PR28739b + (__int128)(unsigned long)-1; // expected-error {{not a compile-time constant}}
-__int128 PR28739c = (&PR28739c + (__int128)(unsigned long)-1) - &PR28739c; // expected-error {{not a compile-time constant}}
-void *PR28739d = &(&PR28739d)[(__int128)(unsigned long)-1]; // expected-error {{not a compile-time constant}}
-__int128 PR28739e = (__int128)(unsigned long)-1 + (long)&PR28739e; // expected-error {{not a compile-time constant}}
-__int128 PR28739f = (long)&PR28739f + (__int128)(unsigned long)-1; // expected-error {{not a compile-time constant}}
+// We evaluate these by providing 2s' complement semantics in constant
+// expressions, like we do for integers.
+void *PR28739a = (__int128)(unsigned long)-1 + &PR28739a;
+void *PR28739b = &PR28739b + (__int128)(unsigned long)-1;
+__int128 PR28739c = (&PR28739c + (__int128)(unsigned long)-1) - &PR28739c;
+void *PR28739d = &(&PR28739d)[(__int128)(unsigned long)-1];
Modified: cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp?rev=293595&r1=293594&r2=293595&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp (original)
+++ cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp Mon Jan 30 20:23:02 2017
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple i686-linux -Wno-string-plus-int -Wno-pointer-arith -Wno-zero-length-array -fsyntax-only -fcxx-exceptions -verify -std=c++11 -pedantic %s -Wno-comment -Wno-tautological-pointer-compare -Wno-bool-conversion
+// RUN: %clang_cc1 -triple x86_64-linux -Wno-string-plus-int -Wno-pointer-arith -Wno-zero-length-array -fsyntax-only -fcxx-exceptions -verify -std=c++11 -pedantic %s -Wno-comment -Wno-tautological-pointer-compare -Wno-bool-conversion
namespace StaticAssertFoldTest {
@@ -1303,7 +1303,7 @@ namespace ConvertedConstantExpr {
eo = (m +
n // expected-error {{not a constant expression}}
),
- eq = reinterpret_cast<int>((int*)0) // expected-error {{not a constant expression}} expected-note {{reinterpret_cast}}
+ eq = reinterpret_cast<long>((int*)0) // expected-error {{not a constant expression}} expected-note {{reinterpret_cast}}
};
}
@@ -1420,8 +1420,8 @@ namespace Fold {
// otherwise a constant expression.
#define fold(x) (__builtin_constant_p(x) ? (x) : (x))
- constexpr int n = (int)(char*)123; // expected-error {{constant expression}} expected-note {{reinterpret_cast}}
- constexpr int m = fold((int)(char*)123); // ok
+ constexpr int n = (long)(char*)123; // expected-error {{constant expression}} expected-note {{reinterpret_cast}}
+ constexpr int m = fold((long)(char*)123); // ok
static_assert(m == 123, "");
#undef fold
@@ -2136,3 +2136,17 @@ void g() {
} //end ns PR28366
+namespace PointerArithmeticOverflow {
+ int n;
+ int a[1];
+ constexpr int *b = &n + 1 + (long)-1;
+ constexpr int *c = &n + 1 + (unsigned long)-1; // expected-error {{constant expression}} expected-note {{cannot refer to element 1844}}
+ constexpr int *d = &n + 1 - (unsigned long)1;
+ constexpr int *e = a + 1 + (long)-1;
+ constexpr int *f = a + 1 + (unsigned long)-1; // expected-error {{constant expression}} expected-note {{cannot refer to element 1844}}
+ constexpr int *g = a + 1 - (unsigned long)1;
+
+ constexpr int *p = (&n + 1) + (unsigned __int128)-1; // expected-error {{constant expression}} expected-note {{cannot refer to element 3402}}
+ constexpr int *q = (&n + 1) - (unsigned __int128)-1; // expected-error {{constant expression}} expected-note {{cannot refer to element -3402}}
+ constexpr int *r = &(&n + 1)[(unsigned __int128)-1]; // expected-error {{constant expression}} expected-note {{cannot refer to element 3402}}
+}
Modified: cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp?rev=293595&r1=293594&r2=293595&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp (original)
+++ cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp Mon Jan 30 20:23:02 2017
@@ -980,5 +980,5 @@ constexpr int S = sum(Cs); // expected-e
constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
int *p = &n;
- p += (__int128)(unsigned long)-1; // expected-note {{subexpression}}
+ p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
}
More information about the cfe-commits
mailing list