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. 2013-10-16 Bernd Edlinger <bernd.edlin...@hotmail.de>
Implement -Wportable-volatility warning to warn about
code that accesses volatile structure members for
which different ABI specifications exist.
* expr.c (check_portable_volatility): New function.
(expand_assignment): Call check_portable_volatility.
(expand_real_expr_1): Likewise.
* expr.h (check_portable_volatility): New function.
* fold-const.c (optimize_bit_field_compare): Call
check_portable_volatility. Removed if-statement, because
condition "flag_strict_volatile_bitfields > 0" is always false.
* c-family/c.opt: Add -Wportable-volatility option.
* doc/invoke.texi: Add documentation about -Wportable-volatility.
testsuite:
c-c++-common/
* Wportable-volatility-1.c: New testcase.
* Wportable-volatility-2.c: New testcase.
patch-portable-volatility.diff
Description: Binary data
