diff mbox series

[v2] c++: Defer -fstrong-eval-order processing to template instantiation time [PR117158]

Message ID 01020192f7c20514-b212126d-89d0-4e5b-9479-9a3272dc1424-000000@eu-west-1.amazonses.com
State New
Headers show
Series [v2] c++: Defer -fstrong-eval-order processing to template instantiation time [PR117158] | expand

Commit Message

Simon Martin Nov. 4, 2024, 3:19 p.m. UTC 
Hi Jason,

On 1 Nov 2024, at 16:31, Jason Merrill wrote:

> On 11/1/24 5:07 AM, Simon Martin wrote:
>> Since r10-3793-g1a37b6d9a7e57c, we ICE upon the following valid code
>> with -std=c++17 and above
>>
>> === cut here ===
>> struct Base {
>>    unsigned int *intarray;
>> };
>> template <typename T> struct Sub : public Base {
>>    bool Get(int i) {
>>      return (Base::intarray[++i] == 0);
>>    }
>> };
>> === cut here ===
>>
>> The problem is that from c++17 on, we use -fstrong-eval-order and 
>> need
>> to wrap the array access expression into a SAVE_EXPR, and end up 
>> calling
>> contains_placeholder_p with a SCOPE_REF, that it does not handle 
>> well.
>>
>> This patch fixes this by skipping the first operand of SCOPE_REFs in
>> contains_placeholder_p.
>
> Code in gcc/ shouldn't refer to tree codes from cp-tree.def.
>
> We probably shouldn't do the strong-eval-order transformation when 
> processing_template_decl anyway.
Thanks, that makes sense. The attached updated patch skips the
wrapping when processing_template_decl, and also adds a test
case checking that -fstrong-eval-order processing is properly
done for instantiated templates.

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

Thanks, Simon
From b5c8777bc04bd633781d3f3af2a72efd8888e2ab Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@nasilyan.com>
Date: Thu, 31 Oct 2024 09:04:49 +0100
Subject: [PATCH] c++: Defer -fstrong-eval-order processing to template
 instantiation time [PR117158]

Since r10-3793-g1a37b6d9a7e57c, we ICE upon the following valid code
with -std=c++17 and above

=== cut here ===
struct Base {
  unsigned int *intarray;
};
template <typename T> struct Sub : public Base {
  bool Get(int i) {
    return (Base::intarray[++i] == 0);
  }
};
=== cut here ===

The problem is that from c++17 on, we use -fstrong-eval-order and need
to wrap the array access expression into a SAVE_EXPR. We do so at
template declaration time, and end up calling contains_placeholder_p
with a SCOPE_REF, that it does not handle well.

This patch fixes this by deferring the wrapping into SAVE_EXPR to
instantiation time for templates, when the SCOPE_REF will have been
turned into a COMPONENT_REF.

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/117158

gcc/cp/ChangeLog:

	* typeck.cc (cp_build_array_ref): Only wrap array expression
	into a SAVE_EXPR at template instantiation time.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1z/eval-order13.C: New test.
	* g++.dg/parse/crash77.C: New test.

---
 gcc/cp/typeck.cc                          |  3 ++-
 gcc/testsuite/g++.dg/cpp1z/eval-order13.C | 29 +++++++++++++++++++++++
 gcc/testsuite/g++.dg/parse/crash77.C      | 13 ++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1z/eval-order13.C
 create mode 100644 gcc/testsuite/g++.dg/parse/crash77.C

Comments

Jason Merrill Nov. 5, 2024, 12:23 a.m. UTC | #1 
On 11/4/24 10:19 AM, Simon Martin wrote:
> Hi Jason,
> 
> On 1 Nov 2024, at 16:31, Jason Merrill wrote:
> 
>> On 11/1/24 5:07 AM, Simon Martin wrote:
>>> Since r10-3793-g1a37b6d9a7e57c, we ICE upon the following valid code
>>> with -std=c++17 and above
>>>
>>> === cut here ===
>>> struct Base {
>>>     unsigned int *intarray;
>>> };
>>> template <typename T> struct Sub : public Base {
>>>     bool Get(int i) {
>>>       return (Base::intarray[++i] == 0);
>>>     }
>>> };
>>> === cut here ===
>>>
>>> The problem is that from c++17 on, we use -fstrong-eval-order and
>>> need
>>> to wrap the array access expression into a SAVE_EXPR, and end up
>>> calling
>>> contains_placeholder_p with a SCOPE_REF, that it does not handle
>>> well.
>>>
>>> This patch fixes this by skipping the first operand of SCOPE_REFs in
>>> contains_placeholder_p.
>>
>> Code in gcc/ shouldn't refer to tree codes from cp-tree.def.
>>
>> We probably shouldn't do the strong-eval-order transformation when
>> processing_template_decl anyway.
> Thanks, that makes sense. The attached updated patch skips the
> wrapping when processing_template_decl, and also adds a test
> case checking that -fstrong-eval-order processing is properly
> done for instantiated templates.
> 
> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?

OK.

> Thanks, Simon
Simon Martin Nov. 5, 2024, 9:13 a.m. UTC | #2 
Hi Jason,

On 5 Nov 2024, at 1:23, Jason Merrill wrote:

