diff mbox

[C++/70393] constexpr constructor

Message ID 56FA77A6.8040408@acm.org
State New
Headers show

Commit Message

Nathan Sidwell March 29, 2016, 12:40 p.m. UTC 
This patch fixes 70393  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70393

'ab's construction used to be dynamic but becomes static with C++11 constexpr 
constructors (I'm not sure whether we're doing more than the std requires, but 
that's not important).  However, 'AB's bases are not laid out in program 
declaration order.  B is chosen as the primary base, and A is placed after that.

But the constructor creates A first and then B.  We're simply appending new 
elements onto AB's CONSTRUCTOR, and that's not something varasm is prepared to 
deal with.

First, I add an assert to output_constructor_regular_field so the silent bad 
code generation turns into an ICE.  This discovered the problem in 
g++.dg/cpp0x/constexpr-virtual[34].C testcases too.  I  moved the flushing of 
bitfields earlier so as to move the offset calculation nearer its use (and to 
permit some jump threading in output_constructor_regular_field's code).

Second, the fix itself is in cxx_eval_store_expression.  Currently we scan the 
CONSTRUCTOR ELTS to see if we've met this field before, otherwise we append.  I 
modified the scanning loop to also iterate over the FIELD_DECLS of the record 
itself, and detect when we find the wanted field in the struct before the 
current CTOR ELT.  Then we do an insert at the current point.  (inserting at the 
end is valid, btw).

I did consider just checking 'int_byte_position (local->field) == 0' to detect 
the primary base case, but that struck me as rather fragile, and as constexprs 
get more powerful, likely to break for cases with virtual bases.  (right now 
virtual bases still cause dynamic initialization)

built & tested on x86_64-linux

nathan

Comments

Jason Merrill March 30, 2016, 5:22 p.m. UTC | #1 
On 03/29/2016 08:40 AM, Nathan Sidwell wrote:
> +	      /* The field we're initializing must be on the field
> +		 list.  Look to see if it is present before the
> +		 field the current ELT initializes.  */
> +	      for (; fields != cep->index; fields = DECL_CHAIN (fields))
> +		if (index == fields)
> +		  goto insert;

Instead of searching through the fields, could we compare 
DECL_FIELD_OFFSET of index and cep->index?

Jason
Nathan Sidwell March 30, 2016, 5:40 p.m. UTC | #2 
On 03/30/16 13:22, Jason Merrill wrote:
> On 03/29/2016 08:40 AM, Nathan Sidwell wrote:
>> +          /* The field we're initializing must be on the field
>> +         list.  Look to see if it is present before the
>> +         field the current ELT initializes.  */
>> +          for (; fields != cep->index; fields = DECL_CHAIN (fields))
>> +        if (index == fields)
>> +          goto insert;
>
> Instead of searching through the fields, could we compare DECL_FIELD_OFFSET of
> index and cep->index?

I wondered about that, but then there's  bitfields (and I can't recall how they 
behave WRT DECL_FIELD_OFFSET).  pointer equality seems cheap enough?

nathan
Jason Merrill March 30, 2016, 7:28 p.m. UTC | #3 
On 03/30/2016 01:40 PM, Nathan Sidwell wrote:
> On 03/30/16 13:22, Jason Merrill wrote:
>> On 03/29/2016 08:40 AM, Nathan Sidwell wrote:
>>> +          /* The field we're initializing must be on the field
>>> +         list.  Look to see if it is present before the
>>> +         field the current ELT initializes.  */
>>> +          for (; fields != cep->index; fields = DECL_CHAIN (fields))
>>> +        if (index == fields)
>>> +          goto insert;
>>
>> Instead of searching through the fields, could we compare
>> DECL_FIELD_OFFSET of
>> index and cep->index?
>
> I wondered about that, but then there's  bitfields (and I can't recall
> how they behave WRT DECL_FIELD_OFFSET).  pointer equality seems cheap
> enough?

Fair enough.  OK.

Jason
diff mbox

Patch

2016-03-29  Nathan Sidwell  <nathan@acm.org>

	PR c++/70393
	* varasm.c (output_constructor_regular_field): Flush bitfield
	earlier.  Assert we don't want to move backwards.

	gcc/
	PR c++/70393
	* constexpr.c (cxx_eval_store_expression): Keep CONSTRUCTOR
	elements in field order.

	PR c++/70393
	* g++.dg/cpp0x/constexpr-virtual6.C: New.

