This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH] Portable Volatility Warning


Hi Sandra,

thanks a lot, your comments are very welcome, especially as I am
not a native english speaker...


On Tue, 24 Sep 2013 15:46:22, Sandra Loosemore wrote:
>
> I have some nit-picky documentation suggestions about this patch....
>
> http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00100.html
>
>> + warning_at (input_location, OPT_Wportable_volatility,
>> + "the code to accesses this volatile member is dependent on"
>> + " whether -fstrict-volatile-bitfields is used;"
>> + " accessing device registers may depend on this option"
>> + " however it contradicts the C11/C++11 memory model");
>
> I'd simplify this to something like
>
> "access to volatile member may not conform to the C11/C++11"
> " memory model if -fstrict-volatile-bitfields is in effect"
>
>
>> --- gcc/c-family/c.opt 2013-05-28 21:55:10.000000000 +0200
>> +++ gcc/c-family/c.opt 2013-07-13 11:02:38.000000000 +0200
>> @@ -629,6 +629,10 @@ Wpointer-to-int-cast
>> C ObjC Var(warn_pointer_to_int_cast) Init(1) Warning
>> Warn when a pointer is cast to an integer of a different size
>>
>> +Wportable-volatility
>> +C ObjC C++ ObjC++ Var(warn_portable_volatility) Warning
>> +Warn about code for which separate incompatible definitions exist even within gcc
>> +
>
> This seems too vague. How about just
>
> Warn about potentially nonportable volatile memory accesses
>
>> +@item -Wportable-volatility
>> +@opindex Wportable-volatility
>> +@opindex Wno-portable-volatility
>> +Warn about code which accesses volatile structure members for
>
> s/which/that/
>
>> +which different ABI specifications may exist whether in some
>> +language standard or in a target-specific ABI document.
>> +
>> +For instance on the ARM architecture AAPCS specifies how to
>> +access volatile bit-fields. But for C/C++ there exists a
>> +language standard, the C11/C++11 memory model. GCC tries to
>> +follow the latter by default unless -fstrict-volatile-bitfields
>> +is used.
>> +
>> +As an example this option will warn about code which accesses
>> +packed volatile structure members which may be dependent on
>> +whether -fstrict-volatile-bitfields is used or not.
>
> I'd replace the last two paragraphs with just:
>
> In particular, this option causes GCC to warn about volatile memory
> accesses for which use of the @option{-fstrict-volatile-bitfields}
> option causes code to be generated that does not conform to the
> C11/C++11 memory model.
>
> I'd also like to see a cross-reference added to the documentation for
> -fstrict-volatile-bitfields, like:
>
> You can use @option{-Wportable-volatility} to make GCC issue warnings
> about volatile structure accesses affected by
> @option{-fstrict-volatile-bitfields}.
>
> -Sandra


Richard: I do not know, is this a political issue, that is blocking
the whole of Sandra's patch?

Actually we (softing.com) do not really care what happens to the
default setting of -fstrict-volatile-bitfields. Maybe you could look at
reviewing Sandra's part 1,2,3 independently of the rest?

Once that is approved, I could start again my work on the warnings part.
Meanwhile I've got an idea how to solve you first objection:

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.

The idea is, I'd emit the warning at the time when the COMPONENT_REF
is replaced by a BIT_FIELD_REF. Then the warning code would not have
access to  !MEM_P (op0)       || !MEM_VOLATILE_P (op0) but this should not
be a big issue.


Thanks
Bernd. 		 	   		  

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]