[PATCH] Fix free_lang_data on asm stmts (PR lto/91572)

[PATCH] Fix free_lang_data on asm stmts (PR lto/91572)

[Jakub Jelinek]

Hi!

The following testcase ICEs, because on the inline asm LTO streaming streams
the constraint strings ("g" in this case), including their type, but the
fld type discovery doesn't see that type and so we end up streaming const
char type turned into its own main variant.

The strings for asm are in TREE_PURPOSE of the TREE_LIST args.
walk_tree doesn't walk TREE_PURPOSE though.  Tried to change that, but it
breaks way too much, tried to walk TREE_PURPOSE of TREE_LIST just for the
fld walking (find_decls_types_r), but that doesn't work either, most of the
TREE_PURPOSE we do not want to walk, usually it contains C++ default
arguments which fld clears.  So, this directed patch walks the TREE_PURPOSE
solely for the asm stmt arguments.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-08-31  Jakub Jelinek  <jakub@redhat.com>

	PR lto/91572
	* tree.c (find_decls_types_in_node): Also walk TREE_PURPOSE of
	GIMPLE_ASM TREE_LIST operands.

	* g++.dg/lto/pr91572_0.C: New test.

--- gcc/tree.c.jj	2019-08-29 10:22:06.337702323 +0200
+++ gcc/tree.c	2019-08-29 11:07:16.120107950 +0200
@@ -6142,6 +6142,13 @@ find_decls_types_in_node (struct cgraph_
 	    {
 	      tree arg = gimple_op (stmt, i);
 	      find_decls_types (arg, fld);
+	      /* find_decls_types doesn't walk TREE_PURPOSE of TREE_LISTs,
+		 which we need for asm stmts.  */
+	      if (arg
+		  && TREE_CODE (arg) == TREE_LIST
+		  && TREE_PURPOSE (arg)
+		  && gimple_code (stmt) == GIMPLE_ASM)
+		find_decls_types (TREE_PURPOSE (arg), fld);
 	    }
 	}
     }
--- gcc/testsuite/g++.dg/lto/pr91572_0.C.jj	2019-08-28 18:13:47.718349087 +0200
+++ gcc/testsuite/g++.dg/lto/pr91572_0.C	2019-08-28 18:13:41.695436342 +0200
@@ -0,0 +1,12 @@
+// PR lto/91572
+// { dg-lto-do link }
+// { dg-lto-options { { -O -fPIC -flto } } }
+// { dg-require-effective-target shared }
+// { dg-require-effective-target fpic }
+// { dg-extra-ld-options "-shared" }
+
+void foo (char);
+namespace N {
+  class A { A (); };
+  A::A () { asm ("" : : "g" (0)); }
+}

	Jakub

Re: [PATCH] Fix free_lang_data on asm stmts (PR lto/91572)

[Richard Biener]

On August 31, 2019 8:30:03 AM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>The following testcase ICEs, because on the inline asm LTO streaming
>streams
>the constraint strings ("g" in this case), including their type, but
>the
>fld type discovery doesn't see that type and so we end up streaming
>const
>char type turned into its own main variant.
>
>The strings for asm are in TREE_PURPOSE of the TREE_LIST args.
>walk_tree doesn't walk TREE_PURPOSE though.  Tried to change that, but
>it
>breaks way too much, tried to walk TREE_PURPOSE of TREE_LIST just for
>the
>fld walking (find_decls_types_r), but that doesn't work either, most of
>the
>TREE_PURPOSE we do not want to walk, usually it contains C++ default
>arguments which fld clears.  So, this directed patch walks the
>TREE_PURPOSE
>solely for the asm stmt arguments.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok, but I wonder if we can stream the constraint strings in a simpler way - surely the type doesn't really matter?
Why are they not identifier nodes? 

Richard. 

>2019-08-31  Jakub Jelinek  <jakub@redhat.com>
>
>	PR lto/91572
>	* tree.c (find_decls_types_in_node): Also walk TREE_PURPOSE of
>	GIMPLE_ASM TREE_LIST operands.
>
>	* g++.dg/lto/pr91572_0.C: New test.
>
>--- gcc/tree.c.jj	2019-08-29 10:22:06.337702323 +0200
>+++ gcc/tree.c	2019-08-29 11:07:16.120107950 +0200
>@@ -6142,6 +6142,13 @@ find_decls_types_in_node (struct cgraph_
> 	    {
> 	      tree arg = gimple_op (stmt, i);
> 	      find_decls_types (arg, fld);
>+	      /* find_decls_types doesn't walk TREE_PURPOSE of TREE_LISTs,
>+		 which we need for asm stmts.  */
>+	      if (arg
>+		  && TREE_CODE (arg) == TREE_LIST
>+		  && TREE_PURPOSE (arg)
>+		  && gimple_code (stmt) == GIMPLE_ASM)
>+		find_decls_types (TREE_PURPOSE (arg), fld);
> 	    }
> 	}
>     }
>--- gcc/testsuite/g++.dg/lto/pr91572_0.C.jj	2019-08-28
>18:13:47.718349087 +0200
>+++ gcc/testsuite/g++.dg/lto/pr91572_0.C	2019-08-28 18:13:41.695436342
>+0200
>@@ -0,0 +1,12 @@
>+// PR lto/91572
>+// { dg-lto-do link }
>+// { dg-lto-options { { -O -fPIC -flto } } }
>+// { dg-require-effective-target shared }
>+// { dg-require-effective-target fpic }
>+// { dg-extra-ld-options "-shared" }
>+
>+void foo (char);
>+namespace N {
>+  class A { A (); };
>+  A::A () { asm ("" : : "g" (0)); }
>+}
>
>	Jakub

Re: [PATCH] Fix free_lang_data on asm stmts (PR lto/91572)

[Jakub Jelinek]

On Sat, Aug 31, 2019 at 03:02:18PM +0200, Richard Biener wrote:
> Ok, but I wonder if we can stream the constraint strings in a simpler way - surely the type doesn't really matter?
> Why are they not identifier nodes? 

I guess they have type because they are parsed like any other string
literals during parsing and once we parse them that way, it isn't worth
changing them to something else like an identifier.
Would it be enough to just clear their type during free lang data or
something similar?

	Jakub

Re: [PATCH] Fix free_lang_data on asm stmts (PR lto/91572)

[Richard Biener]

On September 1, 2019 1:55:14 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Sat, Aug 31, 2019 at 03:02:18PM +0200, Richard Biener wrote:
>> Ok, but I wonder if we can stream the constraint strings in a simpler
>way - surely the type doesn't really matter?
>> Why are they not identifier nodes? 
>
>I guess they have type because they are parsed like any other string
>literals during parsing and once we parse them that way, it isn't worth
>changing them to something else like an identifier.

Hmm, so we accept wide-char literals as well here? 

>Would it be enough to just clear their type during free lang data or
>something similar?

I guess so. Maybe do that during gimplification already? Wonder how we pretty print asms then when we mangle their types. Note the original patch is OK as-is I just wondered why we bother to even keep the asm constrains as something different than a char *. 

Richard. 

>
>	Jakub

Re: [PATCH] Fix free_lang_data on asm stmts (PR lto/91572)

[Jakub Jelinek]

On Sun, Sep 01, 2019 at 02:30:12PM +0200, Richard Biener wrote:
> On September 1, 2019 1:55:14 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
> >On Sat, Aug 31, 2019 at 03:02:18PM +0200, Richard Biener wrote:
> >> Ok, but I wonder if we can stream the constraint strings in a simpler
> >way - surely the type doesn't really matter?
> >> Why are they not identifier nodes? 
> >
> >I guess they have type because they are parsed like any other string
> >literals during parsing and once we parse them that way, it isn't worth
> >changing them to something else like an identifier.
> 
> Hmm, so we accept wide-char literals as well here? 
> 
> >Would it be enough to just clear their type during free lang data or
> >something similar?
> 
> I guess so.  Maybe do that during gimplification already?  Wonder how we
> pretty print asms then when we mangle their types.  Note the original
> patch is OK as-is I just wondered why we bother to even keep the asm
> constrains as something different than a char *.

The type was actually const char *, which was the problem on the testcase,
as nothing in the testcase required otherwise const char, so we made it
its own distinct type and then were unhappy that a streamed type was handled
that way.

	Jakub