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

[Alex L via cfe-commits]

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.


> Wouldn't cases 1) and 3) catch most situations where this happens
> unintentionally? E.g. when one #includes a file that forgets to
> #pragma pop, and then includes a new file afterwards?
>
> I've reverted in r309186 in the meantime.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170727/7af71d68/attachment.html>