RE: [PATCH] Portable Volatility Warning

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

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?

Thanks
Bernd.

Attachment: changelog-portable-volatility.txt
Description: Text document

Attachment: patch-portable-volatility.diff
Description: Binary data