Patchwork [cxx-mem-model] bitfield tests

login
register
mail settings
Submitter Aldy Hernandez
Date March 29, 2011, 5 p.m.
Message ID <4D92103E.90100@redhat.com>
Download mbox | patch
Permalink /patch/88814/
State New
Headers show

Comments

Aldy Hernandez - March 29, 2011, 5 p.m.
[Language lawyers, please correct me if I have mis-interpreted the 
upcoming standard in any way.]

In the C++ memory model, contiguous bitfields comprise a single memory 
location, so it's fair game to bit twiddle them when setting them.  For 
example:

	struct {
		unsigned int a : 4;
		unsigned int b : 4;
		unsigned int c : 4;
	};

In the above example, you can touch <b> and <c> while setting <a>.  No 
race there.

However, non contiguous bitfields are a different story:

	struct {
		unsigned int a : 4;
		char b;
		unsigned int c : 6;
	};

Here we have 3 distinct memory locations, so you can't touch <b> or <c> 
while setting <a>.  No bit twiddling allowed.

Similarly for bitfields separated by a zero-length bitfield:

	struct {
		unsigned int a : 4;
		int : 0;
		unsigned int c : 6;
	};

In the above example, <a> and <c> are distinct memory locations.

Also, a structure/union boundary will also separate previously 
contiguous bit sequences:

	struct {
		unsigned int a : 4;
		struct { unsigned int b : 4 } BBB;
		unsigned int c : 4;
	};

Here we have 3 distinct memory locations, so again, we can't clobber <b> 
or <c> while setting <a>.

The patch below adds a non-contiguous bit test (bitfields-2.C) which 
passes on x86, but upon assembly inspection, fails on PPC64, s390, and 
Alpha.  These 3 architectures bit-twiddle their way out of the problem.

There is also a similar test already in the testsuite (bitfields.C) 
which is similar except one field is a volatile.  This test fails on 
x86-64 as well and is the subject of PR48128 which Jakub is currently 
tackling.

As soon as Jakub finishes with PR48128, I will be working on getting 
these bitfield tests working in a C++ memory model fashion.

Committing to branch.
* gcc.dg/memmodel/subfields.c (set_a): Set noinline attribute.
	* g++.dg/memmodel/bitfields.C (set_a): Same.
	* g++.dg/memmodel/bitfields-2.C: New.
Richard Guenther - March 30, 2011, 9:46 a.m.
On Tue, Mar 29, 2011 at 7:00 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> [Language lawyers, please correct me if I have mis-interpreted the upcoming
> standard in any way.]
>
> In the C++ memory model, contiguous bitfields comprise a single memory
> location, so it's fair game to bit twiddle them when setting them.  For
> example:
>
>        struct {
>                unsigned int a : 4;
>                unsigned int b : 4;
>                unsigned int c : 4;
>        };
>
> In the above example, you can touch <b> and <c> while setting <a>.  No race
> there.
>
> However, non contiguous bitfields are a different story:
>
>        struct {
>                unsigned int a : 4;
>                char b;
>                unsigned int c : 6;
>        };
>
> Here we have 3 distinct memory locations, so you can't touch <b> or <c>
> while setting <a>.  No bit twiddling allowed.
>
> Similarly for bitfields separated by a zero-length bitfield:
>
>        struct {
>                unsigned int a : 4;
>                int : 0;
>                unsigned int c : 6;
>        };
>
> In the above example, <a> and <c> are distinct memory locations.
>
> Also, a structure/union boundary will also separate previously contiguous
> bit sequences:
>
>        struct {
>                unsigned int a : 4;
>                struct { unsigned int b : 4 } BBB;
>                unsigned int c : 4;
>        };
>
> Here we have 3 distinct memory locations, so again, we can't clobber <b> or
> <c> while setting <a>.
>
> The patch below adds a non-contiguous bit test (bitfields-2.C) which passes
> on x86, but upon assembly inspection, fails on PPC64, s390, and Alpha.
>  These 3 architectures bit-twiddle their way out of the problem.

The memory model is not implementable on strict-alignment targets
that do not have a byte store operation.  But we previously said that ;)

Also consider global vars

char a;
char b;

accessing them on strict-align targets may access adjacent globals
(that's a problem anyway, also with alias analysis).

Richard.

> There is also a similar test already in the testsuite (bitfields.C) which is
> similar except one field is a volatile.  This test fails on x86-64 as well
> and is the subject of PR48128 which Jakub is currently tackling.
>
> As soon as Jakub finishes with PR48128, I will be working on getting these
> bitfield tests working in a C++ memory model fashion.
>
> Committing to branch.
>

Patch

Index: gcc.dg/memmodel/subfields.c
===================================================================
--- gcc.dg/memmodel/subfields.c	(revision 170937)
+++ gcc.dg/memmodel/subfields.c	(working copy)
@@ -20,6 +20,7 @@  struct test_struct {
    not affect any of the other fields in the structure.  An improper
    implementation may load an entire word, change the 8 bits for field
    'a' and write the entire word back out. */
+__attribute__((noinline))
 void set_a(char x)
 {
   var.a = x;
Index: g++.dg/memmodel/bitfields-2.C
===================================================================
--- g++.dg/memmodel/bitfields-2.C	(revision 0)
+++ g++.dg/memmodel/bitfields-2.C	(revision 0)
@@ -0,0 +1,71 @@ 
+/* { dg-do link } */
+/* { dg-options "-O2 --param allow-load-data-races=0 --param allow-store-data-races=0" } */
+/* { dg-final { memmodel-gdb-test } } */
+
+/* Test that setting <var.a> does not touch either <var.b> or <var.c>.
+   In the C++ memory model, non contiguous bitfields ("a" and "c"
+   here) should be considered as distinct memory locations, so we
+   can't use bit twiddling to set either one.  */
+
+#include <stdio.h>
+#include "memmodel.h"
+
+#define CONSTA 12
+
+static int global;
+struct S
+{
+  unsigned int a : 4;
+  unsigned char b;
+  unsigned int c : 6;
+} var;
+
+__attribute__((noinline))
+void set_a()
+{
+  var.a = CONSTA;
+}
+
+void memmodel_other_threads()
+{
+  ++global;
+  var.b = global;
+  var.c = global;
+}
+
+int memmodel_step_verify()
+{
+  int ret = 0;
+  if (var.b != global)
+    {
+      printf ("FAIL: Unexpected value: var.b is %d, should be %d\n",
+	      var.b, global);
+      ret = 1;
+    }
+  if (var.c != global)
+    {
+      printf ("FAIL: Unexpected value: var.c is %d, should be %d\n",
+	      var.c, global);
+      ret = 1;
+    }
+  return ret;
+}
+
+int memmodel_final_verify()
+{
+  int ret = memmodel_step_verify();
+  if (var.a != CONSTA)
+    {
+      printf ("FAIL: Unexpected value: var.a is %d, should be %d\n",
+	      var.a, CONSTA);
+      ret = 1;
+    }
+  return ret;
+}
+
+int main()
+{
+  set_a();
+  memmodel_done();
+  return 0;
+}
Index: g++.dg/memmodel/bitfields.C
===================================================================
--- g++.dg/memmodel/bitfields.C	(revision 171248)
+++ g++.dg/memmodel/bitfields.C	(working copy)
@@ -23,6 +23,7 @@  struct S
   unsigned int c : 6;
 } var;
 
+__attribute__((noinline))
 void set_a()
 {
   var.a = CONSTA;