[cfe-commits] r133136 - in /cfe/trunk: lib/Sema/SemaChecking.cpp test/SemaCXX/warn-memset-bad-sizeof.cpp
thakis at chromium.org
Wed Jun 15 20:15:53 PDT 2011
On Wed, Jun 15, 2011 at 7:35 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> First off, thanks for the review.
> On Wed, Jun 15, 2011 at 7:27 PM, Nico Weber <thakis at chromium.org> wrote:
>> > Nico, please review; does this miss any of the bugs you were trying to
>> > find with this warning? The array test case you had should be caught by
>> > the array-specific sizeof warning I think.
>> Yes, now it doesn't find http://codereview.chromium.org/7167009/
>> Can you provide any examples where this warns when it shouldn't? (…and
>> maybe revert this in the meantime?)
> Ugh. So, I'd really rather not revert this, as it hits a frustratingly
> common false positive I added to the test cases: copying a pointer's value
> into or out of a character array, often from an opaque memory buffer. If the
> pointer is itself a character pointer, then the warning fires. Because the
> standard specifically blesses the use af character arrays and memcpy, it
> seems bad to warn on their interactions just because of one particular
> sizeof() usage in the third argument.
char c = 42;
memcpy(&parr, &c, sizeof(&c));
Doesn't this copy the (typically) 4-byte pointer parr into the
1-byte memory occupied by c? Shouldn't this be
memcpy(&parr, &c, sizeof(c));
(In which case the old version wouldn't warn either.)
/me feels blind
> Looking at the bug you point to, it's not clear why comparing the types of
> the first argument and the sizeof argument is the principled way to warn
> about this construct. You're not copying characters, and so the expected
> type of the argument to sizeof() isn't a 'char' (as the warning suggests).
> Instead, the correct code doesn't have any sizeof at all...
> Could chat and see if there is some better way to flag this kind of code?
More information about the cfe-commits