diff mbox

Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

Message ID 20160219140330.GJ3017@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 19, 2016, 2:03 p.m. UTC 
Hi!

As described in the PR, in C++ we can have assignments
where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
including padding, but the FIELD_DECLs are artificial fields that have
narrower bit sizes.
store_field in this case takes the path of bit-field handling (even when
it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
because the rhs is expanded in that case through expand_normal, which
for a result type wider than the FIELD_DECL with forces it into a temporary.
In older GCCs that just generated inefficient code (copy the rhs into a
stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
Fixed by not taking the bit-field path in that case after verifying
we'll be able to expand it properly using the normal store_expr.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-02-19  Jakub Jelinek  <jakub@redhat.com>

	PR c++/69851
	* expr.c (store_field): Don't use bit-field path if exp is
	COMPONENT_REF with TREE_ADDRESSABLE type, where TYPE_SIZE is
	different from bitsize, but DECL_SIZE of FIELD_DECL is bitsize
	and the assignment can be performed by bitwise copy.  Formatting
	fix.

	* g++.dg/torture/pr69851.C: New test.


	Jakub

Comments

Jason Merrill Feb. 19, 2016, 6:30 p.m. UTC | #1 
On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
> As described in the PR, in C++ we can have assignments
> where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
> including padding, but the FIELD_DECLs are artificial fields that have
> narrower bit sizes.
> store_field in this case takes the path of bit-field handling (even when
> it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
> necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
> because the rhs is expanded in that case through expand_normal, which
> for a result type wider than the FIELD_DECL with forces it into a temporary.
> In older GCCs that just generated inefficient code (copy the rhs into a
> stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
> Fixed by not taking the bit-field path in that case after verifying
> we'll be able to expand it properly using the normal store_expr.

Won't store_expr clobber tail padding because it doesn't know about 
bitsize?  I would think that what we want is to use emit_block_move in 
the bit-field path whenever we're dealing with whole bytes in memory.

Jason
Jakub Jelinek Feb. 19, 2016, 6:41 p.m. UTC | #2 
On Fri, Feb 19, 2016 at 01:30:52PM -0500, Jason Merrill wrote:
> On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
> >As described in the PR, in C++ we can have assignments
> >where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
> >including padding, but the FIELD_DECLs are artificial fields that have
> >narrower bit sizes.
> >store_field in this case takes the path of bit-field handling (even when
> >it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
> >necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
> >because the rhs is expanded in that case through expand_normal, which
> >for a result type wider than the FIELD_DECL with forces it into a temporary.
> >In older GCCs that just generated inefficient code (copy the rhs into a
> >stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
> >Fixed by not taking the bit-field path in that case after verifying
> >we'll be able to expand it properly using the normal store_expr.
> 
> Won't store_expr clobber tail padding because it doesn't know about bitsize?

It doesn't clobber it, because it uses get_inner_reference, expands the
inner reference (which is necessarily for something TREE_ADDRESSABLE either
a MEM_REF or some decl that lives in memory), and get_inner_reference in
that case gives it the bitsize/bitpos from the FIELD_DECL.
Which is why in the patch I've posted there is the comparison of DECL_SIZE
of the FIELD_DECL against the bitsize that is passed to store_field.

> I would think that what we want is to use emit_block_move in the bit-field
> path whenever we're dealing with whole bytes in memory.

I'm afraid we'd need to duplicate too much code if we'd wanted to bypass all
the layers in there.

	Jakub
Jason Merrill Feb. 19, 2016, 7 p.m. UTC | #3 
On 02/19/2016 01:41 PM, Jakub Jelinek wrote:
> On Fri, Feb 19, 2016 at 01:30:52PM -0500, Jason Merrill wrote:
>> On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
>>> As described in the PR, in C++ we can have assignments
>>> where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
>>> including padding, but the FIELD_DECLs are artificial fields that have
>>> narrower bit sizes.
>>> store_field in this case takes the path of bit-field handling (even when
>>> it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
>>> necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
>>> because the rhs is expanded in that case through expand_normal, which
>>> for a result type wider than the FIELD_DECL with forces it into a temporary.
>>> In older GCCs that just generated inefficient code (copy the rhs into a
>>> stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
>>> Fixed by not taking the bit-field path in that case after verifying
>>> we'll be able to expand it properly using the normal store_expr.
>>
>> Won't store_expr clobber tail padding because it doesn't know about bitsize?
>
> It doesn't clobber it, because it uses get_inner_reference, expands the
> inner reference (which is necessarily for something TREE_ADDRESSABLE either
> a MEM_REF or some decl that lives in memory), and get_inner_reference in
> that case gives it the bitsize/bitpos from the FIELD_DECL.
> Which is why in the patch I've posted there is the comparison of DECL_SIZE
> of the FIELD_DECL against the bitsize that is passed to store_field.

Ah, that makes sense.  Please mention that in your added comment.

For GCC 7, can we drop the TREE_ADDRESSABLE check?

Jason
Jakub Jelinek Feb. 19, 2016, 7:07 p.m. UTC | #4 
On Fri, Feb 19, 2016 at 02:00:07PM -0500, Jason Merrill wrote:
> On 02/19/2016 01:41 PM, Jakub Jelinek wrote:
> >On Fri, Feb 19, 2016 at 01:30:52PM -0500, Jason Merrill wrote:
> >>On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
> >>>As described in the PR, in C++ we can have assignments
> >>>where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
> >>>including padding, but the FIELD_DECLs are artificial fields that have
> >>>narrower bit sizes.
> >>>store_field in this case takes the path of bit-field handling (even when
> >>>it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
> >>>necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
> >>>because the rhs is expanded in that case through expand_normal, which
> >>>for a result type wider than the FIELD_DECL with forces it into a temporary.
> >>>In older GCCs that just generated inefficient code (copy the rhs into a
> >>>stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
> >>>Fixed by not taking the bit-field path in that case after verifying
> >>>we'll be able to expand it properly using the normal store_expr.
> >>
> >>Won't store_expr clobber tail padding because it doesn't know about bitsize?
> >
> >It doesn't clobber it, because it uses get_inner_reference, expands the
> >inner reference (which is necessarily for something TREE_ADDRESSABLE either
> >a MEM_REF or some decl that lives in memory), and get_inner_reference in
> >that case gives it the bitsize/bitpos from the FIELD_DECL.
> >Which is why in the patch I've posted there is the comparison of DECL_SIZE
> >of the FIELD_DECL against the bitsize that is passed to store_field.
> 
> Ah, that makes sense.  Please mention that in your added comment.
> 
> For GCC 7, can we drop the TREE_ADDRESSABLE check?

I think we can't drop it, but we could replace it with a check that
get_inner_reference is something that must live in memory
(MEM_REF/TARGET_MEM_REF of SSA_NAME, or of decl that lives in memory,
or decl itself that lives in memory).

	Jakub
Jason Merrill Feb. 19, 2016, 7:11 p.m. UTC | #5 
On 02/19/2016 02:07 PM, Jakub Jelinek wrote:
> On Fri, Feb 19, 2016 at 02:00:07PM -0500, Jason Merrill wrote:
>> On 02/19/2016 01:41 PM, Jakub Jelinek wrote:
>>> On Fri, Feb 19, 2016 at 01:30:52PM -0500, Jason Merrill wrote:
>>>> On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
>>>>> As described in the PR, in C++ we can have assignments
>>>>> where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
>>>>> including padding, but the FIELD_DECLs are artificial fields that have
>>>>> narrower bit sizes.
>>>>> store_field in this case takes the path of bit-field handling (even when
>>>>> it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
>>>>> necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
>>>>> because the rhs is expanded in that case through expand_normal, which
>>>>> for a result type wider than the FIELD_DECL with forces it into a temporary.
>>>>> In older GCCs that just generated inefficient code (copy the rhs into a
>>>>> stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
>>>>> Fixed by not taking the bit-field path in that case after verifying
>>>>> we'll be able to expand it properly using the normal store_expr.
>>>>
>>>> Won't store_expr clobber tail padding because it doesn't know about bitsize?
>>>
>>> It doesn't clobber it, because it uses get_inner_reference, expands the
>>> inner reference (which is necessarily for something TREE_ADDRESSABLE either
>>> a MEM_REF or some decl that lives in memory), and get_inner_reference in
>>> that case gives it the bitsize/bitpos from the FIELD_DECL.
>>> Which is why in the patch I've posted there is the comparison of DECL_SIZE
>>> of the FIELD_DECL against the bitsize that is passed to store_field.
>>
>> Ah, that makes sense.  Please mention that in your added comment.
>>
>> For GCC 7, can we drop the TREE_ADDRESSABLE check?
>
> I think we can't drop it, but we could replace it with a check that
> get_inner_reference is something that must live in memory
> (MEM_REF/TARGET_MEM_REF of SSA_NAME, or of decl that lives in memory,
> or decl itself that lives in memory).

Please mention that in the comment, as well.  OK with those comment changes.

Jason
diff mbox

Patch

--- gcc/expr.c.jj	2016-02-12 00:50:55.000000000 +0100
+++ gcc/expr.c	2016-02-19 10:43:59.639162531 +0100
@@ -6643,14 +6643,24 @@  store_field (rtx target, HOST_WIDE_INT b
 	  /* Except for initialization of full bytes from a CONSTRUCTOR, which
 	     we will handle specially below.  */
 	  && !(TREE_CODE (exp) == CONSTRUCTOR
-	       && bitsize % BITS_PER_UNIT == 0))
+	       && bitsize % BITS_PER_UNIT == 0)
+	  /* And except for bitwise copying of TREE_ADDRESSABLE types,
+	     where the FIELD_DECL has the right bitsize, but TREE_TYPE (exp)
+	     includes some extra padding.  */
+	  && (!TREE_ADDRESSABLE (TREE_TYPE (exp))
+	      || TREE_CODE (exp) != COMPONENT_REF
+	      || TREE_CODE (DECL_SIZE (TREE_OPERAND (exp, 1))) != INTEGER_CST
+	      || (bitsize % BITS_PER_UNIT != 0)
+	      || (bitpos % BITS_PER_UNIT != 0)
+	      || (compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)), bitsize)
+		  != 0)))
       /* If we are expanding a MEM_REF of a non-BLKmode non-addressable
          decl we must use bitfield operations.  */
       || (bitsize >= 0
 	  && TREE_CODE (exp) == MEM_REF
 	  && TREE_CODE (TREE_OPERAND (exp, 0)) == ADDR_EXPR
 	  && DECL_P (TREE_OPERAND (TREE_OPERAND (exp, 0), 0))
-	  && !TREE_ADDRESSABLE (TREE_OPERAND (TREE_OPERAND (exp, 0),0 ))
+	  && !TREE_ADDRESSABLE (TREE_OPERAND (TREE_OPERAND (exp, 0), 0))
 	  && DECL_MODE (TREE_OPERAND (TREE_OPERAND (exp, 0), 0)) != BLKmode))
     {
       rtx temp;
--- gcc/testsuite/g++.dg/torture/pr69851.C.jj	2016-02-19 10:59:22.224438830 +0100
+++ gcc/testsuite/g++.dg/torture/pr69851.C	2016-02-19 10:59:12.000000000 +0100
@@ -0,0 +1,24 @@ 
+// PR c++/69851
+// { dg-do compile }
+// { dg-options "-std=c++11" }
+
+template <typename T>
+struct A { T a; };
+template <unsigned long, typename...>
+struct B;
+template <unsigned long N, typename T, typename... U>
+struct B<N, T, U...> : B<1, U...>, A<T>
+{
+  B (B &) = default;
+  B (B &&x) : B(x) {}
+};
+template <unsigned long N, typename T>
+struct B<N, T> {};
+struct C { C (C &); };
+struct D {};
+
+void
+foo (B<0, C, D, int, int> a)
+{
+  B<0, C, D, int, int> b (a);
+}