Refactor IPA devirt a bit.

Message ID fc5d6c34-eb20-b4ba-3fcc-1cb3c48d499e@suse.cz
State New
Headers show
Series
  • Refactor IPA devirt a bit.
Related show

Commit Message

Martin Liška Dec. 2, 2019, 10:46 a.m.
Hello.

The patches makes a small refactoring in ipa-devirt.c and comes up
with a handy debugging function debug_tree_odr_name.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

2019-12-02  Martin Liska  <mliska@suse.cz>

	* ipa-devirt.c (warn_types_mismatch): Use get_odr_name_for_type.
	* print-tree.c (debug_tree_odr_name): New.
	* print-tree.h (debug_tree_odr_name): New.
	* tree.h (get_odr_name_for_type): New.

gcc/testsuite/ChangeLog:

2019-12-02  Martin Liska  <mliska@suse.cz>

	* g++.dg/lto/odr-7_0.C: New test.
	* g++.dg/lto/odr-7_1.C: New test.
---
  gcc/ipa-devirt.c                   | 21 +++++++--------------
  gcc/print-tree.c                   | 17 +++++++++++++++++
  gcc/print-tree.h                   |  1 +
  gcc/testsuite/g++.dg/lto/odr-7_0.C | 18 ++++++++++++++++++
  gcc/testsuite/g++.dg/lto/odr-7_1.C | 13 +++++++++++++
  gcc/tree.h                         | 13 +++++++++++++
  6 files changed, 69 insertions(+), 14 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/lto/odr-7_0.C
  create mode 100644 gcc/testsuite/g++.dg/lto/odr-7_1.C

Comments

Jan Hubicka Dec. 2, 2019, 10:50 a.m. | #1
> Hello.

> 

> The patches makes a small refactoring in ipa-devirt.c and comes up

> with a handy debugging function debug_tree_odr_name.

> 

> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

> 

> Ready to be installed?

> Thanks,

> Martin

> 

> gcc/ChangeLog:

> 

> 2019-12-02  Martin Liska  <mliska@suse.cz>

> 

> 	* ipa-devirt.c (warn_types_mismatch): Use get_odr_name_for_type.

> 	* print-tree.c (debug_tree_odr_name): New.

> 	* print-tree.h (debug_tree_odr_name): New.

> 	* tree.h (get_odr_name_for_type): New.

> 

> gcc/testsuite/ChangeLog:

> 

> 2019-12-02  Martin Liska  <mliska@suse.cz>

> 

> 	* g++.dg/lto/odr-7_0.C: New test.

> 	* g++.dg/lto/odr-7_1.C: New test.


This is OK except ...
> +/* Print ODR name of a TYPE if available.

> +   Use demangler when option DEMANGLE is used.  */

> +

> +DEBUG_FUNCTION void

> +debug_tree_odr_name (tree type, bool demangle)


I would probably keep these in ipa-devirt since it has everything
related to devirtualization in it and there is not much related
to normal tree printing machinery.
> +/* If TYPE has mangled ODR name, return it.  Otherwise return NULL.  */

> +

> +inline const char *

> +get_odr_name_for_type (tree type)


Similarly here I would keep it in ipa-util where all the other ODR
related stuff is (next stage1 we could move it into something like
odr-type.[ch]).

Also document that it works only after free_lang_data was run.

Honza
> +{

> +  tree type_name = TYPE_NAME (type);

> +  if (type_name == NULL_TREE

> +      || !DECL_ASSEMBLER_NAME_SET_P (type_name))

> +    return NULL;

> +

> +  return IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (type_name));

> +}

> +

>  /* A struct for encapsulating location information about an operator

>     and the operation built from it.

>  

>
Martin Liška Dec. 2, 2019, 11:01 a.m. | #2
On 12/2/19 11:50 AM, Jan Hubicka wrote:
>> Hello.

>>

>> The patches makes a small refactoring in ipa-devirt.c and comes up

>> with a handy debugging function debug_tree_odr_name.

>>

>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

>>

>> Ready to be installed?

>> Thanks,

>> Martin

>>

>> gcc/ChangeLog:

>>

>> 2019-12-02  Martin Liska  <mliska@suse.cz>

>>

>> 	* ipa-devirt.c (warn_types_mismatch): Use get_odr_name_for_type.

>> 	* print-tree.c (debug_tree_odr_name): New.

>> 	* print-tree.h (debug_tree_odr_name): New.

>> 	* tree.h (get_odr_name_for_type): New.

>>

>> gcc/testsuite/ChangeLog:

>>

>> 2019-12-02  Martin Liska  <mliska@suse.cz>

>>

>> 	* g++.dg/lto/odr-7_0.C: New test.

>> 	* g++.dg/lto/odr-7_1.C: New test.

> 

> This is OK except ...

>> +/* Print ODR name of a TYPE if available.

>> +   Use demangler when option DEMANGLE is used.  */

