| Message ID | 51BE0D1C.9070404@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote: > This patch fixes the PR23623 regression. In conjunction with part 2 > of the series, it also fixes the new volatile-bitfields-3.c test > case. > > As I noted in previous discussion, there might be a better place to > accomplish this effect, but hacking DECL_BIT_FIELD_REPRESENTATIVE > can't work because the volatile-ness may be coming from a qualifier > on the pointer or object from which the field is being extracted, > rather than from a volatile qualifier on the bit field decl. I > think the choices are to do it in get_bit_range (as in this patch), > in the callers of get_bit_range, or at the places where the bit > range information is being used. So does this means you just always violate C++11 memory model requirements with -fstrict-volatile-bitfields? That doesn't seem to be a good idea. > 2013-06-16 Sandra Loosemore <sandra@codesourcery.com> > > PR middle-end/23623 > > gcc/ > * expr.c (get_bit_range): Handle flag_strict_volatile_bitfields. Jakub
On Sun, Jun 16, 2013 at 9:26 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote: >> This patch fixes the PR23623 regression. In conjunction with part 2 >> of the series, it also fixes the new volatile-bitfields-3.c test >> case. >> >> As I noted in previous discussion, there might be a better place to >> accomplish this effect, but hacking DECL_BIT_FIELD_REPRESENTATIVE >> can't work because the volatile-ness may be coming from a qualifier >> on the pointer or object from which the field is being extracted, >> rather than from a volatile qualifier on the bit field decl. I >> think the choices are to do it in get_bit_range (as in this patch), >> in the callers of get_bit_range, or at the places where the bit >> range information is being used. > > So does this means you just always violate C++11 memory model requirements > with -fstrict-volatile-bitfields? That doesn't seem to be a good idea. Yeah, it's not clear to me that this patch fixes anything by design. It mainly changes things from limiting the access in some way to not limiting it at all ... Richard. >> 2013-06-16 Sandra Loosemore <sandra@codesourcery.com> >> >> PR middle-end/23623 >> >> gcc/ >> * expr.c (get_bit_range): Handle flag_strict_volatile_bitfields. > > Jakub
On Mon, 17 Jun 2013 13:12:09 +0200 Richard Biener <richard.guenther@gmail.com> wrote: > On Sun, Jun 16, 2013 at 9:26 PM, Jakub Jelinek <jakub@redhat.com> > wrote: > > On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote: > >> This patch fixes the PR23623 regression. In conjunction with part > >> 2 of the series, it also fixes the new volatile-bitfields-3.c test > >> case. > >> > >> As I noted in previous discussion, there might be a better place to > >> accomplish this effect, but hacking DECL_BIT_FIELD_REPRESENTATIVE > >> can't work because the volatile-ness may be coming from a qualifier > >> on the pointer or object from which the field is being extracted, > >> rather than from a volatile qualifier on the bit field decl. I > >> think the choices are to do it in get_bit_range (as in this patch), > >> in the callers of get_bit_range, or at the places where the bit > >> range information is being used. > > > > So does this means you just always violate C++11 memory model > > requirements with -fstrict-volatile-bitfields? That doesn't seem > > to be a good idea. > > Yeah, it's not clear to me that this patch fixes anything by design. > It mainly changes things from limiting the access in some way to not > limiting it at all ... IIUC, the incompatibility between the specified -fstrict-volatile-bitfields behaviour and the C++ memory model is a recognised deficiency in the ARM EABI. It might be an unpopular suggestion, but how about disabling the option by default for C++ on ARM [*]? Maybe even with a "sorry" if the user tries to turn it on manually? The assumption behind that idea is that people who most care about the EABI-specified behaviour (to access hardware registers, etc.) are probably using bare metal and plain C. The downside is the potential for "surprise" for users though, I suppose. Julian [*] Or some other variant target-dependent hack, like depending on -ansi, bare-metal vs. $operating system, etc....
On Mon, Jun 17, 2013 at 1:31 PM, Julian Brown <julian@codesourcery.com> wrote: > On Mon, 17 Jun 2013 13:12:09 +0200 > Richard Biener <richard.guenther@gmail.com> wrote: > >> On Sun, Jun 16, 2013 at 9:26 PM, Jakub Jelinek <jakub@redhat.com> >> wrote: >> > On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote: >> >> This patch fixes the PR23623 regression. In conjunction with part >> >> 2 of the series, it also fixes the new volatile-bitfields-3.c test >> >> case. >> >> >> >> As I noted in previous discussion, there might be a better place to >> >> accomplish this effect, but hacking DECL_BIT_FIELD_REPRESENTATIVE >> >> can't work because the volatile-ness may be coming from a qualifier >> >> on the pointer or object from which the field is being extracted, >> >> rather than from a volatile qualifier on the bit field decl. I >> >> think the choices are to do it in get_bit_range (as in this patch), >> >> in the callers of get_bit_range, or at the places where the bit >> >> range information is being used. >> > >> > So does this means you just always violate C++11 memory model >> > requirements with -fstrict-volatile-bitfields? That doesn't seem >> > to be a good idea. >> >> Yeah, it's not clear to me that this patch fixes anything by design. >> It mainly changes things from limiting the access in some way to not >> limiting it at all ... > > IIUC, the incompatibility between the specified > -fstrict-volatile-bitfields behaviour and the C++ memory model is a > recognised deficiency in the ARM EABI. It might be an unpopular > suggestion, but how about disabling the option by default for C++ on > ARM [*]? Maybe even with a "sorry" if the user tries to turn it on > manually? How are they incompatible? As far as I understand the -fstrict-volatile-bitfields at most restricts the size of the access further, no? Can you write down an example that shows the incompatibility? (woudl be nice to see that as comment before the relevant code). Richard. > The assumption behind that idea is that people who most care about the > EABI-specified behaviour (to access hardware registers, etc.) are > probably using bare metal and plain C. The downside is the potential > for "surprise" for users though, I suppose. > > Julian > > [*] Or some other variant target-dependent hack, like depending on > -ansi, bare-metal vs. $operating system, etc....
On Mon, 17 Jun 2013 13:38:05 +0200 Richard Biener <richard.guenther@gmail.com> wrote: > On Mon, Jun 17, 2013 at 1:31 PM, Julian Brown > <julian@codesourcery.com> wrote: > > On Mon, 17 Jun 2013 13:12:09 +0200 > > Richard Biener <richard.guenther@gmail.com> wrote: > > > >> On Sun, Jun 16, 2013 at 9:26 PM, Jakub Jelinek <jakub@redhat.com> > >> wrote: > >> > On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote: > >> >> This patch fixes the PR23623 regression. In conjunction with > >> >> part 2 of the series, it also fixes the new > >> >> volatile-bitfields-3.c test case. > >> >> > >> >> As I noted in previous discussion, there might be a better > >> >> place to accomplish this effect, but hacking > >> >> DECL_BIT_FIELD_REPRESENTATIVE can't work because the > >> >> volatile-ness may be coming from a qualifier on the pointer or > >> >> object from which the field is being extracted, rather than > >> >> from a volatile qualifier on the bit field decl. I think the > >> >> choices are to do it in get_bit_range (as in this patch), in > >> >> the callers of get_bit_range, or at the places where the bit > >> >> range information is being used. > >> > > >> > So does this means you just always violate C++11 memory model > >> > requirements with -fstrict-volatile-bitfields? That doesn't seem > >> > to be a good idea. > >> > >> Yeah, it's not clear to me that this patch fixes anything by > >> design. It mainly changes things from limiting the access in some > >> way to not limiting it at all ... > > > > IIUC, the incompatibility between the specified > > -fstrict-volatile-bitfields behaviour and the C++ memory model is a > > recognised deficiency in the ARM EABI. It might be an unpopular > > suggestion, but how about disabling the option by default for C++ on > > ARM [*]? Maybe even with a "sorry" if the user tries to turn it on > > manually? > > How are they incompatible? As far as I understand the > -fstrict-volatile-bitfields > at most restricts the size of the access further, no? Can you write > down an example that shows the incompatibility? (woudl be nice to > see that as comment before the relevant code). Well -- I'm certainly no expert on the C++ memory model, but I am under the impression (that I can't seem to verify by googling ;-)) that accesses to adjacent bitfields during volatile access of a particular bitfield are forbidden. So simply, for the following: struct foo { int a : 8; int b : 8; int c : 16; }; volatile struct foo x; void bar (void) { x.b++; } to satisfy the ARM EABI, 'bar' will access all three of a, b and c using read-modify-write (using int-sized accesses). IIUC for the C++ memory model, only 'b' may be accessed, e.g. using byte-sized loads/stores. I'm quite prepared to be wrong though, in which case sorry for the noise :-). Julian
On Mon, Jun 17, 2013 at 01:27:38PM +0100, Julian Brown wrote: > Well -- I'm certainly no expert on the C++ memory model, but I am under > the impression (that I can't seem to verify by googling ;-)) that > accesses to adjacent bitfields during volatile access of a particular > bitfield are forbidden. So simply, for the following: > > struct foo { > int a : 8; > int b : 8; > int c : 16; > }; > > volatile struct foo x; > > void bar (void) { x.b++; } I believe in the above it is ok in C++ memory model if the RMW cycle is using 32-bit type, but in struct foo { int a : 8; int b : 8; char c, d; }; volatile struct foo x; void bar (void) { x.b++; } it is not (but it is laid out the same), because modification to x.a or x.b must not create data races on x.c and/or x.d. Jakub
On 06/17/2013 02:27 PM, Julian Brown wrote: > On Mon, 17 Jun 2013 13:38:05 +0200 > Richard Biener <richard.guenther@gmail.com> wrote: > >> On Mon, Jun 17, 2013 at 1:31 PM, Julian Brown >> <julian@codesourcery.com> wrote: >>> IIUC, the incompatibility between the specified >>> -fstrict-volatile-bitfields behaviour and the C++ memory model is a >>> recognised deficiency in the ARM EABI. It might be an unpopular >>> suggestion, but how about disabling the option by default for C++ on >>> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on >>> manually? >> >> How are they incompatible? As far as I understand the >> -fstrict-volatile-bitfields >> at most restricts the size of the access further, no? Can you write >> down an example that shows the incompatibility? (woudl be nice to >> see that as comment before the relevant code). > > Well -- I'm certainly no expert on the C++ memory model, but I am under > the impression (that I can't seem to verify by googling ;-)) that > accesses to adjacent bitfields during volatile access of a particular > bitfield are forbidden. So simply, for the following: > > struct foo { > int a : 8; > int b : 8; > int c : 16; > }; > > volatile struct foo x; > > void bar (void) { x.b++; } > > to satisfy the ARM EABI, 'bar' will access all three of a, b and c > using read-modify-write (using int-sized accesses). IIUC for the C++ > memory model, only 'b' may be accessed, e.g. using byte-sized > loads/stores. The one I came up with after reading about the C++ memory model spec is struct x { int x: 8; char : 0; short y : 8; } z; I interpret the C++ rules to say that due to the zero-sized bitfield, x and y are different memory locations, and modifying one shouldn't affect the other. However, with -fstrict-volatile-bitfields, x must be accessed as an int, so it overlaps y. > I'm quite prepared to be wrong though, in which case sorry for the > noise :-). Same here :) Bernd
On Mon, Jun 17, 2013 at 2:38 PM, Bernd Schmidt <bernds@codesourcery.com> wrote: > On 06/17/2013 02:27 PM, Julian Brown wrote: >> On Mon, 17 Jun 2013 13:38:05 +0200 >> Richard Biener <richard.guenther@gmail.com> wrote: >> >>> On Mon, Jun 17, 2013 at 1:31 PM, Julian Brown >>> <julian@codesourcery.com> wrote: >>>> IIUC, the incompatibility between the specified >>>> -fstrict-volatile-bitfields behaviour and the C++ memory model is a >>>> recognised deficiency in the ARM EABI. It might be an unpopular >>>> suggestion, but how about disabling the option by default for C++ on >>>> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on >>>> manually? >>> >>> How are they incompatible? As far as I understand the >>> -fstrict-volatile-bitfields >>> at most restricts the size of the access further, no? Can you write >>> down an example that shows the incompatibility? (woudl be nice to >>> see that as comment before the relevant code). >> >> Well -- I'm certainly no expert on the C++ memory model, but I am under >> the impression (that I can't seem to verify by googling ;-)) that >> accesses to adjacent bitfields during volatile access of a particular >> bitfield are forbidden. So simply, for the following: >> >> struct foo { >> int a : 8; >> int b : 8; >> int c : 16; >> }; >> >> volatile struct foo x; >> >> void bar (void) { x.b++; } >> >> to satisfy the ARM EABI, 'bar' will access all three of a, b and c >> using read-modify-write (using int-sized accesses). IIUC for the C++ >> memory model, only 'b' may be accessed, e.g. using byte-sized >> loads/stores. > > The one I came up with after reading about the C++ memory model spec is > struct x > { > int x: 8; > char : 0; > short y : 8; > } z; > > I interpret the C++ rules to say that due to the zero-sized bitfield, x > and y are different memory locations, and modifying one shouldn't affect > the other. True. IIRC the char : 0 isn't even necessary for the limitation to take place (just the different underlying types is enough). > However, with -fstrict-volatile-bitfields, x must be accessed > as an int, so it overlaps y. Ok, that's a good example then. Looking at the mode of the FIELD_DECL for x isn't a good way to query the access mode then, but the mode of DECL_BIT_FIELD_TYPE would. Note that for z.x you are not guaranteed to end up with a COMPONENT_REF at RTL expansion time at all but it may be lowered to a MEM_REF by arbitrary optimization passes (unlikely because most simply go away when they see a volatile access), because it's bitsize is a multiple of BITS_PER_UNIT and it's boundary is byte-aligned. Richard. >> I'm quite prepared to be wrong though, in which case sorry for the >> noise :-). > > Same here :) > > > Bernd >
On Mon, 17 Jun 2013, Julian Brown wrote: > IIUC, the incompatibility between the specified > -fstrict-volatile-bitfields behaviour and the C++ memory model is a > recognised deficiency in the ARM EABI. It might be an unpopular > suggestion, but how about disabling the option by default for C++ on > ARM [*]? Maybe even with a "sorry" if the user tries to turn it on > manually? The C11 and C++11 memory models are the same, this is nothing to do with C++ as opposed to C. Someone from ARM will need to answer as to what their plans are to make AAPCS compatible with the memory model. I'd say strict-volatile-bitfields should only ever apply in cases where it does not conflict with language semantics - so just as "packed" should take precedence, so the memory model should also take precedence (in the absence of --param allow-store-data-races=1, anyway) in those cases where AAPCS would indicate writing to an object outside the maximal sequence of consecutive non-zero-width bit-fields.
On 06/17/2013 08:41 AM, Joseph S. Myers wrote: > On Mon, 17 Jun 2013, Julian Brown wrote: > >> IIUC, the incompatibility between the specified >> -fstrict-volatile-bitfields behaviour and the C++ memory model is a >> recognised deficiency in the ARM EABI. It might be an unpopular >> suggestion, but how about disabling the option by default for C++ on >> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on >> manually? > > The C11 and C++11 memory models are the same, this is nothing to do with > C++ as opposed to C. > > Someone from ARM will need to answer as to what their plans are to make > AAPCS compatible with the memory model. I'd say strict-volatile-bitfields > should only ever apply in cases where it does not conflict with language > semantics - so just as "packed" should take precedence, so the memory > model should also take precedence (in the absence of --param > allow-store-data-races=1, anyway) in those cases where AAPCS would > indicate writing to an object outside the maximal sequence of consecutive > non-zero-width bit-fields. The problem with always choosing C++ memory model compliance over ABI compliance, no matter what the setting of -fstrict-volatile-bitfields, is that some users may have legacy code out there where they really want the ABI-compliant behavior. Perhaps we should not make -fstrict-volatile-bitfields not be the default on any target any more, but if you supply it explicitly it tells GCC that you really want the ABI-compliant behavior to override the language-standard-compliant behavior? I don't think there's an actual conflict there between -fstrict-volatile-bitfields and packed structure extensions. AAPCS allows for the possibility of packed structures but says conforming programs don't use them for external interfaces (this is the note at the end of 7.1.7.1). As I noted previously, my understanding is that the volatile access requirements in the AAPCS only apply to non-packed struct fields whose layout and alignment is fully specified by AAPCS. The various packed-structure bugs addressed by part 4 of my patch series are mostly cases where the required single access *cannot* be emitted because the field is not aligned; instead, we're emitting code that is totally wrong even under GCC's normal access rules, because it faults at runtime or only accesses part of the bit-field. -Sandra
Earlier today I wrote: > On 06/17/2013 08:41 AM, Joseph S. Myers wrote: >> On Mon, 17 Jun 2013, Julian Brown wrote: >> >>> IIUC, the incompatibility between the specified >>> -fstrict-volatile-bitfields behaviour and the C++ memory model is a >>> recognised deficiency in the ARM EABI. It might be an unpopular >>> suggestion, but how about disabling the option by default for C++ on >>> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on >>> manually? >> >> The C11 and C++11 memory models are the same, this is nothing to do with >> C++ as opposed to C. > > The problem with always choosing C++ memory model compliance over ABI > compliance, no matter what the setting of -fstrict-volatile-bitfields, > is that some users may have legacy code out there where they really want > the ABI-compliant behavior. Perhaps we should not make > -fstrict-volatile-bitfields not be the default on any target any more, > but if you supply it explicitly it tells GCC that you really want the > ABI-compliant behavior to override the language-standard-compliant > behavior? I had another thought: perhaps -fstrict-volatile-bitfields could remain the default on targets where it currently is, but it can be overridden by an appropriate -std= option. Perhaps also GCC could give an error if -fstrict-volatile-bitfields is given explicitly with an incompatible -std= option. -Sandra
Index: gcc/expr.c =================================================================== --- gcc/expr.c (revision 199963) +++ gcc/expr.c (working copy) @@ -4508,6 +4508,16 @@ get_bit_range (unsigned HOST_WIDE_INT *b gcc_assert (TREE_CODE (exp) == COMPONENT_REF); + /* If -fstrict-volatile-bitfields was given and this is a volatile + access, then we must ignore any DECL_BIT_FIELD_REPRESENTATIVE and + do the access in the mode of the field. */ + if (TREE_THIS_VOLATILE (exp) + && flag_strict_volatile_bitfields > 0) + { + *bitstart = *bitend = 0; + return; + } + field = TREE_OPERAND (exp, 1); repr = DECL_BIT_FIELD_REPRESENTATIVE (field); /* If we do not have a DECL_BIT_FIELD_REPRESENTATIVE there is no