[v2] c++: Properly detect calls to digest_init in build_vec_init [PR114619]

Message ID 01020194cc001f33-5b7f7c43-e784-49bd-bcfb-099c22eefe95-000000@eu-west-1.amazonses.com
State New
Headers
Series [v2] c++: Properly detect calls to digest_init in build_vec_init [PR114619] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed

Commit Message

Simon Martin Feb. 3, 2025, 1:29 p.m. UTC
  Hi Jason,

On 16 Jan 2025, at 23:28, Jason Merrill wrote:

> On 10/19/24 5:09 AM, Simon Martin wrote:
>> We currently ICE in checking mode with cxx_dialect < 17 on the 
>> following
>> valid code
>>
>> === cut here ===
>> struct X {
>>    X(const X&) {}
>> };
>> extern X x;
>> void foo () {
>>    new X[1]{x};
>> }
>> === cut here ===
>>
>> The problem is that cp_gimplify_expr gcc_checking_asserts that a
>> TARGET_EXPR is not TARGET_EXPR_ELIDING_P (or cannot be elided), while 
>> in
>> this case with cxx_dialect < 17, it is TARGET_EXPR_ELIDING_P but we 
>> have
>> not even tried to elide.
>>
>> This patch relaxes that gcc_checking_assert to not fail when using
>> cxx_dialect < 17 and -fno-elide-constructors (I considered being more

>> clever at setting TARGET_EXPR_ELIDING_P appropriately but it looks 

>> more
>> risky and not worth the extra complexity for a checking assert).
>
> The problem is that in that case we end up with two copy constructor 
> calls instead of one: one built in massage_init_elt, and the other in 
> expand_default_init.  The result of the first copy is marked 
> TARGET_EXPR_ELIDING_P, so when we try to pass it to the second copy we 
> hit the assert.  I think the assert is catching a real bug: even with 
> -fno-elide-constructors we should only copy once, not twice.
That’s right, thanks for pointing me in the right direction.

> This seems to be because 'digested' has the wrong value in 
> build_vec_init; we did just call digest_init in build_new_1, but 
> build_vec_init doesn't understand that.
The test to determine whether digest_init has been called is indeed 
incorrect, in that it will work if BASE is a reference to the array but 
not if it’s a pointer to its first element. The attached updated patch 
fixes this.

Successfully tested on x86_64-pc-linux-gnu. OK for trunk?

Simon
From 578ac1a022ff039cdca45cdfca31bdfe8b571b79 Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@nasilyan.com>
Date: Mon, 3 Feb 2025 11:43:14 +0100
Subject: [PATCH] c++: Properly detect calls to digest_init in build_vec_init
 [PR114619]

We currently ICE in checking mode with cxx_dialect < 17 on the following
valid code

=== cut here ===
struct X {
  X(const X&) {}
};
extern X x;
void foo () {
  new X[1]{x};
}
=== cut here ===

We trip on a gcc_checking_assert in cp_gimplify_expr due to a
TARGET_EXPR that is not TARGET_EXPR_ELIDING_P. As pointed by Jason, the
problem is that build_vec_init does not recognize that digest_init has
been called, and we end up calling the copy constructor twice.

This happens because the detection in build_vec_init assumes that BASE
is a reference to the array, while it's a pointer to its first element
here. This patch makes sure that the detection works in both cases.

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/114619

gcc/cp/ChangeLog:

	* init.cc (build_vec_init): Properly determine whether
	digest_init has been called.

gcc/testsuite/ChangeLog:

	* g++.dg/init/no-elide4.C: New test.

---
 gcc/cp/init.cc                        |  3 ++-
 gcc/testsuite/g++.dg/init/no-elide4.C | 11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/init/no-elide4.C

-- 
2.44.0
  

Comments

Jason Merrill Feb. 3, 2025, 11:45 p.m. UTC | #1
On 2/3/25 8:29 AM, Simon Martin wrote:
> Hi Jason,
> 
> On 16 Jan 2025, at 23:28, Jason Merrill wrote:
> 
>> On 10/19/24 5:09 AM, Simon Martin wrote:
>>> We currently ICE in checking mode with cxx_dialect < 17 on the
>>> following
>>> valid code
>>>
>>> === cut here ===
>>> struct X {
>>>     X(const X&) {}
>>> };
>>> extern X x;
>>> void foo () {
>>>     new X[1]{x};
>>> }
>>> === cut here ===
>>>
>>> The problem is that cp_gimplify_expr gcc_checking_asserts that a
>>> TARGET_EXPR is not TARGET_EXPR_ELIDING_P (or cannot be elided), while
>>> in
>>> this case with cxx_dialect < 17, it is TARGET_EXPR_ELIDING_P but we
>>> have
>>> not even tried to elide.
>>>
>>> This patch relaxes that gcc_checking_assert to not fail when using
>>> cxx_dialect < 17 and -fno-elide-constructors (I considered being more
> 
>>> clever at setting TARGET_EXPR_ELIDING_P appropriately but it looks
> 
>>> more
>>> risky and not worth the extra complexity for a checking assert).
>>
>> The problem is that in that case we end up with two copy constructor
>> calls instead of one: one built in massage_init_elt, and the other in
>> expand_default_init.  The result of the first copy is marked
>> TARGET_EXPR_ELIDING_P, so when we try to pass it to the second copy we
>> hit the assert.  I think the assert is catching a real bug: even with
>> -fno-elide-constructors we should only copy once, not twice.
> That’s right, thanks for pointing me in the right direction.
> 
>> This seems to be because 'digested' has the wrong value in
>> build_vec_init; we did just call digest_init in build_new_1, but
>> build_vec_init doesn't understand that.
> The test to determine whether digest_init has been called is indeed
> incorrect, in that it will work if BASE is a reference to the array but
> not if it’s a pointer to its first element. The attached updated patch
> fixes this.
> 
> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?

OK.

Jason
  

Patch

diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
index 3ab7f96335c..613775c5a7c 100644
--- a/gcc/cp/init.cc
+++ b/gcc/cp/init.cc
@@ -4786,7 +4786,8 @@  build_vec_init (tree base, tree maxindex, tree init,
       tree field, elt;
       /* If the constructor already has the array type, it's been through
 	 digest_init, so we shouldn't try to do anything more.  */
-      bool digested = same_type_p (atype, TREE_TYPE (init));
+      bool digested = (TREE_CODE (TREE_TYPE (init)) == ARRAY_TYPE
+		       && same_type_p (type, TREE_TYPE (TREE_TYPE (init))));
       from_array = 0;
 
       if (length_check)
diff --git a/gcc/testsuite/g++.dg/init/no-elide4.C b/gcc/testsuite/g++.dg/init/no-elide4.C
new file mode 100644
index 00000000000..9377d9f0161
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/no-elide4.C
@@ -0,0 +1,11 @@ 
+// PR c++/114619
+// { dg-do "compile" { target c++11 } }
+// { dg-options "-fno-elide-constructors" }
+
+struct X {
+  X(const X&) {}
+};
+extern X x;
+void foo () {
+  new X[1]{x};
+}