>> +

>> +DEBUG_FUNCTION void

>> +debug_tree_odr_name (tree type, bool demangle)

> 

> I would probably keep these in ipa-devirt since it has everything

> related to devirtualization in it and there is not much related

> to normal tree printing machinery.

>> +/* If TYPE has mangled ODR name, return it.  Otherwise return NULL.  */

>> +

>> +inline const char *

>> +get_odr_name_for_type (tree type)

> 

> Similarly here I would keep it in ipa-util where all the other ODR

> related stuff is (next stage1 we could move it into something like

> odr-type.[ch]).

> 

> Also document that it works only after free_lang_data was run.


Sure, I'm sending updated version of the patch.

Ready for trunk now?

Martin

> 

> Honza

>> +{

>> +  tree type_name = TYPE_NAME (type);

>> +  if (type_name == NULL_TREE

>> +      || !DECL_ASSEMBLER_NAME_SET_P (type_name))

>> +    return NULL;

>> +

>> +  return IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (type_name));

>> +}

>> +

>>   /* A struct for encapsulating location information about an operator

>>      and the operation built from it.

>>   

>>

>
From 6126f8f44106cd779a2d77a4755c68bcf6b9fa20 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>

Date: Mon, 2 Dec 2019 09:59:26 +0100
Subject: [PATCH] Refactor IPA devirt a bit.

gcc/ChangeLog:

2019-12-02  Martin Liska  <mliska@suse.cz>

	* ipa-devirt.c (warn_types_mismatch): Use get_odr_name_for_type
	function.
	(debug_tree_odr_name): New.
	* ipa-utils.h (get_odr_name_for_type): New.

gcc/testsuite/ChangeLog:

2019-12-02  Martin Liska  <mliska@suse.cz>

	* g++.dg/lto/odr-7_0.C: New test.
	* g++.dg/lto/odr-7_1.C: New test.
---
 gcc/ipa-devirt.c                   | 37 +++++++++++++++++++-----------
 gcc/ipa-utils.h                    | 14 +++++++++++
 gcc/testsuite/g++.dg/lto/odr-7_0.C | 18 +++++++++++++++
 gcc/testsuite/g++.dg/lto/odr-7_1.C | 13 +++++++++++
 4 files changed, 68 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/lto/odr-7_0.C
 create mode 100644 gcc/testsuite/g++.dg/lto/odr-7_1.C

diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
index a884a465a5d..1017b2a5c7c 100644
--- a/gcc/ipa-devirt.c
+++ b/gcc/ipa-devirt.c
@@ -1036,20 +1036,13 @@ warn_types_mismatch (tree t1, tree t2, location_t loc1, location_t loc2)
   /* If types have mangled ODR names and they are different, it is most
      informative to output those.
      This also covers types defined in different namespaces.  */
