Hi Jason,
On Tue Mar 4, 2025 at 11:47 PM CET, Jason Merrill wrote:
> On 2/14/25 12:08 PM, Simon Martin wrote:
>> We have been miscompiling the following valid code since GCC8, and
>> r8-3497-g281e6c1d8f1b4c
>>
>> === cut here ===
>> struct span {
>> span (const int (&__first)[1]) : _M_ptr (__first) {}
>> int operator[] (long __i) { return _M_ptr[__i]; }
>> const int *_M_ptr;
>> };
>> void foo () {
>> constexpr int a_vec[]{1};
>> auto vec{[&a_vec]() -> span { return a_vec; }()};
>> }
>> === cut here ===
>>
>> The problem is that perform_implicit_conversion_flags (via
>> mark_rvalue_use) replaces "a_vec" in the return statement by a
>> CONSTRUCTOR representing a_vec's constant value, and then takes its
>> address when invoking span's constructor. So we end up with an instance
>> that points to garbage instead of a_vec's storage.
>>
>> I've tried many things to somehow recover from this replacement, but I
>> actually think we should not do it when converting to a class type: we
>> have no idea whether the conversion will involve a constructor taking an
>> address or reference. So we should assume it's the case, and call
>> mark_lvalue_use, not mark_rvalue_use (I might very weel be overseeing
>> things, and feedback is more than welcome).
>
> Yeah, those mark_*_use calls seem misplaced, they should be called
> instead by the code that actually does the conversion.
>
> What if we replace all of them here with just mark_exp_read? Or nothing?
Thanks for the suggestions; simply removing those calls actually works. This
is what the attached updated patch does.
Successfully tested on x86_64-pc-linux-gnu. OK for trunk? And if so, OK for
branches after 2-3 weeks since it's a wrong code bug?
Simon
From cbb1fae22ff259b75f4304b1b0e6d6f84216b3d9 Mon Sep 17 00:00:00 2001
From: Simon Martin <si...@nasilyan.com>
Date: Wed, 5 Mar 2025 12:42:37 +0100
Subject: [PATCH] c++: Don't replace INDIRECT_REFs by a const capture proxy too
eagerly [PR117504]
We have been miscompiling the following valid code since GCC8, and
r8-3497-g281e6c1d8f1b4c
=== cut here ===
struct span {
span (const int (&__first)[1]) : _M_ptr (__first) {}
int operator[] (long __i) { return _M_ptr[__i]; }
const int *_M_ptr;
};
void foo () {
constexpr int a_vec[]{1};
auto vec{[&a_vec]() -> span { return a_vec; }()};
}
=== cut here ===
The problem is that perform_implicit_conversion_flags (via
mark_rvalue_use) replaces "a_vec" in the return statement by a
CONSTRUCTOR representing a_vec's constant value, and then takes its
address when invoking span's constructor. So we end up with an instance
that points to garbage instead of a_vec's storage.
As per Jason's suggestion, this patch simply removes the calls to
mark_*_use from perform_implicit_conversion_flags, which fixes the PR.
Successfully tested on x86_64-pc-linux-gnu.
PR c++/117504
gcc/cp/ChangeLog:
* call.cc (perform_implicit_conversion_flags): Don't call
mark_{l,r}value_use.
gcc/testsuite/ChangeLog:
* g++.dg/cpp2a/constexpr-117504.C: New test.
* g++.dg/cpp2a/constexpr-117504a.C: New test.
---
gcc/cp/call.cc | 5 --
gcc/testsuite/g++.dg/cpp2a/constexpr-117504.C | 60 +++++++++++++++++++
.../g++.dg/cpp2a/constexpr-117504a.C | 12 ++++
3 files changed, 72 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-117504.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-117504a.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index f7b4cccb1c7..c1c8987ec8b 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13971,11 +13971,6 @@ perform_implicit_conversion_flags (tree type, tree
expr,
conversion *conv;
location_t loc = cp_expr_loc_or_input_loc (expr);
- if (TYPE_REF_P (type))
- expr = mark_lvalue_use (expr);
- else
- expr = mark_rvalue_use (expr);
-
if (error_operand_p (expr))
return error_mark_node;
diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-117504.C
b/gcc/testsuite/g++.dg/cpp2a/constexpr-117504.C
new file mode 100644
index 00000000000..290d3dfd61e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-117504.C
@@ -0,0 +1,60 @@
+// PR c++/117504 - Initial report
+// { dg-do "run" { target c++20 } }
+
+struct span {
+ span (const int (&__first)[1]) : _M_ptr (__first) {}
+ int operator[] (long __i) { return _M_ptr[__i]; }
+ const int *_M_ptr;
+};
+
+constexpr int a_global_vec[]{1};
+span myFunctor() {
+ return a_global_vec;
+}
+
+int main() {
+ constexpr int a_vec[]{1};
+
+ //
+ // This PR's case, that used to be miscompiled.
+ //
+ auto lambda_1 = [&a_vec] () -> span { return a_vec; };
+ auto vec_1 { lambda_1 () };
+ if (vec_1[0] != 1)
+ __builtin_abort ();
+
+ // Variant that used to be miscompiled as well.
+ auto lambda_2 = [&] () -> span { return a_vec; };
+ auto vec_2 { lambda_2 () };
+ if (vec_2[0] != 1)
+ __builtin_abort ();
+
+ //
+ // Related cases that worked already.
+ //
+ auto lambda_3 = [&a_vec] () /* -> span */ { return a_vec; };
+ auto vec_3 { lambda_3 () };
+ if (vec_3[0] != 1)
+ __builtin_abort ();
+
+ auto lambda_4 = [&] () /* -> span */ { return a_vec; };
+ auto vec_4 { lambda_4 () };
+ if (vec_4[0] != 1)
+ __builtin_abort ();
+
+ const int (&vec_5)[1] = a_vec;
+ if (vec_5[0] != 1)
+ __builtin_abort ();
+
+ span vec_6 (a_vec);
+ if (vec_6[0] != 1)
+ __builtin_abort ();
+
+ auto vec_7 = myFunctor ();
+ if (vec_7[0] != 1)
+ __builtin_abort ();
+
+ const int (&vec_8)[1] { a_vec };
+ if (vec_8[0] != 1)
+ __builtin_abort ();
+}
diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-117504a.C
b/gcc/testsuite/g++.dg/cpp2a/constexpr-117504a.C
new file mode 100644
index 00000000000..f6d4dc8cbc5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-117504a.C
@@ -0,0 +1,12 @@
+// PR c++/117504 - ICE discovered by ppalka@ when reducing.
+// { dg-do "compile" { target c++20 } }
+
+struct span {
+ span (const int* __first) : _M_ptr (__first) {}
+ int operator[] (long __i) { return _M_ptr[__i]; }
+ const int *_M_ptr;
+};
+int main() {
+ constexpr int a_vec[]{1};
+ auto vec { [&a_vec]() -> span { return a_vec; } () };
+}
--
2.44.0
