[cfe-commits] r133136 - in /cfe/trunk: lib/Sema/SemaChecking.cpp test/SemaCXX/warn-memset-bad-sizeof.cpp

[Nico Weber]

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.

You added

  char c = 42;
  char* parr[5];
  memcpy(&parr[3], &c, sizeof(&c));

Doesn't this copy the (typically) 4-byte pointer parr[3] into the
1-byte memory occupied by c? Shouldn't this be

  char* c;
  char* parr[5];
  memcpy(&parr[3], &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?