r309106 - Recommit r308327 2nd time: Add a warning for missing

[Hans Wennborg via cfe-commits]

On Thu, Jul 27, 2017 at 3:41 AM, Alex L <arphaman at gmail.com> wrote:
>
>
> On 26 July 2017 at 22:32, Hans Wennborg <hans at chromium.org> wrote:
>>
>> On Wed, Jul 26, 2017 at 11:27 AM, Hans Wennborg <hans at chromium.org> wrote:
>> > On Wed, Jul 26, 2017 at 5:20 AM, Alex Lorenz via cfe-commits
>> > <cfe-commits at lists.llvm.org> wrote:
>> >> Author: arphaman
>> >> Date: Wed Jul 26 05:20:57 2017
>> >> New Revision: 309106
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=309106&view=rev
>> >> Log:
>> >> Recommit r308327 2nd time: Add a warning for missing
>> >> '#pragma pack (pop)' and suspicious uses of '#pragma pack' in included
>> >> files
>> >>
>> >> The first recommit (r308441) caused a "non-default #pragma pack value
>> >> might
>> >> change the alignment of struct or union members in the included file"
>> >> warning
>> >> in LLVM itself. This recommit tweaks the added warning to avoid
>> >> warnings for
>> >> #includes that don't have any records that are affected by the
>> >> non-default
>> >> alignment. This tweak avoids the previously emitted warning in LLVM.
>> >>
>> >> Original message:
>> >>
>> >> This commit adds a new -Wpragma-pack warning. It warns in the following
>> >> cases:
>> >>
>> >> - When a translation unit is missing terminating #pragma pack (pop)
>> >> directives.
>> >> - When entering an included file if the current alignment value as
>> >> determined
>> >>   by '#pragma pack' directives is different from the default alignment
>> >> value.
>> >> - When leaving an included file that changed the state of the current
>> >> alignment
>> >>   value.
>> >>
>> >> rdar://10184173
>> >>
>> >> Differential Revision: https://reviews.llvm.org/D35484
>> >
>> > We have code in Chromium that does exactly this:
>> >
>> > gles2_cmd_format.h does #pragma pack(push, 4) and then #includes a
>> > file with some generated structs, with the intention that the pragma
>> > applies to them.
>> >
>> > What's the best way to pacify the warning in this case?
>> >
>> > (We're tracking this in
>> > https://bugs.chromium.org/p/chromium/issues/detail?id=749197)
>>
>> I agree that cases 1) and 3) from your patch description make sense to
>> warn for, but I'm not sure that's the case for 2). Do you have
>> examples where this catches any bugs? In our case #pragma packing an
>> included file is intentional, and I suspect it might be a bit of a
>> pattern.
>
>
> I see, thanks for your input.
>
> 2) is generally designed for times when #pragma pack pop was accidentally
> used too late (after some #includes that unintentionally receive the
> alignment). I can see how some projects use this pattern heavily, and I
> don't think there's a good way to pacify this warning in that case.
>
> I think that for us it would be reasonable to turn 2) off by default, and
> allow users to enable it explicitly using a stronger flag (something like
> -Wpragma-pack-suspicious-include?). I think that I will leave 2) out of this
> commit, recommit it without 2) and then commit 2) as a non-default warning
> that uses a separate flag.

That sounds reasonable. You can probably still do it with the same
commit, just moving 2) behind a separate flag.

Thanks,
Hans