Hello Hans-Peter, > On Sat, 13 Jul 2013, Bernd Edlinger wrote: >> Hi Sandra, >> >> On Fri, 5 Jul 2013, Hans-Peter Nilsson wrote >>> Or - maybe more acceptable - an optional warning, say >>> -Wportable-volatility, to warn about code for which separate >>> incompatbile definitions on different platforms (or between C >>> and C++) exist even within gcc. It would be usable for driver >>> code you want to be usable on different architectures, say, in >>> an OS commonly compiled with gcc, if you can think of some. :) >> >> >> I like this idea, and this warning would add some real value for everyone. >> Therefore I added that option as part 5 of this patch series, I hope you >> don't mind :) > > Definitely not. Thanks for picking up the ball! > >> I really hope that the GCC maintainers can accept this patch now, as the >> current state of -fstrict-volatile-bitfields is very painful to all of us. > > I guess I should offer a first-hand review of the warnings part. > > I've got nothing on the code, however the documentation ties the > warning only to -fstrict-volatile-bitfields, which it shouldn't; > it should be stated generic, but perhaps with > -fstrict-volatile-bitfields as an *example*. (And for those who > now feel the need to say "but volatile behavior is undefined" > without reading the rest of the thread, remember that this is > intended to cover cases where some definition actually *exist* > whether in some language standard or some target-specific ABI > document.) > > It also needs test-cases, both for some positive cases (warning > hits) and some case where it doesn't (and shouldn't). > > ...and ChangeLog entries. > > Thanks again! > > brgds, H-P
thanks a lot for your Feedback, I created a change log entry, revised the documentation as you suggested, and added two new almost identical test cases: One with -fstrict-volatile-bitfields and one without. This warning should only be emitted if the code is significantly different when -fstrict-volatile-bitfields is used or not. It should not be emitted for the code which is not affected by this option. In other words, it should be emitted on all bit-fields when the definition or the context is volatile, except when the whole structure is not AAPCS ABI compliant, i.e. packed or unaligned. On the other hand you should always get the same warnings if you combine -Wportable-volatility with -fstrict-volatile-bitfields or not. And of course it should not depend on the specific target that is used to compile. However when I tried to write simple test cases, I realized that my initial code was emitting almost no warnings when -fno-strict-volatile-bitfields is used. I analyzed that and came to the conclusion, that the function strict_volatile_bitfield_p() is not the best place to emit this warning, even if is is always executed with -fstrict-volatile-bitfields. But with -fno-strict-volatile-bitfields the program flow in store_expr() may divert to emit_move_insn(), especially in trivial cases. Well, I decided that the best place to emit that warning would be the function expand_assignment() and expand_expr_real_1(), because here we have a rtx and the tree object plus the bitsize and bitnum all together. However the first time when the flag_strict_volatile_bitfields is used, is in stor_layout.c: Here a bit-field int a:8 is replaced by char a if that is possible. Therefore I decided that this optimization is in the way of -Wportable-volatility, and has to be disabled for the same reasons as with -fstrict-volatle-bitfields. The next problem was in fold-const.c: optimize_bit_field_compare() can replace the COMPONENT_REF with a BIT_FIELD_REF, even for volatile objects. But that conversion looses too much information to recover the bit-field's DECL_BIT_FIELD_TYPE. Therefore this warning has to be skipped at least for volatile lhs or rhs arguments if -Wportable-volatility is used. I boot-strapped this on a i686-pc-linux-gnu and all Wportable-volatility test cases are passed for C and C++. I used a cross-compiler for arm-eabi to manually cross-check that the warnings are independent of the target platform. Sandra's test case pr23623 does emit exactly 4 warnings if compiled with -Wportable-volatilty, which is correct. All other test cases from part 3 use unaligned or packed structures which are not AAPCS compliant at all. Therefore they do not emit any warnings, which is also correct, because they emit exactly the same code, regardless of the -fstrict-volatile-bitfield setting. This may however deserve a warning on it's own. H-P: I hope you can approve my little patch for trunk now, although it turned out to be less trivial than I'd have expected. Of course it is dependent on Sandra's patch part 1 and part 2, which must be applied first. As far as I can see, the part 2 is still missing a formal approval of one of the global reviewers. Could some one please do that now? Sandra, regarding Part 3, I have a small recommendation on it: The test programs pr56997-*.c depend on <stdint.h> and other headers. Could you please remove these dependencies? Thanks. Regarding Part 4, I see both sides have of course good reasons. If you follow the C++11 memory model you may have byte accesses when word accesses would be required for device registers. But if you follow the AAPCS standard you may have race conditions outside the bit-field representative. I personally am in favour of the C++11 memory model, because today the majority of the software are multi-threaded C++ applications. And having a portable way to decide where to use mutexes and where not is a very important feature. That's why I felt the need to implement the portable-volatility warning. Because we really should try hard, not to silently break existing valid code. Thanks Bernd.
2013-07-23 Bernd Edlinger <bernd.edlin...@hotmail.de>
Implement -Wportable-volatility warning to warn about
code which accesses volatile structure members for
which different ABI specifications exist.
gcc/
* expr.c (warn_portable_volatility_p): New function.
(expand_assignment): call warn_portable_volatility_p.
(expand_real_expr_1): Likewise.
* fold-const.c (optimize_bit_field_compare): Do not optimize
on volatile data here, as it would defeat this warning later.
* stor-layout.c (layout_decl): Handle warn_portable_volatility
analogous to flag_strict_volatile_bitfileds, as it would
loose information and defeat this warning later.
* c-family/c.opt: Add -Wportable-volatility option.
* doc/invoke.texi: Add documentation about -Wportable-volatility.
gcc/testsuite/c-c++-common/
* Wportable-volatility-1.c: New testcase.
* Wportable-volatility-2.c: New testcase.
gcc-portable-volatility.diff
Description: Binary data