> On 11/4/24 10:19 AM, Simon Martin wrote:
>> Hi Jason,
>>
>> On 1 Nov 2024, at 16:31, Jason Merrill wrote:
>>
>>> On 11/1/24 5:07 AM, Simon Martin wrote:
>>>> Since r10-3793-g1a37b6d9a7e57c, we ICE upon the following valid 
>>>> code
>>>> with -std=c++17 and above
>>>>
>>>> === cut here ===
>>>> struct Base {
>>>>     unsigned int *intarray;
>>>> };
>>>> template <typename T> struct Sub : public Base {
>>>>     bool Get(int i) {
>>>>       return (Base::intarray[++i] == 0);
>>>>     }
>>>> };
>>>> === cut here ===
>>>>
>>>> The problem is that from c++17 on, we use -fstrong-eval-order and
>>>> need
>>>> to wrap the array access expression into a SAVE_EXPR, and end up
>>>> calling
>>>> contains_placeholder_p with a SCOPE_REF, that it does not handle
>>>> well.
>>>>
>>>> This patch fixes this by skipping the first operand of SCOPE_REFs 
>>>> in
>>>> contains_placeholder_p.
>>>
>>> Code in gcc/ shouldn't refer to tree codes from cp-tree.def.
>>>
>>> We probably shouldn't do the strong-eval-order transformation when
>>> processing_template_decl anyway.
>> Thanks, that makes sense. The attached updated patch skips the
>> wrapping when processing_template_decl, and also adds a test
>> case checking that -fstrong-eval-order processing is properly
>> done for instantiated templates.
>>
>> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?
>
> OK.
Thanks, pushed to trunk as r15-4957-gb1d92aeb8583c8.

Since it’s a reject-valid regression, OK to back port to active 
branches as well?

Thanks, Simon
Jason Merrill Nov. 5, 2024, 1:28 p.m. UTC | #3 
On 11/5/24 4:13 AM, Simon Martin wrote:
> Hi Jason,
> 
> On 5 Nov 2024, at 1:23, Jason Merrill wrote:
> 
>> On 11/4/24 10:19 AM, Simon Martin wrote:
>>> Hi Jason,
>>>
>>> On 1 Nov 2024, at 16:31, Jason Merrill wrote:
>>>
>>>> On 11/1/24 5:07 AM, Simon Martin wrote:
>>>>> Since r10-3793-g1a37b6d9a7e57c, we ICE upon the following valid
>>>>> code
>>>>> with -std=c++17 and above
>>>>>
>>>>> === cut here ===
>>>>> struct Base {
>>>>>      unsigned int *intarray;
>>>>> };
>>>>> template <typename T> struct Sub : public Base {
>>>>>      bool Get(int i) {
>>>>>        return (Base::intarray[++i] == 0);
>>>>>      }
>>>>> };
>>>>> === cut here ===
>>>>>
>>>>> The problem is that from c++17 on, we use -fstrong-eval-order and
>>>>> need
>>>>> to wrap the array access expression into a SAVE_EXPR, and end up
>>>>> calling
>>>>> contains_placeholder_p with a SCOPE_REF, that it does not handle
>>>>> well.
>>>>>
>>>>> This patch fixes this by skipping the first operand of SCOPE_REFs
>>>>> in
>>>>> contains_placeholder_p.
>>>>
>>>> Code in gcc/ shouldn't refer to tree codes from cp-tree.def.
>>>>
>>>> We probably shouldn't do the strong-eval-order transformation when
>>>> processing_template_decl anyway.
>>> Thanks, that makes sense. The attached updated patch skips the
>>> wrapping when processing_template_decl, and also adds a test
>>> case checking that -fstrong-eval-order processing is properly
>>> done for instantiated templates.
>>>
>>> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?
>>
>> OK.
> Thanks, pushed to trunk as r15-4957-gb1d92aeb8583c8.
> 
> Since it’s a reject-valid regression, OK to back port to active
> branches as well?

OK.

Jason
diff mbox series

Patch

diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 6ce1bb61fe7..439681216be 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -4092,7 +4092,8 @@  cp_build_array_ref (location_t loc, tree array, tree idx,
     tree ar = cp_default_conversion (array, complain);
     tree ind = cp_default_conversion (idx, complain);
 
-    if (!first && flag_strong_eval_order == 2 && TREE_SIDE_EFFECTS (ind))
+    if (!processing_template_decl
+	&& !first && flag_strong_eval_order == 2 && TREE_SIDE_EFFECTS (ind))
       ar = first = save_expr (ar);
 
     /* Put the integer in IND to simplify error checking.  */
diff --git a/gcc/testsuite/g++.dg/cpp1z/eval-order13.C b/gcc/testsuite/g++.dg/cpp1z/eval-order13.C
new file mode 100644
index 00000000000..6e8ebeb3096
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/eval-order13.C
@@ -0,0 +1,29 @@ 
+// PR c++/117158 - Similar to eval-order7.C, only with templates.
+// { dg-do run { target c++11 } }
+// { dg-options "-fstrong-eval-order" }
+
+int a[4] = { 1, 2, 3, 4 };
+int b[4] = { 5, 6, 7, 8 };
+
+struct Base {
+  int *intarray;
+};
+
+template <typename T>
+struct Sub : public Base {
+  int Get(int i) {
+    Base::intarray = a;
+    int r = Base::intarray[(Base::intarray = b, i)];
+    if (Base::intarray != b)
+      __builtin_abort ();
+    return r;
+  }
+};
+
+int
+main ()
+{
+  Sub<int> s;
+  if (s.Get (3) != 4)
+    __builtin_abort ();
+}
diff --git a/gcc/testsuite/g++.dg/parse/crash77.C b/gcc/testsuite/g++.dg/parse/crash77.C
new file mode 100644
index 00000000000..729362eb599
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/crash77.C
@@ -0,0 +1,13 @@ 
+// PR c++/117158
+// { dg-do "compile" }
+
+struct Base {
+  unsigned int *intarray;
+};
+
+template <typename T>
+struct Sub : public Base {
+  bool Get(int i) {
+    return (Base::intarray[++i] == 0);
+  }
+};