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

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Aug 23, 2012 at 5:37 AM, Sandra Loosemore
<sandra@codesourcery.com> wrote:
> 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?

First of all the warning should be probably issued from stor-layout.c
itself - see
other cases where we warn about packed structs.  Yes, that means you'll
get the warning even when there is no access but you'll only get it a
single time.

As of detecting whether a field is not aligned according to its mode, well,
that's impossible if you just look at the field or its containing type (and thus
for a warning without false positives / negatives from stor-layout).  It's also
impossible to determine optimally (thus, it will say "not aligned according to
its mode" in more cases).  The way you detect such misalignment is,
given an access to such field,

  get_object_alignment (exp) < GET_MODE_ALIGNMENT (DECL_MODE (field))

when exp is a COMPONENT_REF with TREE_OPERAND (exp, 1) == field
and field has non-BLK mode.

Note that an access a.f with f being a volatile field may be represented
by a volatile MEM_REF[&a, 16] where 16 is the byte offset relative to a.

Now, if you are only interested in bitfields (note, bitfields which fit a mode
are _not_ bitfields as far as optimization passes are concerned and thus
may end up like above), then you probably want to look at
DECL_BIT_FIELD_REPRESENTATIVE of the FIELD_DECL of such field.
DECL_BIT_FIELD_REPRESENTATIVE constrains the offset / size said
field is to be accessed and you should call get_object_alignment with
a COMPONENT_REF using that field instead.  You also want to make
sure DECL_BIT_FIELD_REPRESENTATIVE is computed correctly for
volatile bitfields on ARM (stor-layout computes that).

Richard.

> -Sandra the confused
>