Re: [PATCH] Portable Volatility Warning

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

On Wed, Oct 16, 2013 at 2:48 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi Richard,
>
> well I think I have now a solution for both of your comments on the
> initial version of the portable volatility warning patch.
> Furthermore I have integrated Sandra's comments.
>
> Therefore I think it might be worth another try, if you don't mind.
>
> Technically this patch is not dependent on Sandra's patch any more.
> However it may be useful to use -Wportable-volatility together with
> -fno-strict-volatile-bitmaps, to get correct code, together with a warning
> where the device registers are accessed in possible wrong way.
> As we know the -fstrict-volatile-bitmaps generates wrong
> code most of the time, but the warning does not cover all cases
> where -fstrict-volatile-bitmaps generates wrong code
> without Sandra's patch.
>
> On Tue, 3 Sep 2013 10:53:10, Richard Biener wrote:
>> Just a quick first note:
>>
>> @@ -3470,6 +3470,13 @@ optimize_bit_field_compare (location_t l
>> || offset != 0 || TREE_CODE (linner) == PLACEHOLDER_EXPR)
>> return 0;
>>
>> + /* Do not try to optimize on volatiles, as it would defeat the
>> + -Wportable-volatility warning later:
>> + If the COMPONENT_REF is replaced by a BIT_FIELD_REF we loose
>> + information and thus the warning cannot be generated correctly. */
>> + if (warn_portable_volatility && lvolatilep)
>> + return 0;
>>
>> changing code-generation with a warning option looks wrong to me. Note
>> that I completely removed this code once (it also generates strange
>> BIT_FIELD_REFs) but re-instantiated it on request because of lost
>> optimizations.
>>
>
> I just emit the warning, when the COMPONENT_REF is about to be replaced,
> but leave the code generation flow intact.
>
>> Similar for
>>
>> @@ -609,11 +609,14 @@ layout_decl (tree decl, unsigned int kno
>> occupying a complete byte or bytes on proper boundary,
>> and not -fstrict-volatile-bitfields. If the latter is set,
>> we unfortunately can't check TREE_THIS_VOLATILE, as a cast
>> - may make a volatile object later. */
>> + may make a volatile object later.
>> + Handle -Wportable-volatility analogous, as we would loose
>> + information and defeat the warning later. */
>> if (TYPE_SIZE (type) != 0
>> && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
>> && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT
>> - && flag_strict_volatile_bitfields <= 0)
>> + && flag_strict_volatile_bitfields <= 0
>> + && warn_portable_volatility == 0)
>> {
>> enum machine_mode xmode
>> = mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);
>>
>> I also wonder about "we unfortunately can't check TREE_THIS_VOLATILE, as a cast
>> may make a volatile object later." - does that mean that AAPCS requires
>>
>> struct X { int x : 8; char c; };
>>
>> void foo (struct X *p)
>> {
>> ((volatile struct X *)p)->x = 1;
>> }
>>
>> to be treated as volatile bitfield access? Similar, is
>>
>> volatile struct X x;
>> x.x = 1;
>>
>> a volatile bitifled access?
>>
>> I think the warning can be completely implemented inside struct-layout.c
>> for example in finish_bitfield_representative (if you pass that the first field
>> in the group, too). Of course that is at the expense of warning for
>> struct declarations rather than actual code differences (the struct may
>> not be used)
>>
>> Richard.
>
>
> Bootstrapped and regression-tested on x86_64-linux-gnu.
>
> OK for trunk?

Hmm, I see the issue with optimize_bit_field_compare (though I
see a -fstrict-volatile-bitfields early-out in that function).  The folding
itself is weird enough (and also possibly violates the C++ memory
model eventually) that I'd consider instead disabling it unconditionally
if lvolatile || rvolatile (and remove all -fstrict-volatile-bitfield handling
from it).  Can you prepare a split out patch to do that?  I promise I'll
quickly approve it ;)

For the rest I think that this warning should warn independent of
optimization levels or actual bitfield member references, thus,
be implemented entirely at the time we lay out the structure.
That may not be able to take into account alignment issues, but your
patch doesn't either AFAIK.

Thanks and sorry for taking so long to review this again,
Richard.

> Thanks
> Bernd.