Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Qing Zhao <qing.zhao@oracle.com>]

On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:

On Thu, 30 Sep 2021, Jason Merrill wrote:

On 9/29/21 17:30, Qing Zhao wrote:
Hi,

PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359

Is due to -ftrivial-auto-var-init adding initialization for READONLY
variable “this” in the following routine: (t.cpp.005t.original)

=======

;; Function A::foo()::<lambda()> (null)
;; enabled by -tree-original

{
 const struct A * const this [value-expr: &__closure->__this];
   const struct A * const this [value-expr: &__closure->__this];
 return <retval> = (double) ((const struct A *) this)->a;
}
=======

However, in the above routine, “this” is NOT marked as READONLY, but its
value-expr "&__closure->__this” is marked as READONLY.

There are two major issues:

1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
marked as READONLY;
2. In the C++ FE, “this” should be marked as READONLY.

The idea solution will be:

1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;

Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?

In the case it’s not a quick fix in C++FE, I proposed the following fix in
middle end:

Let me know your comments or suggestions on this.

Thanks a lot for the help.

I'd think is_var_need_auto_init should be false for any variable with
DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
objects that are initialized elsewhere.

IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
auto-init VLAs, otherwise I tend to agree - would we handle those
when we see a DECL_EXPR then?

The current implementation is:


gimplify_decl_expr:


For each DECL_EXPR “decl”

   If (VAR_P (decl) && !DECL_EXTERNAL (decl))
     {
if (is_vla (decl))
          gimplify_vla_decl (decl, …);      /* existing handling: create a VALUE_EXPR for this vla decl*/

…
if (has_explicit_init (decl))
 {
  …;     /* existing handling.  */
 }
else if (is_var_need_auto_init (decl))      /*. New code. */
 {
   gimple_add_init_for_auto_var (….);   /*  new code.  */
   ...
 }
     }


Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization.

if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either.  We will miss adding initializations for these decls.

So, I think that the current implementation is correct.

And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.

Let me know your opinion on this.

Thanks.

Qing



Qing

==============================
From 0a5982cd61bc4610655d3df00ae8d2fbcb3c8e9b Mon Sep 17 00:00:00 2001
From: Qing Zhao <qing.zhao@oracle.com>
Date: Wed, 29 Sep 2021 20:49:59 +0000
Subject: [PATCH] Fix PR102359

---
gcc/gimplify.c                  | 15 +++++++++++++++
gcc/testsuite/g++.dg/pr102359.C | 13 +++++++++++++
2 files changed, 28 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/pr102359.C

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 1067113b1639..a2587869b35d 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1819,12 +1819,27 @@ gimple_add_padding_init_for_auto_var (tree decl,
bool is_vla,
  gimplify_seq_add_stmt (seq_p, call);
}

+/* Return true if the DECL is READONLY.
+   This is to workaround a C++ FE bug that only mark the value_expr of
"this"
+   as readonly but does not mark "this" as readonly.
+   C++ FE should fix this issue before replacing this routine with
+   TREE_READONLY (decl).  */
+
+static bool
+is_decl_readonly (tree decl)
+{
+  return (TREE_READONLY (decl)
+   || (DECL_HAS_VALUE_EXPR_P (decl)
+ && TREE_READONLY (DECL_VALUE_EXPR (decl))));
+}
+
/* Return true if the DECL need to be automaticly initialized by the
   compiler.  */
static bool
is_var_need_auto_init (tree decl)
{
  if (auto_var_p (decl)
+      && !is_decl_readonly (decl)
      && (flag_auto_var_init > AUTO_INIT_UNINITIALIZED)
      && (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl)))
      && !is_empty_type (TREE_TYPE (decl)))
diff --git a/gcc/testsuite/g++.dg/pr102359.C
b/gcc/testsuite/g++.dg/pr102359.C
new file mode 100644
index 000000000000..da643cde7bed
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr102359.C
@@ -0,0 +1,13 @@
+/* PR middle-end/102359 ICE gimplification failed since
+   r12-3433-ga25e0b5e6ac8a77a.  */
+/* { dg-do compile } */
+/* { dg-options "-ftrivial-auto-var-init=zero" } */
+/* { dg-require-effective-target c++17 } */
+
+struct A {
+  double a = 111;
+  auto foo() {
+    return [*this] { return a; };
+  }
+};
+int X = A{}.foo()();




--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)