[PATCH] D20561: Warn when taking address of packed member

[Roger Ferrer Ibanez via cfe-commits]

Hi Aaron, Euvgeni,

I'll try to whitelist these functions.

Kind regards,
Roger

-----Original Message-----
From: Aaron Ballman [mailto:aaron.ballman at gmail.com] 
Sent: 14 June 2016 00:30
To: Evgenii Stepanov
Cc: reviews+D20561+public+af1fea8a731d85ae at reviews.llvm.org; Roger Ferrer Ibanez; Richard Smith; cfe-commits
Subject: Re: [PATCH] D20561: Warn when taking address of packed member

On Mon, Jun 13, 2016 at 7:17 PM, Evgenii Stepanov <eugenis at google.com> wrote:
> On Mon, Jun 13, 2016 at 4:12 PM, Aaron Ballman <aaron.ballman at gmail.com> wrote:
>> On Mon, Jun 13, 2016 at 6:30 PM, Evgeniy Stepanov <eugenis at google.com> wrote:
>>> eugenis added a subscriber: eugenis.
>>> eugenis added a comment.
>>>
>>> In http://reviews.llvm.org/D20561#446031, @aaron.ballman wrote:
>>>
>>>> In http://reviews.llvm.org/D20561#445824, @rogfer01 wrote:
>>>>
>>>> > I think I wasn't clear with the purpose of the fix-it: there are a few cases where getting the address of an unaligned pointer is safe (i.e. false positives).
>>>> >
>>>> > For instance, when I checked Firefox and Chromium there are cases where getting the address of an unaligned pointer is fine. For the particular case of these two browsers, they both use a library (usrsctp) that represents protocol data as packed structs. That library passes addresses of packed fields to `memcpy` and `memmove` which is OK.
>>>>
>>>>
>>>> I think this is a false-positive that should be fixed.
>>>
>>>
>>> This patch was committed without fixing the false positive case, why?
>>
>> Can you give some more code context, like perhaps a test case we can 
>> add to the suite? I believe the current behavior should be 
>> essentially false-positive-free because it only triggers the 
>> diagnostic when the alignments actually differ, so if there are other 
>> false-positives, I agree that we should determine a disposition for them.
>
> This is actually the same library the comment above is talking about:
> https://bugs.chromium.org/p/chromium/issues/detail?id=619640

The first example (gettimeofday()) looks to be a true-positive for sure. The remainder are tricky. They're not going to trigger UB in practice because the void * that memcpy() takes will get promptly typecast into a character pointer to avoid UB with the copy, and that's 1-byte aligned. There's a part of me that thinks it may be reasonable to assume that casting to void * always means "trust me, it'll be fine" because the pointer needs to be cast back out of a void
* to be useful, and so the diagnostic should be skipped in that case.
However, it's also just as likely to see code like (esp in C):

void func(void *ptr) {
  int *blah = ptr;
  *blah = 12;
}

void foo(void) {
  struct some_packed_struct s;
  func(&s.something);
}

which has drifted into UB.

Roger, is there a way to whitelist memcpy (memmove, memset, et al) for this check?

>>> Could this warning be excluded from -Wall?
>>
>> Would you like the warning to be excluded from -Wall even if the 
>> false-positive is fixed?
>
> No, the warning seems useful if the false positive is fixed.

Ok, good to know.

~Aaron