From c53aa4c3dd8c60071a320ea5fd851103366fe7fd Mon Sep 17 00:00:00 2001 From: kakje Date: Wed, 16 Jun 2021 14:20:04 -0700 Subject: [PATCH 1/5] Add ConstantFoldUpperOffset and GetRHSConstant methods to BaseRange --- clang/lib/Sema/SemaBounds.cpp | 94 +++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/clang/lib/Sema/SemaBounds.cpp b/clang/lib/Sema/SemaBounds.cpp index 60037d354a8e..20565818265d 100644 --- a/clang/lib/Sema/SemaBounds.cpp +++ b/clang/lib/Sema/SemaBounds.cpp @@ -1366,6 +1366,100 @@ namespace { LowerOffsetVariable(LowerOffsetVariable), UpperOffsetVariable(UpperOffsetVariable) { } + private: + // If E is of the form e +/- c, where c is a constant, GetRHSConstant + // returns true and sets the out parameter Constant. + // If E is of the form e + c, Constant will be set to c. + // If E is of the form e - c, Constant will be set to -c. + bool GetRHSConstant(BinaryOperator *E, llvm::APSInt &Constant) { + if (!E->isAdditiveOp()) + return false; + if (!E->getRHS()->isIntegerConstantExpr(Constant, S.Context)) + return false; + + bool Overflow; + Constant = BoundsUtil::ConvertToSignedPointerWidth(S.Context, Constant, Overflow); + if (Overflow) + return false; + // Normalize the operation by negating the offset if necessary. + if (E->getOpcode() == BO_Sub) { + uint64_t PointerWidth = S.Context.getTargetInfo().getPointerWidth(0); + Constant = llvm::APSInt(PointerWidth, false).ssub_ov(Constant, Overflow); + if (Overflow) + return false; + } + llvm::APSInt ElemSize; + if (!BoundsUtil::getReferentSizeInChars(S.Context, Base->getType(), ElemSize)) + return false;; + Constant = Constant.smul_ov(ElemSize, Overflow); + if (Overflow) + return false; + + return true; + } + + // ConstantFoldUpperOffset performs simple constant folding operations on + // UpperOffsetVariable. It attempts to extract a Variable part and a + // Constant part, based on the form of UpperOffsetVariable. + // + // If UpperOffsetVariable is of the form (e + a) + b: + // Variable = e, Constant = a + b. + // If UpperOffsetVariable is of the form (e + a) - b: + // Variable = e, Constant = a + -b. + // If UpperOffsetVariable is of the form (e - a) + b: + // Variable = e, Constant = -a + b. + // If UpperOffsetVariable is of the form (e - a) - b: + // Variable = e, Constant = -a + -b. + // + // Otherwise, ConstantFoldUpperOffset returns false, and: + // Variable = UpperOffsetVariable, Constant = 0. + // + // TODO: this method is part of a temporary solution to enable bounds + // checking to validate bounds such as (p, p + (len + 1) - 1). In the + // future, we should handle constant folding, commutativity, and + // associativity in bounds expressions in a more general way. + bool ConstantFoldUpperOffset(Expr *&Variable, llvm::APSInt &Constant) { + if (!IsUpperOffsetVariable()) + return false; + + llvm::APSInt LHSConst; + llvm::APSInt RHSConst; + + BinaryOperator *RootBinOp = dyn_cast(UpperOffsetVariable->IgnoreParens()); + if (!RootBinOp) + goto exit; + if (!RootBinOp->isAdditiveOp()) + goto exit; + + BinaryOperator *LHSBinOp = dyn_cast(RootBinOp->getLHS()->IgnoreParens()); + if (!LHSBinOp) + goto exit; + if (!LHSBinOp->isAdditiveOp()) + goto exit; + + if (!GetRHSConstant(RootBinOp, RHSConst)) + goto exit; + + if (!GetRHSConstant(LHSBinOp, LHSConst)) + goto exit; + + Variable = LHSBinOp->getLHS(); + + bool Overflow; + EnsureEqualBitWidths(LHSConst, RHSConst); + Constant = LHSConst.sadd_ov(RHSConst, Overflow); + if (Overflow) + goto exit; + return true; + + exit: + // Return (UpperOffsetVariable, 0). + Variable = UpperOffsetVariable; + uint64_t PointerWidth = S.Context.getTargetInfo().getPointerWidth(0); + Constant = llvm::APSInt(PointerWidth, false); + return false; + } + // Is R a subrange of this range? ProofResult InRange(BaseRange &R, ProofFailure &Cause, EquivExprSets *EquivExprs, std::pair& Facts) { From c804b2cb54f476522e6381ce649aaf63e2028493 Mon Sep 17 00:00:00 2001 From: kakje Date: Wed, 16 Jun 2021 14:21:33 -0700 Subject: [PATCH 2/5] Add CompareConstantFoldedUpperOffsets and EnsureEqualBitWidths methods to BaseRange --- clang/lib/Sema/SemaBounds.cpp | 79 +++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/clang/lib/Sema/SemaBounds.cpp b/clang/lib/Sema/SemaBounds.cpp index 20565818265d..db0adcad69e4 100644 --- a/clang/lib/Sema/SemaBounds.cpp +++ b/clang/lib/Sema/SemaBounds.cpp @@ -1367,10 +1367,30 @@ namespace { } private: + // EnsureEqualBitWidths modifies A or B if necessary so that A and B + // have the same bit width. The bit width of A and B will be the larger + // of the original bit widths of A and B. + // + // TODO: this method is part of a temporary solution to enable bounds + // checking to validate bounds such as (p, p + (len + 1) - 1). In the + // future, we should handle constant folding, commutativity, and + // associativity in bounds expressions in a more general way. + void EnsureEqualBitWidths(llvm::APSInt &A, llvm::APSInt &B) { + if (A.getBitWidth() < B.getBitWidth()) + A = A.extOrTrunc(B.getBitWidth()); + else if (B.getBitWidth() < A.getBitWidth()) + B = B.extOrTrunc(A.getBitWidth()); + } + // If E is of the form e +/- c, where c is a constant, GetRHSConstant // returns true and sets the out parameter Constant. // If E is of the form e + c, Constant will be set to c. // If E is of the form e - c, Constant will be set to -c. + // + // TODO: this method is part of a temporary solution to enable bounds + // checking to validate bounds such as (p, p + (len + 1) - 1). In the + // future, we should handle constant folding, commutativity, and + // associativity in bounds expressions in a more general way. bool GetRHSConstant(BinaryOperator *E, llvm::APSInt &Constant) { if (!E->isAdditiveOp()) return false; @@ -1460,6 +1480,55 @@ namespace { return false; } + // CompareConstantFoldedUpperOffsets is a fallback method that attempts + // to prove that R.UpperOffsetVariable <= this.UpperOffsetVariable. + // It returns true if: + // 1. this and R are both variable-sized ranges, and: + // 2. The upper offsets of this and R can both be constant folded + // according to the definition of ConstantFoldUpperOffset above, and: + // 3. The variable parts of the constant folded upper offsets are + // equivalent, and: + // 4. The constant upper part of R <= the constant upper part of this. + // + // Since lexicographically comparing variable upper offsets will not + // account for any constant folding, this method can be used to compare + // upper offsets that are not lexicographically equivalent. + // + // TODO: this method is part of a temporary solution to enable bounds + // checking to validate bounds such as (p, p + (len + 1) - 1). In the + // future, we should handle constant folding, commutativity, and + // associativity in bounds expressions in a more general way. + bool CompareUpperOffsetsWithConstantFolding(BaseRange &R, + EquivExprSets *EquivExprs) { + Expr *Variable = nullptr; + llvm::APSInt Constant; + bool ConstFolded = ConstantFoldUpperOffset(Variable, Constant); + + Expr *RVariable = nullptr; + llvm::APSInt RConstant; + bool RConstFolded = R.ConstantFoldUpperOffset(RVariable, RConstant); + + // If neither this nor R had their upper offsets constant folded, then + // the variable parts will be the respective upper offsets and the + // constant will both be 0. We already know the upper offsets are not + // equal from comparing them in CompareUpperOffsets, so there is no + // need for further comparison here. + if (!ConstFolded && !RConstFolded) + return false; + + // The variable parts of both upper offsets must have been set + // by ConstantFoldUpperOffset in order to compare them. + if (!Variable || !RVariable) + return false; + + if (!EqualValue(S.Context, Variable, RVariable, EquivExprs)) + return false; + + EnsureEqualBitWidths(Constant, RConstant); + return RConstant <= Constant; + } + + public: // Is R a subrange of this range? ProofResult InRange(BaseRange &R, ProofFailure &Cause, EquivExprSets *EquivExprs, std::pair& Facts) { @@ -1612,6 +1681,16 @@ namespace { UpperOffsetVariable->getType()->isUnsignedIntegerType() && R.UpperOffsetConstant.getExtValue() == 0) return ProofResult::True; + // If we cannot prove that R.UpperOffset <= this.UpperOffset using + // lexicographic comparison of expressions, attempt to perform simple + // constant folding operations on the upper offsets. + // TODO: this method is part of a temporary solution to enable bounds + // checking to validate bounds such as (p, p + (len + 1) - 1). In the + // future, we should handle constant folding, commutativity, and + // associativity in bounds expressions in a more general way. + if (CompareUpperOffsetsWithConstantFolding(R, EquivExprs)) + return ProofResult::True; + return ProofResult::Maybe; } From 84242677d0a43ddc59d61b3db328c1b0d24a338a Mon Sep 17 00:00:00 2001 From: kakje Date: Wed, 16 Jun 2021 14:22:00 -0700 Subject: [PATCH 3/5] Add tests for validating bounds with simple constant folding --- .../static-checking/bounds-decl-checking.c | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/clang/test/CheckedC/static-checking/bounds-decl-checking.c b/clang/test/CheckedC/static-checking/bounds-decl-checking.c index 778b087d222a..b77b9ee94c92 100644 --- a/clang/test/CheckedC/static-checking/bounds-decl-checking.c +++ b/clang/test/CheckedC/static-checking/bounds-decl-checking.c @@ -701,3 +701,36 @@ void f86(void) { } len = 4; } + +// +// Test comparing bounds expressions using simple constant folding +// + +extern int simulate_snprintf(char * restrict s : itype(restrict _Nt_array_ptr) count(n - 1), size_t n); + +void f87(_Nt_array_ptr p : count(len), size_t len) { + // No warning when proving that inferred argument bounds (p, p + len) imply + // expected argument bounds (p, p + ((len + 1) - 1)) + simulate_snprintf(p, len + 1); +} + +void f88(int len) { + _Array_ptr p : count(len + 2) = 0; + _Array_ptr q : count(len + 2 - 1 + 1) = p; +} + +void f89(_Ptr p) { + _Nt_array_ptr a : count(*p) = 0; + _Nt_array_ptr b : count(*p + 2 - 1) = 0; + a = b; +} + +struct S3 { + _Array_ptr f : count(len - 2 + 1); + _Array_ptr g : count(len); + int len; +}; + +void f90(struct S3 s) { + s.f = s.g; +} From 0a1cc3ecdbf57ffb9eb9e0c3cbf9d6da539a6172 Mon Sep 17 00:00:00 2001 From: kakje Date: Wed, 16 Jun 2021 16:28:07 -0700 Subject: [PATCH 4/5] Minor refactoring and comments --- clang/lib/Sema/SemaBounds.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/clang/lib/Sema/SemaBounds.cpp b/clang/lib/Sema/SemaBounds.cpp index db0adcad69e4..c24927b08243 100644 --- a/clang/lib/Sema/SemaBounds.cpp +++ b/clang/lib/Sema/SemaBounds.cpp @@ -1444,22 +1444,30 @@ namespace { llvm::APSInt LHSConst; llvm::APSInt RHSConst; + BinaryOperator *LHSBinOp = nullptr; - BinaryOperator *RootBinOp = dyn_cast(UpperOffsetVariable->IgnoreParens()); + // UpperOffsetVariable must be of the form LHS +/- RHS. + BinaryOperator *RootBinOp = + dyn_cast(UpperOffsetVariable->IgnoreParens()); if (!RootBinOp) goto exit; if (!RootBinOp->isAdditiveOp()) goto exit; - BinaryOperator *LHSBinOp = dyn_cast(RootBinOp->getLHS()->IgnoreParens()); + // UpperOffsetVariable must be of the form (e1 +/- e2) +/- RHS. + LHSBinOp = dyn_cast(RootBinOp->getLHS()->IgnoreParens()); if (!LHSBinOp) goto exit; if (!LHSBinOp->isAdditiveOp()) goto exit; + // UpperOffsetVariable must be of the form (e1 +/- e2) +/- b, + // where b is a constant. if (!GetRHSConstant(RootBinOp, RHSConst)) goto exit; + // UpperOffsetVariable must be of the form (e1 +/- a) +/- b, + // where a is a constant. if (!GetRHSConstant(LHSBinOp, LHSConst)) goto exit; From 067f14df46ea275079e27545c8097c4643bc1b03 Mon Sep 17 00:00:00 2001 From: kakje Date: Thu, 17 Jun 2021 11:04:01 -0700 Subject: [PATCH 5/5] Remove extra semicolon --- clang/lib/Sema/SemaBounds.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Sema/SemaBounds.cpp b/clang/lib/Sema/SemaBounds.cpp index c24927b08243..886b07a94893 100644 --- a/clang/lib/Sema/SemaBounds.cpp +++ b/clang/lib/Sema/SemaBounds.cpp @@ -1410,7 +1410,7 @@ namespace { } llvm::APSInt ElemSize; if (!BoundsUtil::getReferentSizeInChars(S.Context, Base->getType(), ElemSize)) - return false;; + return false; Constant = Constant.smul_ov(ElemSize, Overflow); if (Overflow) return false;