-  if (TYPE_NAME (mt1) && TYPE_NAME (mt2)
-      && TREE_CODE (TYPE_NAME (mt1)) == TYPE_DECL
-      && TREE_CODE (TYPE_NAME (mt2)) == TYPE_DECL
-      && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (mt1))
-      && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (mt2))
-      && DECL_ASSEMBLER_NAME (TYPE_NAME (mt1))
-	 != DECL_ASSEMBLER_NAME (TYPE_NAME (mt2)))
-    {
-      char *name1 = xstrdup (cplus_demangle
-	 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (mt1))),
-	  DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES));
-      char *name2 = cplus_demangle
-	 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (mt2))),
-	  DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES);
+  const char *odr1 = get_odr_name_for_type (mt1);
+  const char *odr2 = get_odr_name_for_type (mt2);
+  if (odr1 != NULL && odr2 != NULL && odr1 != odr2)
+    {
+      const int opts = DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES;
+      char *name1 = xstrdup (cplus_demangle (odr1, opts));
+      char *name2 = xstrdup (cplus_demangle (odr2, opts));
       if (name1 && name2 && strcmp (name1, name2))
 	{
 	  inform (loc_t1,
@@ -3989,4 +3982,20 @@ make_pass_ipa_devirt (gcc::context *ctxt)
   return new pass_ipa_devirt (ctxt);
 }
 
+/* Print ODR name of a TYPE if available.
+   Use demangler when option DEMANGLE is used.  */
+
+DEBUG_FUNCTION void
+debug_tree_odr_name (tree type, bool demangle)
+{
+  const char *odr = get_odr_name_for_type (type);
+  if (demangle)
+    {
+      const int opts = DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES;
+      odr = cplus_demangle (odr, opts);
+    }
+
+  fprintf (stderr, "%s\n", odr);
+}
+
 #include "gt-ipa-devirt.h"
diff --git a/gcc/ipa-utils.h b/gcc/ipa-utils.h
index 60c52e0fa53..81a54797558 100644
--- a/gcc/ipa-utils.h
+++ b/gcc/ipa-utils.h
@@ -248,4 +248,18 @@ odr_type_p (const_tree t)
          && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t));
 }
 
+/* If TYPE has mangled ODR name, return it.  Otherwise return NULL.
+   The function works only when free_lang_data is run.  */
+
+inline const char *
+get_odr_name_for_type (tree type)
+{
+  tree type_name = TYPE_NAME (type);
+  if (type_name == NULL_TREE
+      || !DECL_ASSEMBLER_NAME_SET_P (type_name))
+    return NULL;
+
+  return IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (type_name));
+}
+
 #endif  /* GCC_IPA_UTILS_H  */
diff --git a/gcc/testsuite/g++.dg/lto/odr-7_0.C b/gcc/testsuite/g++.dg/lto/odr-7_0.C
new file mode 100644
index 00000000000..e33b8192bc1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/odr-7_0.C
@@ -0,0 +1,18 @@
+// { dg-lto-do link }
+
+struct bar // { dg-lto-message "type name 'bar' should match type name 'foobar<float>'" }
+{
+  int xxx;
+};
+
+struct foo // { dg-lto-warning "8: 'struct foo' violates the C\\+\\+ One Definition Rule" }
+{
+  bar a;
+};
+
+foo myfoo2;
+
+int
+main()
+{
+}
diff --git a/gcc/testsuite/g++.dg/lto/odr-7_1.C b/gcc/testsuite/g++.dg/lto/odr-7_1.C
new file mode 100644
index 00000000000..464bd895900
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/odr-7_1.C
@@ -0,0 +1,13 @@
+template <class T>
+struct foobar
+{
+  int xxx;
+  T pes;
+};
+
+struct foo
+{
+  foobar<float> a;
+};
+
+foo myfoo;
-- 
2.24.0
Jan Hubicka Dec. 2, 2019, 11:06 a.m. | #3
> 

> Sure, I'm sending updated version of the patch.

> 

> Ready for trunk now?


OK, thanks!

Honza
Richard Sandiford Dec. 9, 2019, 12:24 p.m. | #4
Martin Liška <mliska@suse.cz> writes:
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c

> index a884a465a5d..e53461b1f5c 100644

> --- a/gcc/ipa-devirt.c

> +++ b/gcc/ipa-devirt.c

> @@ -1036,20 +1036,13 @@ warn_types_mismatch (tree t1, tree t2, location_t loc1, location_t loc2)

>    /* If types have mangled ODR names and they are different, it is most

>       informative to output those.

>       This also covers types defined in different namespaces.  */

> -  if (TYPE_NAME (mt1) && TYPE_NAME (mt2)

> -      && TREE_CODE (TYPE_NAME (mt1)) == TYPE_DECL

> -      && TREE_CODE (TYPE_NAME (mt2)) == TYPE_DECL

> -      && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (mt1))

> -      && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (mt2))

> -      && DECL_ASSEMBLER_NAME (TYPE_NAME (mt1))

> -	 != DECL_ASSEMBLER_NAME (TYPE_NAME (mt2)))

> -    {

> -      char *name1 = xstrdup (cplus_demangle

> -	 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (mt1))),

> -	  DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES));

