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}} } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits