For clarification, which part of the restrictions I mentioned do you see as an ommision in the logic that need to be mentioned in comments with the patch to indicate they are intentional--given that Sema::CheckArrayAccess explicitly only works with an Expr of ConstantArrayType and a second Expr that must return true for isIntegerConstantExpr otherwise CheckArrayAccess doesn't have the two values it needs to perform the check, and given there is already a comment explaining the slight difference in acceptable values depending on whether the index value is an array subscript or a pointer-arithmetic offset?
On Thu, Jul 14, 2011 at 11:21 AM, Ted Kremenek <[email protected]> wrote: > Okay, I think that is a solid argument. I'd actually like those design > restrictions to be directly reflected in comments with the patch. That way > it is clear that those restrictions are intentional, as oppose to an > omission in the logic. > > On Jul 14, 2011, at 11:13 AM, Kaelyn Uhrain wrote: > > On Thu, Jul 14, 2011 at 10:28 AM, Ted Kremenek <[email protected]> wrote: > >> Hi Kaelyn, >> >> I was reviewing this patch (which I think is a great step), and I had a >> high-level comment about the following test case: >> >> +void swallow (const char *x) { (void)x; } >> +void test_pointer_arithmetic() { >> + const char hello[] = "Hello world!"; // expected-note 2 {{declared >> here}} >> + const char *helloptr = hello; >> + >> + swallow("Hello world!" + 6); // no-warning >> + swallow("Hello world!" - 6); // expected-warning {{refers before the >> beginning of the array}} >> + swallow("Hello world!" + 14); // expected-warning {{refers past the end >> of the array}} >> >> Do we really want this to be a warning? There are plenty of examples >> where an out-of-bounds pointer is computed for legit reasons. As long as >> that address is not dereferenced, there isn't necessarily a problem. I'm >> fearful this may generate a fair amount of noise on codebases that do >> elaborate tricks with pointer offsets. Indeed this very example doesn't >> actually exhibit a "bug". >> > > This patch came about from a Google-internal bug requesting a warning for > pointer arithmetic with string constants that is guaranteed to be undefined. > Given that the warning only triggers on pointer arithmetic involving a > constant-sized array and a constant value (as that's the only time the > compiler is guaranteed to know the result is invalid in regard to the array > itself) if and only if the result goes more than one past the end of the > array, I doubt there are many elaborate pointer offset tricks that would > trigger this warning and are not inherently buggy. The resulting pointers > would refer to arbitrary memory in or near the static program code/data or > the stack since the arrays that could trigger this warning are pretty much > by definition not allocated on the heap. Also, math on pointers (not arrays > or constant strings) does not trigger the warning, as shown by the uses of > helloptr in the test case--and at least in my limited experience, all of the > fancy pointer arithmetic tricks I've seen have on pointers (possibly to > constant arrays), not on literal arrays or array variables. Given the > constraints on when the warning will be triggered for pointer arithmetic, I > feel it does much more good than harm. > > Cheers, > Kaelyn > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