> -      char *name2 = cplus_demangle

> -	 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (mt2))),

> -	  DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES);

> +  const char *odr1 = get_odr_name_for_type (mt1);

> +  const char *odr2 = get_odr_name_for_type (mt2);

> +  if (odr1 != NULL && odr2 != NULL && odr1 != odr2)

> +    {

> +      const int opts = DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES;

> +      char *name1 = xstrdup (cplus_demangle (odr1, opts));

> +      char *name2 = xstrdup (cplus_demangle (odr2, opts));


This adds an xstrdup for name2.  Is that intentional or just a pasto?
The old code assumed that the demangler buffer wouldn't be reused by
the diagnostics machinery, but maybe copying the buffer is more robust.
In that case though, we need to free name2 in the same way that we
already free name1.

In the patch below I went for removing the xstrdup, but I can add
the frees instead if that seems better.

> diff --git a/gcc/tree.h b/gcc/tree.h

> index 0f3cc5d7e5a..40a4fde6aec 100644

> --- a/gcc/tree.h

> +++ b/gcc/tree.h

> @@ -6222,6 +6222,19 @@ fndecl_built_in_p (const_tree node, built_in_function name)

>  	  && DECL_FUNCTION_CODE (node) == name);

>  }

>  

> +/* If TYPE has mangled ODR name, return it.  Otherwise return NULL.  */

> +

> +inline const char *

> +get_odr_name_for_type (tree type)

> +{

> +  tree type_name = TYPE_NAME (type);

> +  if (type_name == NULL_TREE

> +      || !DECL_ASSEMBLER_NAME_SET_P (type_name))

> +    return NULL;

> +

> +  return IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (type_name));

> +}

> +

>  /* A struct for encapsulating location information about an operator

>     and the operation built from it.

>  


This drops the TYPE_DECL test from the original code above, which
causes an ICE for C tags.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Thanks,
Richard


2019-12-09  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* ipa-utils.h (get_odr_name_for_type): Check for a TYPE_DECL.
	* ipa-devirt.c (warn_types_mismatch): Don't call xstrdup for the
	second demangled name.

gcc/testsuite/
	* gcc.dg/lto/tag-1_0.c, gcc.dg/lto/tag-1_1.c: New test.

Index: gcc/ipa-utils.h
===================================================================
--- gcc/ipa-utils.h	2019-12-09 12:23:47.000000000 +0000
+++ gcc/ipa-utils.h	2019-12-09 12:23:48.326292463 +0000
@@ -256,6 +256,7 @@ get_odr_name_for_type (tree type)
 {
   tree type_name = TYPE_NAME (type);
   if (type_name == NULL_TREE
+      || TREE_CODE (type_name) != TYPE_DECL
       || !DECL_ASSEMBLER_NAME_SET_P (type_name))
     return NULL;
 
Index: gcc/ipa-devirt.c
===================================================================
--- gcc/ipa-devirt.c	2019-12-09 12:23:47.000000000 +0000
+++ gcc/ipa-devirt.c	2019-12-09 12:23:48.326292463 +0000
@@ -1042,7 +1042,7 @@ warn_types_mismatch (tree t1, tree t2, l
     {
       const int opts = DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES;
       char *name1 = xstrdup (cplus_demangle (odr1, opts));
-      char *name2 = xstrdup (cplus_demangle (odr2, opts));
+      char *name2 = cplus_demangle (odr2, opts);
       if (name1 && name2 && strcmp (name1, name2))
 	{
 	  inform (loc_t1,
Index: gcc/testsuite/gcc.dg/lto/tag-1_0.c
===================================================================
--- /dev/null	2019-09-17 11:41:18.176664108 +0100
+++ gcc/testsuite/gcc.dg/lto/tag-1_0.c	2019-12-09 12:23:48.326292463 +0000
@@ -0,0 +1,5 @@
+/* { dg-lto-do link } */
+/* { dg-lto-options { { -Wodr -flto } } }  */
+
+struct foo { int x; };
+struct foo a = {};
Index: gcc/testsuite/gcc.dg/lto/tag-1_1.c
===================================================================
--- /dev/null	2019-09-17 11:41:18.176664108 +0100
+++ gcc/testsuite/gcc.dg/lto/tag-1_1.c	2019-12-09 12:23:48.326292463 +0000
@@ -0,0 +1,6 @@
+struct foo { short x; };
+
+extern struct foo a; /* { dg-lto-warning {type of 'a' does not match original declaration} } */
+struct foo *ptr = &a;
+
+int main () { return 0; }
Martin Liška Dec. 10, 2019, 8:18 a.m. | #5
On 12/9/19 1:24 PM, Richard Sandiford wrote:
> Martin Liška <mliska@suse.cz> writes:

>> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c

>> index a884a465a5d..e53461b1f5c 100644

>> --- a/gcc/ipa-devirt.c

>> +++ b/gcc/ipa-devirt.c

>> @@ -1036,20 +1036,13 @@ warn_types_mismatch (tree t1, tree t2, location_t loc1, location_t loc2)

>>     /* If types have mangled ODR names and they are different, it is most

>>        informative to output those.

>>        This also covers types defined in different namespaces.  */

>> -  if (TYPE_NAME (mt1) && TYPE_NAME (mt2)

>> -      && TREE_CODE (TYPE_NAME (mt1)) == TYPE_DECL

>> -      && TREE_CODE (TYPE_NAME (mt2)) == TYPE_DECL

>> -      && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (mt1))

>> -      && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (mt2))

>> -      && DECL_ASSEMBLER_NAME (TYPE_NAME (mt1))

>> -	 != DECL_ASSEMBLER_NAME (TYPE_NAME (mt2)))

>> -    {

>> -      char *name1 = xstrdup (cplus_demangle

>> -	 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (mt1))),

>> -	  DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES));

>> -      char *name2 = cplus_demangle

>> -	 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (mt2))),

>> -	  DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES);

>> +  const char *odr1 = get_odr_name_for_type (mt1);

>> +  const char *odr2 = get_odr_name_for_type (mt2);

>> +  if (odr1 != NULL && odr2 != NULL && odr1 != odr2)

>> +    {

>> +      const int opts = DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES;

>> +      char *name1 = xstrdup (cplus_demangle (odr1, opts));

>> +      char *name2 = xstrdup (cplus_demangle (odr2, opts));


Hello.

> 

> This adds an xstrdup for name2.  Is that intentional or just a pasto?


It's a pasto.

> The old code assumed that the demangler buffer wouldn't be reused by

> the diagnostics machinery, but maybe copying the buffer is more robust.

> In that case though, we need to free name2 in the same way that we

> already free name1.

> 

> In the patch below I went for removing the xstrdup, but I can add

> the frees instead if that seems better.


I would not call xstrdup for the second call of get_odr_name_for_type.

> 

>> diff --git a/gcc/tree.h b/gcc/tree.h

>> index 0f3cc5d7e5a..40a4fde6aec 100644

>> --- a/gcc/tree.h

>> +++ b/gcc/tree.h

>> @@ -6222,6 +6222,19 @@ fndecl_built_in_p (const_tree node, built_in_function name)

>>   	  && DECL_FUNCTION_CODE (node) == name);

>>   }

>>   

>> +/* If TYPE has mangled ODR name, return it.  Otherwise return NULL.  */

>> +

>> +inline const char *

>> +get_odr_name_for_type (tree type)

>> +{

>> +  tree type_name = TYPE_NAME (type);

>> +  if (type_name == NULL_TREE

>> +      || !DECL_ASSEMBLER_NAME_SET_P (type_name))

>> +    return NULL;

>> +

>> +  return IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (type_name));

>> +}

>> +

