Re: [PATCH] fix wrong-code bug for -fstrict-volatile-bitfields

[Sandra Loosemore <sandra at codesourcery dot com>]

On 08/22/2012 03:27 PM, Eric Botcazou wrote:
> > +             bool packedp = false;
> > +
> > +             if (TREE_CODE(to) == COMPONENT_REF
> > +&&  (TYPE_PACKED (TREE_TYPE (TREE_OPERAND (to, 0)))
> > +                     || (TREE_CODE (TREE_OPERAND (to, 1)) == FIELD_DECL
> > +&&  DECL_PACKED (TREE_OPERAND (to, 1)))))
> > +               packedp = true;
> >
> > note that this is not a reliable way of determining if a field is packed
> > (yes, the existing code is broken in the same way).  I realize that you
> > are only using this for diagnostic purposes, still something worth
> > mentioning.
>
> Indeed, ideally DECL_PACKED shouldn't be used outside stor-layout.c and
> dwarf2out.c in the middle-end.
>
> > I dislike passing around the packedp flag throughout the expander and to
> > warn inside the expander.  First, the flag has a name that can be confusing
> > (it does not conservatively flag all packed accesses).  Second, why don't
> > you warn for this from inside the frontend when the bitfield access is
> > generated?  This way the diagnostic won't depend on optimization levels
> > and you avoid uglifying the expander.
>
> Seconded.  If the -fstrict-volatile-bitfields support is still incomplete,
> let's take this opportunity to clean it up.  Testing DECL_PACKED or issuing
> a warning from the RTL expander is to be avoided.

Well, I'm willing to give it a shot, but from a pragmatic point of view 
I've only got about a week left to wind up the current batch of patches 
I've got pending before I'm reassigned to a new project.  Can one of you 
give a more specific outline of how this should be fixed, to increase 
the chances that I can actually implement an acceptable solution in the 
time available?  In particular, the packedp flag that's being passed 
down is *not* just used for diagnostics; it changes code generation, and 
this code is scary enough that I'm worried about preserving the current 
behavior (which IIUC is required by the ARM EABI) if this is 
restructured.  If only the diagnostics should be moved elsewhere, where, 
exactly?  Or is there just a better test that can be used in the 
existing places to determine whether a field is packed?

-Sandra the confused