Hi Ted,
I've attached a new version of my patch that moves the pointer arithmetic
portion of the -Warray-bounds improvements into a separate flag,
-Warray-bounds-pointer-arithmetic. Since the remaining noise in the pointer
arithmetic check is more a case of undefined behavior that does the right
thing with most (all?) compilers than a case of being true false positives,
the plan is to have -Warray-bounds-pointer-arithmetic initially be disabled
by default and in later patches to enhance the warning by e.g. providing a
fixit note to add parens to exprs like "ptr + sizeof("foo") - 1" so they
become "ptr + (sizeof("foo")) - 1".
Cheers,
Kaelyn
On Thu, Jul 21, 2011 at 1:18 PM, Ted Kremenek <[email protected]> wrote:
> Hi Kaelyn,
>
> While I think there will be some contention here, I think this is still too
> high of a false positive rate for this warning to be on by default in the
> compiler. Users simply aren't going to tolerate it. I think it is
> reasonable for a codebase to adopt a strict policy for this warning, but I
> don't think a 17% false positive rate (or possibly higher) is acceptable for
> a default warning for all users of Clang.
>
> Unless you believe you think there are additional heuristics to drop the
> false positive rate down below 5-10%, I think I'm fine with proceeding as
> putting this in as an opt-in warning, and then refine from there.
>
> On Jul 20, 2011, at 11:39 AM, Kaelyn Uhrain wrote:
>
> I've attached an updated version of my patch that better handles cases
> where pointer arithmetic is done after casting a constant-size array to a
> pointer for a smaller base type (e.g. casting an int array to char*). Of the
> pointer arithmetic warnings, about 24% could be considered false positives;
> however, the actual number of false positives is quite small and 2/3 of them
> stem from the use of a single macro--if you count those as a single warning
> & false positive, the rate drops to 17%. Of the false positives most are
> from semi-questionable pointer arithmetic where a constant greater than the
> length of the array/pointer is being added to the pointer and some int > 1
> being subtracted from it, e.g.:
>
> void foo(int n) {
> char x[5];
> if (n > 0) bar(x + 6 - n);
> }
>
> Cheers,
> Kaelyn
>
> On Tue, Jul 19, 2011 at 11:44 AM, Kaelyn Uhrain <[email protected]> wrote:
>
>> I'm still looking at the pointer arithmetic warnings to determine which
>> are false positives, but I realized I screwed up my previous stats a bit as
>> I forgot to account for duplicated/repeated warnings (e.g. the same header
>> included in multiple compilation units and so generating the same warnings
>> multiple times). For unique warnings, the stats are:
>>
>> - 16.5% increase in warnings from before my patch (originally reported a
>> 24% increase)
>> - 47% of those new warnings being about pointer arithmetic
>> - 6.7% of all of the warnings emitted with my patch applied are concerning
>> pointer arithmetic. (originally reported 8.3%)
>> - The new pointer arithmetic warnings represent a 7.7% increase in
>> warnings from before my patch, not 10%.
>>
>> I'll send another email once I have a feel for how noisy the pointer
>> arithmetic warnings are.
>>
>> Cheers,
>> Kaelyn
>>
>>
>> On Mon, Jul 18, 2011 at 5:54 PM, Kaelyn Uhrain <[email protected]> wrote:
>>
>>> Ted,
>>>
>>> You're welcome. I'll try to figure out what fraction of the pointer
>>> arithmetic warnings are false positives (requires a bit of manual digging on
>>> my part to determine if the code is indeed buggy or if it is valid /
>>> intended). For the overall 24% increase in warnings, keep in mind that over
>>> half of that is the existing bounds checking now being applied to cases
>>> where it wasn't before, i.e.:
>>>
>>> char *foo[5];
>>> foo[77]; // -Warray-bounds already found
>>> &foo[77]; // -Warray-bounds currently misses
>>> *foo[77]; // -Warray-bounds currently misses
>>>
>>> The function that did the bounds checking would never catch the last two
>>> cases because it would see a UnaryOperator (in the above cases for the '&'
>>> and the '*') and skip the expression instead of looking inside the
>>> UnaryOperator expression for the array subscripting.
>>>
>>> The new pointer arithmetic bounds checking only represents a 10% increase
>>> in warnings--and IMHO that is the only part where the number of false
>>> positives introduced might be an issue.
>>>
>>> Cheers,
>>> Kaelyn
>>>
>>>
>>> On Mon, Jul 18, 2011 at 5:36 PM, Ted Kremenek <[email protected]>wrote:
>>>
>>>> Hi Kaelyn,
>>>>
>>>> Thanks for the statistics. What would be good to know is what fraction
>>>> of these are false positives (i.e., are these all real bugs)? A small
>>>> random sample might be helpful. A 24% increase in warnings is fairly
>>>> substantial, and we don't want to do that unless there is a real benefit.
>>>>
>>>> Ted
>>>>
>>>> On Jul 18, 2011, at 5:18 PM, Kaelyn Uhrain wrote:
>>>>
>>>> Ted,
>>>>
>>>> On Thu, Jul 14, 2011 at 5:04 PM, Ted Kremenek <[email protected]>wrote:
>>>>
>>>>>
>>>>> The only other issue: should this be controlled under a separate
>>>>> warning flag, at least initially so we can experiment with this new
>>>>> warning
>>>>> and see how noisy it is? E.g. "-Warray-bounds-pointer-arithmetic".
>>>>>
>>>>
>>>> I've tested the -Warray-bounds changes against the Google codebase and
>>>> my patch increases the number of warnings from -Warray-bounds by 24%. Of
>>>> the
>>>> new warnings, 57.33% are for array indexes that most likely weren't picked
>>>> up before because of unary operators like & or * (11.1% of all the warnings
>>>> now emitted), and the remaining 42.67% are from out-of-bounds pointer
>>>> arithmetic (8.3% of all the warnings from -Warray-bounds).
>>>>
>>>> Cheers,
>>>> Kaelyn
>>>>
>>>>
>>>>
>>>
>>
> <array-bounds-enhancement3.diff>
>
>
>
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 97414f2..3b79751 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4014,11 +4014,17 @@ def err_out_of_line_default_deletes : Error<
def err_defaulted_move_unsupported : Error<
"defaulting move functions not yet supported">;
+def warn_ptr_arith_precedes_bounds : Warning<
+ "the pointer decremented by %0 refers before the beginning of the array">,
+ InGroup<DiagGroup<"array-bounds-pointer-arithmetic">>, DefaultIgnore;
+def warn_ptr_arith_exceeds_bounds : Warning<
+ "the pointer incremented by %0 refers past the end of the array (that contains %1 element%s2)">,
+ InGroup<DiagGroup<"array-bounds-pointer-arithmetic">>, DefaultIgnore;
def warn_array_index_precedes_bounds : Warning<
"array index of '%0' indexes before the beginning of the array">,
InGroup<DiagGroup<"array-bounds">>;
def warn_array_index_exceeds_bounds : Warning<
- "array index of '%0' indexes past the end of an array (that contains %1 elements)">,
+ "array index of '%0' indexes past the end of an array (that contains %1 element%s2)">,
InGroup<DiagGroup<"array-bounds">>;
def note_array_index_out_of_bounds : Note<
"array %0 declared here">;
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 263c1bd..0264221 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -5874,6 +5874,8 @@ public:
unsigned ByteNo) const;
private:
+ void CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
+ bool isSubscript=false);
void CheckArrayAccess(const Expr *E);
bool CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall);
bool CheckBlockCall(NamedDecl *NDecl, CallExpr *TheCall);
diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 690a29d..04e21ee 100644
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -3480,43 +3480,87 @@ void Sema::CheckCastAlign(Expr *Op, QualType T, SourceRange TRange) {
<< TRange << Op->getSourceRange();
}
-static void CheckArrayAccess_Check(Sema &S,
- const clang::ArraySubscriptExpr *E) {
- const Expr *BaseExpr = E->getBase()->IgnoreParenImpCasts();
+static const Type* getElementType(const Expr *BaseExpr) {
+ const Type* EltType = BaseExpr->getType().getTypePtr();
+ if (EltType->isAnyPointerType())
+ return EltType->getPointeeType().getTypePtr();
+ else if (EltType->isArrayType())
+ return EltType->getBaseElementTypeUnsafe();
+ return EltType;
+}
+
+void Sema::CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
+ bool isSubscript) {
+ const Type* EffectiveType = getElementType(BaseExpr);
+ BaseExpr = BaseExpr->IgnoreParenCasts();
+ IndexExpr = IndexExpr->IgnoreParenCasts();
+
const ConstantArrayType *ArrayTy =
- S.Context.getAsConstantArrayType(BaseExpr->getType());
+ Context.getAsConstantArrayType(BaseExpr->getType());
if (!ArrayTy)
return;
- const Expr *IndexExpr = E->getIdx();
if (IndexExpr->isValueDependent())
return;
llvm::APSInt index;
- if (!IndexExpr->isIntegerConstantExpr(index, S.Context))
+ if (!IndexExpr->isIntegerConstantExpr(index, Context))
return;
if (index.isUnsigned() || !index.isNegative()) {
llvm::APInt size = ArrayTy->getSize();
if (!size.isStrictlyPositive())
return;
+
+ const Type* BaseType = getElementType(BaseExpr);
+ if (BaseType != EffectiveType) {
+ // Make sure we're comparing apples to apples when comparing index to size
+ uint64_t ptrarith_typesize = Context.getTypeSize(EffectiveType);
+ uint64_t array_typesize = Context.getTypeSize(BaseType);
+ if (ptrarith_typesize != array_typesize) {
+ // There's a cast to a different size type involved
+ uint64_t ratio = array_typesize / ptrarith_typesize;
+ if (ptrarith_typesize * ratio != array_typesize)
+ // If the size of the array's base type is not a multiple of the
+ // casted-to pointee type, the results of the pointer arithmetic
+ // may or may not point to something consistently meaningful or within a
+ // valid reference...
+ return;
+
+ size *= llvm::APInt(size.getBitWidth(), ratio);
+ }
+ }
+
if (size.getBitWidth() > index.getBitWidth())
index = index.sext(size.getBitWidth());
else if (size.getBitWidth() < index.getBitWidth())
size = size.sext(index.getBitWidth());
- if (index.slt(size))
+ // For array subscripting the index must be less than size, but for pointer
+ // arithmetic also allow the index (offset) to be equal to size since
+ // computing the next address after the end of the array is legal and
+ // commonly done e.g. in C++ iterators and range-based for loops.
+ if (isSubscript ? index.slt(size) : index.sle(size))
return;
- S.DiagRuntimeBehavior(E->getBase()->getLocStart(), BaseExpr,
- S.PDiag(diag::warn_array_index_exceeds_bounds)
- << index.toString(10, true)
- << size.toString(10, true)
- << IndexExpr->getSourceRange());
+ unsigned DiagID = diag::warn_ptr_arith_exceeds_bounds;
+ if (isSubscript)
+ DiagID = diag::warn_array_index_exceeds_bounds;
+
+ DiagRuntimeBehavior(BaseExpr->getLocStart(), BaseExpr,
+ PDiag(DiagID) << index.toString(10, true)
+ << size.toString(10, true)
+ << (unsigned)size.getLimitedValue(~0U)
+ << IndexExpr->getSourceRange());
} else {
- S.DiagRuntimeBehavior(E->getBase()->getLocStart(), BaseExpr,
- S.PDiag(diag::warn_array_index_precedes_bounds)
- << index.toString(10, true)
- << IndexExpr->getSourceRange());
+ unsigned DiagID = diag::warn_array_index_precedes_bounds;
+ if (!isSubscript) {
+ DiagID = diag::warn_ptr_arith_precedes_bounds;
+ if (index.isNegative()) index = -index;
+ }
+
+ DiagRuntimeBehavior(BaseExpr->getLocStart(), BaseExpr,
+ PDiag(DiagID) << index.toString(10, true)
+ << IndexExpr->getSourceRange());
}
const NamedDecl *ND = NULL;
@@ -3525,18 +3569,21 @@ static void CheckArrayAccess_Check(Sema &S,
if (const MemberExpr *ME = dyn_cast<MemberExpr>(BaseExpr))
ND = dyn_cast<NamedDecl>(ME->getMemberDecl());
if (ND)
- S.DiagRuntimeBehavior(ND->getLocStart(), BaseExpr,
- S.PDiag(diag::note_array_index_out_of_bounds)
- << ND->getDeclName());
+ DiagRuntimeBehavior(ND->getLocStart(), BaseExpr,
+ PDiag(diag::note_array_index_out_of_bounds)
+ << ND->getDeclName());
}
void Sema::CheckArrayAccess(const Expr *expr) {
while (true) {
expr = expr->IgnoreParens();
switch (expr->getStmtClass()) {
- case Stmt::ArraySubscriptExprClass:
- CheckArrayAccess_Check(*this, cast<ArraySubscriptExpr>(expr));
+ case Stmt::ArraySubscriptExprClass: {
+ const ArraySubscriptExpr *ASE = cast<ArraySubscriptExpr>(expr);
+ CheckArrayAccess(ASE->getBase(),
+ ASE->getIdx(), true);
return;
+ }
case Stmt::ConditionalOperatorClass: {
const ConditionalOperator *cond = cast<ConditionalOperator>(expr);
if (const Expr *lhs = cond->getLHS())
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 5efc365..f6e077f 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -268,6 +268,7 @@ ExprResult Sema::DefaultFunctionArrayConversion(Expr *E) {
E = ImpCastExprToType(E, Context.getPointerType(Ty),
CK_FunctionToPointerDecay).take();
else if (Ty->isArrayType()) {
+ CheckArrayAccess(E);
// In C90 mode, arrays only promote to pointers if the array expression is
// an lvalue. The relevant legalese is C90 6.2.2.1p3: "an lvalue that has
// type 'array of type' is converted to an expression that has type 'pointer
@@ -310,7 +311,6 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E) {
// A glvalue of a non-function, non-array type T can be
// converted to a prvalue.
if (!E->isGLValue()) return Owned(E);
-
QualType T = E->getType();
assert(!T.isNull() && "r-value conversion on typeless expression?");
@@ -386,6 +386,14 @@ ExprResult Sema::UsualUnaryConversions(Expr *E) {
QualType Ty = E->getType();
assert(!Ty.isNull() && "UsualUnaryConversions - missing type");
+ if (Ty->isPointerType() || Ty->isArrayType()) {
+ Expr *subE = E->IgnoreParenImpCasts();
+ while (UnaryOperator *UO = dyn_cast<UnaryOperator>(subE)) {
+ subE = UO->getSubExpr()->IgnoreParenImpCasts();
+ }
+ if (subE) CheckArrayAccess(subE);
+ }
+
// Try to perform integral promotions if the object has a theoretically
// promotable type.
if (Ty->isIntegralOrUnscopedEnumerationType()) {
@@ -5813,6 +5821,8 @@ QualType Sema::CheckAdditionOperands( // C99 6.5.6
return QualType();
}
+ CheckArrayAccess(PExp, IExp);
+
if (CompLHSTy) {
QualType LHSTy = Context.isPromotableBitField(lex.get());
if (LHSTy.isNull()) {
@@ -5867,6 +5877,11 @@ QualType Sema::CheckSubtractionOperands(ExprResult &lex, ExprResult &rex,
if (!checkArithmeticOpPointerOperand(*this, Loc, lex.get()))
return QualType();
+ Expr *IExpr = rex.get()->IgnoreParenCasts();
+ UnaryOperator negRex(IExpr, UO_Minus, IExpr->getType(), VK_RValue,
+ OK_Ordinary, IExpr->getExprLoc());
+ CheckArrayAccess(lex.get()->IgnoreParenCasts(), &negRex);
+
if (CompLHSTy) *CompLHSTy = lex.get()->getType();
return lex.get()->getType();
}
diff --git a/test/SemaCXX/array-bounds.cpp b/test/SemaCXX/array-bounds.cpp
index 3bd6c35..f7bb7dd 100644
--- a/test/SemaCXX/array-bounds.cpp
+++ b/test/SemaCXX/array-bounds.cpp
@@ -25,7 +25,7 @@ void f1(int a[1]) {
}
void f2(const int (&a)[1]) { // expected-note {{declared here}}
- int val = a[3]; // expected-warning {{array index of '3' indexes past the end of an array (that contains 1 elements)}}
+ int val = a[3]; // expected-warning {{array index of '3' indexes past the end of an array (that contains 1 element)}}
}
void test() {
@@ -35,15 +35,17 @@ void test() {
s2.a[3] = 0; // no warning for 0-sized array
union {
- short a[2]; // expected-note {{declared here}}
+ short a[2]; // expected-note 2 {{declared here}}
char c[4];
} u;
u.a[3] = 1; // expected-warning {{array index of '3' indexes past the end of an array (that contains 2 elements)}}
u.c[3] = 1; // no warning
+ *(&u.a[3]) = 1; // expected-warning {{array index of '3' indexes past the end of an array (that contains 2 elements)}}
+ *(&u.c[3]) = 1; // no warning
const int const_subscript = 3;
int array[1]; // expected-note {{declared here}}
- array[const_subscript] = 0; // expected-warning {{array index of '3' indexes past the end of an array (that contains 1 elements)}}
+ array[const_subscript] = 0; // expected-warning {{array index of '3' indexes past the end of an array (that contains 1 element)}}
int *ptr;
ptr[3] = 0; // no warning for pointer references
@@ -59,7 +61,7 @@ void test() {
char c2 = str2[5]; // expected-warning {{array index of '5' indexes past the end of an array (that contains 4 elements)}}
int (*array_ptr)[1];
- (*array_ptr)[3] = 1; // expected-warning {{array index of '3' indexes past the end of an array (that contains 1 elements)}}
+ (*array_ptr)[3] = 1; // expected-warning {{array index of '3' indexes past the end of an array (that contains 1 element)}}
}
template <int I> struct S {
@@ -151,8 +153,7 @@ void test_switch() {
enum enumA { enumA_A, enumA_B, enumA_C, enumA_D, enumA_E };
enum enumB { enumB_X, enumB_Y, enumB_Z };
static enum enumB myVal = enumB_X;
-void test_nested_switch()
-{
+void test_nested_switch() {
switch (enumA_E) { // expected-warning {{no case matching constant}}
switch (myVal) { // expected-warning {{enumeration values 'enumB_X' and 'enumB_Z' not handled in switch}}
case enumB_Y: ;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