>>   /* A struct for encapsulating location information about an operator

>>      and the operation built from it.

>>   

> 

> This drops the TYPE_DECL test from the original code above, which

> causes an ICE for C tags.


You are right, I can see the ICE during GCC's LTO lean PGO bootstrap.

[ 2779s] ../../gcc/ada/gnatvsn.adb:57:4: warning: type of 'gnatvsn__version_string' does not match original declaration [-Wlto-type-mismatch]
[ 2779s]    57 |    Version_String : char_array (0 .. Ver_Len_Max - 1);
[ 2779s]       |    ^
[ 2779s] lto1: internal compiler error: tree check: expected tree that contains 'decl with visibility' structure, have 'identifier_node' in get_odr_name_for_type, at ipa-utils.h:259
[ 2779s] 0x6c0afa tree_contains_struct_check_failed(tree_node const*, tree_node_structure_enum, char const*, int, char const*)
[ 2779s] 	../../gcc/tree.c:9862
[ 2779s] 0x6172aa contains_struct_check(tree_node*, tree_node_structure_enum, char const*, int, char const*)
[ 2779s] 	../../gcc/tree.h:3387
[ 2779s] 0x6172aa get_odr_name_for_type(tree_node*)
[ 2779s] 	../../gcc/ipa-utils.h:259
[ 2779s] 0x6172aa warn_types_mismatch(tree_node*, tree_node*, unsigned int, unsigned int)
[ 2779s] 	../../gcc/ipa-devirt.c:1040
[ 2779s] 0x77dea4 lto_symtab_merge_decls_2
[ 2779s] 	../../gcc/lto/lto-symtab.c:730
[ 2779s] 0x77dea4 lto_symtab_merge_decls_1
[ 2779s] 	../../gcc/lto/lto-symtab.c:869
[ 2779s] 0x77dea4 lto_symtab_merge_decls()
[ 2779s] 	../../gcc/lto/lto-symtab.c:895
[ 2779s] 0x78852e read_cgraph_and_symbols(unsigned int, char const**)
[ 2779s] 	../../gcc/lto/lto-common.c:2843
[ 2779s] 0x770fd2 lto_main()
[ 2779s] 	../../gcc/lto/lto.c:630

> 

> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?


Not being a maintainer, but the patch seems obvious to me.

Thank you,
Martin

> 

> Thanks,

> Richard

> 

> 

> 2019-12-09  Richard Sandiford  <richard.sandiford@arm.com>

> 

> gcc/

> 	* ipa-utils.h (get_odr_name_for_type): Check for a TYPE_DECL.

> 	* ipa-devirt.c (warn_types_mismatch): Don't call xstrdup for the

> 	second demangled name.

> 

> gcc/testsuite/

> 	* gcc.dg/lto/tag-1_0.c, gcc.dg/lto/tag-1_1.c: New test.

> 

> Index: gcc/ipa-utils.h

> ===================================================================

> --- gcc/ipa-utils.h	2019-12-09 12:23:47.000000000 +0000

> +++ gcc/ipa-utils.h	2019-12-09 12:23:48.326292463 +0000

> @@ -256,6 +256,7 @@ get_odr_name_for_type (tree type)

>   {

>     tree type_name = TYPE_NAME (type);

>     if (type_name == NULL_TREE

> +      || TREE_CODE (type_name) != TYPE_DECL

>         || !DECL_ASSEMBLER_NAME_SET_P (type_name))

>       return NULL;

>   

> Index: gcc/ipa-devirt.c

> ===================================================================

> --- gcc/ipa-devirt.c	2019-12-09 12:23:47.000000000 +0000

> +++ gcc/ipa-devirt.c	2019-12-09 12:23:48.326292463 +0000

> @@ -1042,7 +1042,7 @@ warn_types_mismatch (tree t1, tree t2, l

>       {

>         const int opts = DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES;

>         char *name1 = xstrdup (cplus_demangle (odr1, opts));

> -      char *name2 = xstrdup (cplus_demangle (odr2, opts));

> +      char *name2 = cplus_demangle (odr2, opts);

>         if (name1 && name2 && strcmp (name1, name2))

>   	{

>   	  inform (loc_t1,

> Index: gcc/testsuite/gcc.dg/lto/tag-1_0.c

> ===================================================================

> --- /dev/null	2019-09-17 11:41:18.176664108 +0100

> +++ gcc/testsuite/gcc.dg/lto/tag-1_0.c	2019-12-09 12:23:48.326292463 +0000

> @@ -0,0 +1,5 @@

> +/* { dg-lto-do link } */

> +/* { dg-lto-options { { -Wodr -flto } } }  */

> +

> +struct foo { int x; };

> +struct foo a = {};

> Index: gcc/testsuite/gcc.dg/lto/tag-1_1.c

> ===================================================================

> --- /dev/null	2019-09-17 11:41:18.176664108 +0100

> +++ gcc/testsuite/gcc.dg/lto/tag-1_1.c	2019-12-09 12:23:48.326292463 +0000

> @@ -0,0 +1,6 @@

> +struct foo { short x; };

> +

> +extern struct foo a; /* { dg-lto-warning {type of 'a' does not match original declaration} } */

