[PATCH] libgccjit: Fix infinite recursion in gt_ggc_mx_lang_tree_node

[PATCH] libgccjit: Fix infinite recursion in gt_ggc_mx_lang_tree_node

[Antoni Boucher]

Hi.
The attached patch fix bug 105827:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105827

I'm not sure how to test this, so please share ideas.

Thanks for the review.

Re: [PATCH] libgccjit: Fix infinite recursion in gt_ggc_mx_lang_tree_node

[Antoni Boucher]

[-- Attachment #1: Type: text/plain, Size: 312 bytes --]

Sorry, forgot to attach the patch.

Here it is.

On Thu, 2022-06-02 at 21:20 -0400, Antoni Boucher via Jit wrote:
> Hi.
> The attached patch fix bug 105827:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105827
> 
> I'm not sure how to test this, so please share ideas.
> 
> Thanks for the review.


[-- Attachment #2: 0001-libgccjit-Fix-infinite-recursion-in-gt_ggc_mx_lang_t.patch --]
[-- Type: text/x-patch, Size: 2423 bytes --]

From 609153a39921b8e9aa1934da131575bb64881d67 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Thu, 2 Jun 2022 21:14:06 -0400
Subject: [PATCH] libgccjit: Fix infinite recursion in gt_ggc_mx_lang_tree_node

2022-06-02  Antoni Boucher  <bouanto@zoho.com>

gcc/jit/
	PR target/105827
	* dummy-frontend.cc: Fix lang_tree_node.
	* jit-common.h: New function (jit_tree_chain_next) used by
	lang_tree_node.
---
 gcc/jit/dummy-frontend.cc | 13 +++++++------
 gcc/jit/jit-common.h      | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc
index 84ff359bfe3..8bb5d03d630 100644
--- a/gcc/jit/dummy-frontend.cc
+++ b/gcc/jit/dummy-frontend.cc
@@ -506,13 +506,14 @@ struct GTY(()) lang_identifier
 
 /* The resulting tree type.  */
 
+/* See lang_tree_node in gcc/c/c-decl.cc.  */
 union GTY((desc ("TREE_CODE (&%h.generic) == IDENTIFIER_NODE"),
-	   chain_next ("CODE_CONTAINS_STRUCT (TREE_CODE (&%h.generic), TS_COMMON) ? ((union lang_tree_node *) TREE_CHAIN (&%h.generic)) : NULL")))
-lang_tree_node
-{
-  union tree_node GTY((tag ("0"),
-		       desc ("tree_node_structure (&%h)"))) generic;
-  struct lang_identifier GTY((tag ("1"))) identifier;
+       chain_next ("(union lang_tree_node *) jit_tree_chain_next (&%h.generic)"))) lang_tree_node
+ {
+  union tree_node GTY ((tag ("0"),
+			desc ("tree_node_structure (&%h)")))
+    generic;
+  struct lang_identifier GTY ((tag ("1"))) identifier;
 };
 
 /* We don't use language_function.  */
diff --git a/gcc/jit/jit-common.h b/gcc/jit/jit-common.h
index 3ff7447fbf3..50580358a1f 100644
--- a/gcc/jit/jit-common.h
+++ b/gcc/jit/jit-common.h
@@ -93,6 +93,21 @@ const int NUM_GCC_JIT_TYPES = GCC_JIT_TYPE_INT128_T + 1;
 
    End of comment for inclusion in the docs.  */
 
+/* See c_tree_chain_next in gcc/c-family/c-common.h.  */
+static inline tree
+jit_tree_chain_next (tree t)
+{
+  /* TREE_CHAIN of a type is TYPE_STUB_DECL, which is different
+     kind of object, never a long chain of nodes.  Prefer
+     TYPE_NEXT_VARIANT for types.  */
+  if (CODE_CONTAINS_STRUCT (TREE_CODE (t), TS_TYPE_COMMON))
+    return TYPE_NEXT_VARIANT (t);
+  /* Otherwise, if there is TREE_CHAIN, return it.  */
+  if (CODE_CONTAINS_STRUCT (TREE_CODE (t), TS_COMMON))
+    return TREE_CHAIN (t);
+  return NULL;
+}
+
 namespace gcc {
 
 namespace jit {
-- 
2.26.2.7.g19db9cfb68.dirty

Re: [PATCH] libgccjit: Fix infinite recursion in gt_ggc_mx_lang_tree_node

[David Malcolm]

On Thu, 2022-06-02 at 21:23 -0400, Antoni Boucher via Gcc-patches
wrote:
> Sorry, forgot to attach the patch.
> 
> Here it is.
> 
> On Thu, 2022-06-02 at 21:20 -0400, Antoni Boucher via Jit wrote:
> > Hi.
> > The attached patch fix bug 105827:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105827
> > 
> > I'm not sure how to test this, so please share ideas.

Do you have a reproducer for this?

With garbage-collections issues in libgccjit, you can set:

  gcc_jit_context_set_bool_option (ctxt,
                                   GCC_JIT_BOOL_OPTION_SELFCHECK_GC,
                                   1);

which will force a full garbage collection at every opportunity that
the collector considers doing one (rather than following heuristics)

This really slows things down, but makes reproducing crashes much more
deterministic, often turning "it crashes every now and then" to "it
crashes every time" (and the test suite runs with this enabled).

> > 
> > Thanks for the review.
> 

> diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc
> index 84ff359bfe3..8bb5d03d630 100644
> --- a/gcc/jit/dummy-frontend.cc
> +++ b/gcc/jit/dummy-frontend.cc
> @@ -506,13 +506,14 @@ struct GTY(()) lang_identifier
>  
>  /* The resulting tree type.  */
>  
> +/* See lang_tree_node in gcc/c/c-decl.cc.  */
>  union GTY((desc ("TREE_CODE (&%h.generic) == IDENTIFIER_NODE"),
> -	   chain_next ("CODE_CONTAINS_STRUCT (TREE_CODE (&%h.generic), TS_COMMON) ? ((union lang_tree_node *) TREE_CHAIN (&%h.generic)) : NULL")))
> -lang_tree_node
> -{
> -  union tree_node GTY((tag ("0"),
> -		       desc ("tree_node_structure (&%h)"))) generic;
> -  struct lang_identifier GTY((tag ("1"))) identifier;
> +       chain_next ("(union lang_tree_node *) jit_tree_chain_next (&%h.generic)"))) lang_tree_node
> + {
> +  union tree_node GTY ((tag ("0"),
> +			desc ("tree_node_structure (&%h)")))
> +    generic;
> +  struct lang_identifier GTY ((tag ("1"))) identifier;
>  };

Those GTY markings on gcc/jit/dummy-frontend.cc's lang_tree_node union
have been like that since my initial proof-of-concept patch back in
2013:
  https://gcc.gnu.org/legacy-ml/gcc-patches/2013-10/msg00228.html
so presumably I simply copied and pasted that from another frontend
when I was initially prototyping libgccjit.  There was an element of
"cargo cult programming" as I was getting the project started.

Jakub had changed the C and C++ frontends in June 2011 with
563007852e8d19b66ec8c1e42e431efaaa967dc6, which introduced the
c_tree_chain_next that you're now copying in your patch, so presumably
I copied from a different frontend.  My notes say I created the first
prototype in July 2013, so that's when I would have copied&pasted the
code.

Dave

Re: [PATCH] libgccjit: Fix infinite recursion in gt_ggc_mx_lang_tree_node

[Antoni Boucher]

It seems I cannot reproduce the issue.
Should we drop this patch, then?
Or do you think there's value in keeping it?

On Mon, 2022-06-06 at 19:01 -0400, David Malcolm wrote:
> On Thu, 2022-06-02 at 21:23 -0400, Antoni Boucher via Gcc-patches
> wrote:
> > Sorry, forgot to attach the patch.
> > 
> > Here it is.
> > 
> > On Thu, 2022-06-02 at 21:20 -0400, Antoni Boucher via Jit wrote:
> > > Hi.
> > > The attached patch fix bug 105827:
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105827
> > > 
> > > I'm not sure how to test this, so please share ideas.
> 
> Do you have a reproducer for this?
> 
> With garbage-collections issues in libgccjit, you can set:
> 
>   gcc_jit_context_set_bool_option (ctxt,
>                                    GCC_JIT_BOOL_OPTION_SELFCHECK_GC,
>                                    1);
> 
> which will force a full garbage collection at every opportunity that
> the collector considers doing one (rather than following heuristics)
> 
> This really slows things down, but makes reproducing crashes much
> more
> deterministic, often turning "it crashes every now and then" to "it
> crashes every time" (and the test suite runs with this enabled).
> 
> > > 
> > > Thanks for the review.
> > 
> 
> > diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc
> > index 84ff359bfe3..8bb5d03d630 100644
> > --- a/gcc/jit/dummy-frontend.cc
> > +++ b/gcc/jit/dummy-frontend.cc
> > @@ -506,13 +506,14 @@ struct GTY(()) lang_identifier
> >  
> >  /* The resulting tree type.  */
> >  
> > +/* See lang_tree_node in gcc/c/c-decl.cc.  */
> >  union GTY((desc ("TREE_CODE (&%h.generic) == IDENTIFIER_NODE"),
> > -	   chain_next ("CODE_CONTAINS_STRUCT (TREE_CODE
> > (&%h.generic), TS_COMMON) ? ((union lang_tree_node *) TREE_CHAIN
> > (&%h.generic)) : NULL")))
> > -lang_tree_node
> > -{
> > -  union tree_node GTY((tag ("0"),
> > -		       desc ("tree_node_structure (&%h)")))
> > generic;
> > -  struct lang_identifier GTY((tag ("1"))) identifier;
> > +       chain_next ("(union lang_tree_node *) jit_tree_chain_next
> > (&%h.generic)"))) lang_tree_node
> > + {
> > +  union tree_node GTY ((tag ("0"),
> > +			desc ("tree_node_structure (&%h)")))
> > +    generic;
> > +  struct lang_identifier GTY ((tag ("1"))) identifier;
> >  };
> 
> Those GTY markings on gcc/jit/dummy-frontend.cc's lang_tree_node
> union
> have been like that since my initial proof-of-concept patch back in
> 2013:
>   https://gcc.gnu.org/legacy-ml/gcc-patches/2013-10/msg00228.html
> so presumably I simply copied and pasted that from another frontend
> when I was initially prototyping libgccjit.  There was an element of
> "cargo cult programming" as I was getting the project started.
> 
> Jakub had changed the C and C++ frontends in June 2011 with
> 563007852e8d19b66ec8c1e42e431efaaa967dc6, which introduced the
> c_tree_chain_next that you're now copying in your patch, so
> presumably
> I copied from a different frontend.  My notes say I created the first
> prototype in July 2013, so that's when I would have copied&pasted the
> code.
> 
> Dave
> 
> 
> 
> 
> 
> 

Re: [PATCH] libgccjit: Fix infinite recursion in gt_ggc_mx_lang_tree_node

[Andrew Pinski]

On Thu, Jun 2, 2022 at 6:21 PM Antoni Boucher via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi.
> The attached patch fix bug 105827:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105827
>
> I'm not sure how to test this, so please share ideas.

Looks like the attachment was removed ...

>
> Thanks for the review.