Index: varasm.c
===================================================================
--- varasm.c	(revision 234503)
+++ varasm.c	(working copy)
@@ -4929,6 +4929,14 @@  output_constructor_regular_field (oc_loc
 
   unsigned int align2;
 
+  /* Output any buffered-up bit-fields preceding this element.  */
+  if (local->byte_buffer_in_use)
+    {
+      assemble_integer (GEN_INT (local->byte), 1, BITS_PER_UNIT, 1);
+      local->total_bytes++;
+      local->byte_buffer_in_use = false;
+    }
+
   if (local->index != NULL_TREE)
     {
       /* Perform the index calculation in modulo arithmetic but
@@ -4945,22 +4953,19 @@  output_constructor_regular_field (oc_loc
   else
     fieldpos = 0;
 
-  /* Output any buffered-up bit-fields preceding this element.  */
-  if (local->byte_buffer_in_use)
-    {
-      assemble_integer (GEN_INT (local->byte), 1, BITS_PER_UNIT, 1);
-      local->total_bytes++;
-      local->byte_buffer_in_use = false;
-    }
-
   /* Advance to offset of this element.
      Note no alignment needed in an array, since that is guaranteed
      if each element has the proper size.  */
-  if ((local->field != NULL_TREE || local->index != NULL_TREE)
-      && fieldpos > local->total_bytes)
+  if (local->field != NULL_TREE || local->index != NULL_TREE)
     {
-      assemble_zeros (fieldpos - local->total_bytes);
-      local->total_bytes = fieldpos;
+      if (fieldpos > local->total_bytes)
+	{
+	  assemble_zeros (fieldpos - local->total_bytes);
+	  local->total_bytes = fieldpos;
+	}
+      else
+	/* Must not go backwards.  */
+	gcc_assert (fieldpos == local->total_bytes);
     }
 
   /* Find the alignment of this element.  */
Index: cp/constexpr.c
===================================================================
--- cp/constexpr.c	(revision 234503)
+++ cp/constexpr.c	(working copy)
@@ -2959,16 +2959,39 @@  cxx_eval_store_expression (const constex
       else
 	{
 	  gcc_assert (TREE_CODE (index) == FIELD_DECL);
-	  for (unsigned HOST_WIDE_INT idx = 0;
+
+	  /* We must keep the CONSTRUCTOR's ELTS in FIELD order.
+	     Usually we meet initializers in that order, but it is
+	     possible for base types to be placed not in program
+	     order.  */
+	  tree fields = TYPE_FIELDS (DECL_CONTEXT (index));
+	  unsigned HOST_WIDE_INT idx;
+
+	  for (idx = 0;
 	       vec_safe_iterate (CONSTRUCTOR_ELTS (*valp), idx, &cep);
 	       idx++)
-	    if (index == cep->index)
-	      break;
-	  if (!cep)
 	    {
-	      constructor_elt ce = { index, NULL_TREE };
-	      cep = vec_safe_push (CONSTRUCTOR_ELTS (*valp), ce);
+	      if (index == cep->index)
+		goto found;
+
+	      /* The field we're initializing must be on the field
+		 list.  Look to see if it is present before the
+		 field the current ELT initializes.  */
+	      for (; fields != cep->index; fields = DECL_CHAIN (fields))
+		if (index == fields)
+		  goto insert;
 	    }
+
+	  /* We fell off the end of the CONSTRUCTOR, so insert a new
+	     entry at the end.  */
+	insert:
+	  {
+	    constructor_elt ce = { index, NULL_TREE };
+
+	    vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
+	    cep = CONSTRUCTOR_ELT (*valp, idx);
+	  }
+	found:;
 	}
       valp = &cep->value;
     }
Index: testsuite/g++.dg/cpp0x/constexpr-virtual6.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-virtual6.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/constexpr-virtual6.C	(working copy)
@@ -0,0 +1,49 @@ 
+// PR c++/70393
+// { dg-do run { target c++11 } }
+
+/* 'ab' has a static initializer, but we flubbed the initializer,
+   because of B being the primary base.  */
+
+struct A
+{
+  int a = 1;
+};
+
+struct B
+{
+  B *element = (B*)2;
+
+    virtual int vfunc() = 0;
+
+    int call_element()
+    {
+      return element->vfunc();
+    }
+
+    void set_element()
+    {
+      element = this;
+    }
+};
+
+struct AB : public A, public B
+{
+    int vfunc()
+    {
+      return 0;
+    }
+};
+
+static AB ab;
+
+int main()
+{
+  if (ab.a != 1)
+    return 1;
+  if (ab.element != (void*)2)
+    return 2;
+  
+  ab.set_element();
+  return ab.call_element();
+}
+