> +struct foo *ptr = &a;

> +

> +int main () { return 0; }

>
Jan Hubicka Dec. 10, 2019, 8:56 a.m. | #6
> > 

> > 2019-12-09  Richard Sandiford  <richard.sandiford@arm.com>

> > 

> > gcc/

> > 	* ipa-utils.h (get_odr_name_for_type): Check for a TYPE_DECL.

> > 	* ipa-devirt.c (warn_types_mismatch): Don't call xstrdup for the

> > 	second demangled name.

> > 

> > gcc/testsuite/

> > 	* gcc.dg/lto/tag-1_0.c, gcc.dg/lto/tag-1_1.c: New test.


OK,
thanks
Honza
> > 

> > Index: gcc/ipa-utils.h

> > ===================================================================

> > --- gcc/ipa-utils.h	2019-12-09 12:23:47.000000000 +0000

> > +++ gcc/ipa-utils.h	2019-12-09 12:23:48.326292463 +0000

> > @@ -256,6 +256,7 @@ get_odr_name_for_type (tree type)

> >   {

> >     tree type_name = TYPE_NAME (type);

> >     if (type_name == NULL_TREE

> > +      || TREE_CODE (type_name) != TYPE_DECL

> >         || !DECL_ASSEMBLER_NAME_SET_P (type_name))

> >       return NULL;

> > Index: gcc/ipa-devirt.c

> > ===================================================================

> > --- gcc/ipa-devirt.c	2019-12-09 12:23:47.000000000 +0000

> > +++ gcc/ipa-devirt.c	2019-12-09 12:23:48.326292463 +0000

> > @@ -1042,7 +1042,7 @@ warn_types_mismatch (tree t1, tree t2, l

> >       {

> >         const int opts = DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES;

> >         char *name1 = xstrdup (cplus_demangle (odr1, opts));

> > -      char *name2 = xstrdup (cplus_demangle (odr2, opts));

> > +      char *name2 = cplus_demangle (odr2, opts);

> >         if (name1 && name2 && strcmp (name1, name2))

> >   	{

> >   	  inform (loc_t1,

> > Index: gcc/testsuite/gcc.dg/lto/tag-1_0.c

> > ===================================================================

> > --- /dev/null	2019-09-17 11:41:18.176664108 +0100

> > +++ gcc/testsuite/gcc.dg/lto/tag-1_0.c	2019-12-09 12:23:48.326292463 +0000

> > @@ -0,0 +1,5 @@

> > +/* { dg-lto-do link } */

> > +/* { dg-lto-options { { -Wodr -flto } } }  */

> > +

> > +struct foo { int x; };

> > +struct foo a = {};

> > Index: gcc/testsuite/gcc.dg/lto/tag-1_1.c

> > ===================================================================

> > --- /dev/null	2019-09-17 11:41:18.176664108 +0100

> > +++ gcc/testsuite/gcc.dg/lto/tag-1_1.c	2019-12-09 12:23:48.326292463 +0000

> > @@ -0,0 +1,6 @@

> > +struct foo { short x; };

> > +

> > +extern struct foo a; /* { dg-lto-warning {type of 'a' does not match original declaration} } */

> > +struct foo *ptr = &a;

> > +

> > +int main () { return 0; }

> > 

>

Patch

diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
index a884a465a5d..e53461b1f5c 100644
--- a/gcc/ipa-devirt.c
+++ b/gcc/ipa-devirt.c
@@ -1036,20 +1036,13 @@  warn_types_mismatch (tree t1, tree t2, location_t loc1, location_t loc2)
   /* If types have mangled ODR names and they are different, it is most
      informative to output those.
      This also covers types defined in different namespaces.  */
-  if (TYPE_NAME (mt1) && TYPE_NAME (mt2)
-      && TREE_CODE (TYPE_NAME (mt1)) == TYPE_DECL
-      && TREE_CODE (TYPE_NAME (mt2)) == TYPE_DECL
-      && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (mt1))
-      && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (mt2))
-      && DECL_ASSEMBLER_NAME (TYPE_NAME (mt1))
-	 != DECL_ASSEMBLER_NAME (TYPE_NAME (mt2)))
-    {
-      char *name1 = xstrdup (cplus_demangle
-	 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (mt1))),
-	  DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES));
-      char *name2 = cplus_demangle
-	 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (mt2))),
-	  DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES);
+  const char *odr1 = get_odr_name_for_type (mt1);
+  const char *odr2 = get_odr_name_for_type (mt2);
+  if (odr1 != NULL && odr2 != NULL && odr1 != odr2)
+    {
+      const int opts = DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES;
+      char *name1 = xstrdup (cplus_demangle (odr1, opts));
+      char *name2 = xstrdup (cplus_demangle (odr2, opts));
       if (name1 && name2 && strcmp (name1, name2))
 	{
 	  inform (loc_t1,
diff --git a/gcc/print-tree.c b/gcc/print-tree.c
index bd09ec4d7a7..5e728a2641a 100644
--- a/gcc/print-tree.c
+++ b/gcc/print-tree.c
@@ -34,6 +34,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "tree-cfg.h"
 #include "dumpfile.h"
 #include "print-tree.h"
+#include "demangle.h"
 
 /* Define the hash table of nodes already seen.
    Such nodes are not repeated; brief cross-references are used.  */
@@ -1141,6 +1142,22 @@  debug_raw (const tree_node *ptr)
     fprintf (stderr, "<nil>\n");
 }
 
+/* Print ODR name of a TYPE if available.
+   Use demangler when option DEMANGLE is used.  */
+
+DEBUG_FUNCTION void
+debug_tree_odr_name (tree type, bool demangle)
+{
+  const char *odr = get_odr_name_for_type (type);
+  if (demangle)
+    {
+      const int opts = DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES;
+      odr = cplus_demangle (odr, opts);
+    }
+
+  fprintf (stderr, "%s\n", odr);
+}
+
 static void
 dump_tree_via_hooks (const tree_node *ptr, dump_flags_t options)
 {
diff --git a/gcc/print-tree.h b/gcc/print-tree.h
index cbea48c486e..10b8df53ac7 100644
--- a/gcc/print-tree.h
+++ b/gcc/print-tree.h
@@ -23,6 +23,7 @@  along with GCC; see the file COPYING3.  If not see
 extern void debug_tree (tree);
 extern void debug_raw (const tree_node &ref);
 extern void debug_raw (const tree_node *ptr);
+extern void debug_tree_odr_name (tree, bool demangle = true);
 extern void debug (const tree_node &ref);
 extern void debug (const tree_node *ptr);
 extern void debug_verbose (const tree_node &ref);
diff --git a/gcc/testsuite/g++.dg/lto/odr-7_0.C b/gcc/testsuite/g++.dg/lto/odr-7_0.C
new file mode 100644
index 00000000000..e33b8192bc1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/odr-7_0.C
@@ -0,0 +1,18 @@ 
+// { dg-lto-do link }
+
+struct bar // { dg-lto-message "type name 'bar' should match type name 'foobar<float>'" }
+{
+  int xxx;
+};
+
+struct foo // { dg-lto-warning "8: 'struct foo' violates the C\\+\\+ One Definition Rule" }
+{
+  bar a;
+};
+
+foo myfoo2;
+
+int
+main()
+{
+}
diff --git a/gcc/testsuite/g++.dg/lto/odr-7_1.C b/gcc/testsuite/g++.dg/lto/odr-7_1.C
new file mode 100644
index 00000000000..464bd895900
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/odr-7_1.C
@@ -0,0 +1,13 @@ 
+template <class T>
+struct foobar
+{
+  int xxx;
+  T pes;
+};
+
+struct foo
+{
+  foobar<float> a;
+};
+
+foo myfoo;
diff --git a/gcc/tree.h b/gcc/tree.h
index 0f3cc5d7e5a..40a4fde6aec 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -6222,6 +6222,19 @@  fndecl_built_in_p (const_tree node, built_in_function name)
 	  && DECL_FUNCTION_CODE (node) == name);
 }
 
+/* If TYPE has mangled ODR name, return it.  Otherwise return NULL.  */
+
+inline const char *
+get_odr_name_for_type (tree type)
+{
+  tree type_name = TYPE_NAME (type);
+  if (type_name == NULL_TREE
+      || !DECL_ASSEMBLER_NAME_SET_P (type_name))
+    return NULL;
+
+  return IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (type_name));
+}
+
 /* A struct for encapsulating location information about an operator
    and the operation built from it.