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.
