Bug 87957 - [9 Regression] ICE tree check: expected tree that contains ‘decl minimal’ structure, have ‘identifier_node’ in warn_odr, at ipa-devirt.c:1051 since r265519
Summary: [9 Regression] ICE tree check: expected tree that contains ‘decl minimal’ str...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: ipa (show other bugs)
Version: 9.0
: P1 normal
Target Milestone: 9.0
Assignee: Jan Hubicka
URL:
Keywords: ice-on-valid-code
Depends on:
Blocks:
 
Reported: 2018-11-09 11:37 UTC by Martin Liška
Modified: 2019-02-09 18:10 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work: 8.2.0
Known to fail: 9.0
Last reconfirmed: 2018-11-09 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Liška 2018-11-09 11:37:11 UTC
Following is causing a new ICE:

$ cat 1.ii
typedef struct {
  int a;
} YYSTYPE;
union yyalloc {
  short yyss;
  YYSTYPE yyvs;
};
void b() { yyalloc c; }

$ cat 2.ii
typedef struct YYSTYPE {
} YYSTYPE;
union yyalloc {
  short yyss;
  YYSTYPE yyvs;
};
void a() { yyalloc b; }

$ g++ -flto [12].ii
1.ii:3:3: warning: type ‘struct YYSTYPE’ violates the C++ One Definition Rule [-Wodr]
    3 | } YYSTYPE;
      |   ^
lto1: internal compiler error: tree check: expected tree that contains ‘decl minimal’ structure, have ‘identifier_node’ in warn_odr, at ipa-devirt.c:1051
0x6b540e tree_contains_struct_check_failed(tree_node const*, tree_node_structure_enum, char const*, int, char const*)
	../../gcc/tree.c:9790
0x614bd4 contains_struct_check(tree_node*, tree_node_structure_enum, char const*, int, char const*)
	../../gcc/tree.h:3268
0x614bd4 warn_odr
	../../gcc/ipa-devirt.c:1051
0xa01c9e type_variants_equivalent_p
	../../gcc/ipa-devirt.c:651
0xa07f68 odr_subtypes_equivalent_p
	../../gcc/ipa-devirt.c:697
0xa08faa odr_types_equivalent_p
	../../gcc/ipa-devirt.c:1576
0xa05ea4 add_type_duplicate
	../../gcc/ipa-devirt.c:1878
0xa05ea4 get_odr_type(tree_node*, bool)
	../../gcc/ipa-devirt.c:2058
0x7894d6 lto_read_decls
	../../gcc/lto/lto.c:1900
0x78a78e lto_file_finalize
	../../gcc/lto/lto.c:2134
0x78a78e lto_create_files_from_ids
	../../gcc/lto/lto.c:2144
0x78a78e lto_file_read
	../../gcc/lto/lto.c:2185
0x78a78e read_cgraph_and_symbols
	../../gcc/lto/lto.c:2865
0x78a78e lto_main()
	../../gcc/lto/lto.c:3401
Comment 1 Martin Liška 2018-11-09 12:07:14 UTC
I've got patch for it, let me take it.
Comment 2 Martin Liška 2018-11-09 13:18:17 UTC
There's patch candidate:

diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
index 4676bdbdf93..71cd35caf0c 100644
--- a/gcc/ipa-devirt.c
+++ b/gcc/ipa-devirt.c
@@ -1048,7 +1048,11 @@ warn_odr (tree t1, tree t2, tree st1, tree st2,
     }
   else
     return;
-  inform (DECL_SOURCE_LOCATION (decl2), reason);
+
+  location_t loc = (TYPE_NAME (t1) && TREE_CODE (TYPE_NAME (t1)) == TYPE_DECL
+		    ? DECL_SOURCE_LOCATION (TYPE_NAME (t1))
+		    : UNKNOWN_LOCATION);
+  inform (loc, reason);
 
   if (warned)
     *warned = true;

But now I see repeated Wodr:

$ g++ -flto [12].ii -shared
1.ii:3:3: warning: type ‘struct YYSTYPE’ violates the C++ One Definition Rule [-Wodr]
    3 | } YYSTYPE;
      |   ^
1.ii:3:3: note: a type with different alignment is defined in another translation unit
1.ii:4:7: warning: type ‘union yyalloc’ violates the C++ One Definition Rule [-Wodr]
    4 | union yyalloc {
      |       ^
1.ii:4:7: note: a different type is defined in another translation unit
1.ii:6:11: note: the first difference of corresponding definitions is field ‘yyvs’
    6 |   YYSTYPE yyvs;
      |           ^
1.ii:4:7: note: a field of same name but different type is defined in another translation unit
    4 | union yyalloc {
      |       ^
1.ii:3:3: note: type ‘struct YYSTYPE’ itself violates the C++ One Definition Rule
    3 | } YYSTYPE;
      |   ^
1.ii:3:3: warning: type ‘struct YYSTYPE’ violates the C++ One Definition Rule [-Wodr]
2.ii:1:16: note: a different type is defined in another translation unit
    1 | typedef struct YYSTYPE {
      |                ^
1.ii:2:7: note: the first difference of corresponding definitions is field ‘a’
    2 |   int a;
      |       ^
1.ii:3:3: note: a type with different number of fields is defined in another translation unit
    3 | } YYSTYPE;
      |   ^

Leaving to Honza.
Comment 3 Jan Hubicka 2018-11-17 11:35:33 UTC
Author: hubicka
Date: Sat Nov 17 11:35:01 2018
New Revision: 266235

URL: https://gcc.gnu.org/viewcvs?rev=266235&root=gcc&view=rev
Log:
	PR ipa/87957
	* ipa-devirt.c (warn_odr): Look for main variant to get TYPE_DECL.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-devirt.c
Comment 4 Jan Hubicka 2018-11-17 11:45:01 UTC
ICE fixed, but lets keep the PR open to track the fact that warning is
quite confused.
Comment 5 Martin Liška 2018-11-19 15:03:22 UTC
With current trunk, I still see following problem:

$ cat 1.ii
enum a {} b;

$ cat 2.ii
class a {
  int *b() const;
};
int *a::b() const { return 0; }

$ g++ -flto [12].ii
1.ii:1:6: warning: type ‘a’ violates the C++ One Definition Rule [-Wodr]
    1 | enum a {} b;
      |      ^
1.ii:1:6: note: a different type is defined in another translation unit
during IPA pass: pure-const
lto1: internal compiler error: tree check: expected enumeral_type, have record_type in free_enum_values, at ipa-devirt.c:2276
0x6de7c0 tree_check_failed(tree_node const*, char const*, int, char const*, ...)
	/home/marxin/Programming/gcc/gcc/tree.c:9657
0x6413a6 tree_check(tree_node*, char const*, int, char const*, tree_code)
	/home/marxin/Programming/gcc/gcc/tree.h:3154
0x6413a6 free_enum_values
	/home/marxin/Programming/gcc/gcc/ipa-devirt.c:2276
0xa34174 build_type_inheritance_graph()
	/home/marxin/Programming/gcc/gcc/ipa-devirt.c:2292
0xa6d852 symbol_table::remove_unreachable_nodes(_IO_FILE*)
	/home/marxin/Programming/gcc/gcc/ipa.c:310
0x7b3832 read_cgraph_and_symbols
	/home/marxin/Programming/gcc/gcc/lto/lto.c:3017
0x7b3832 lto_main()
	/home/marxin/Programming/gcc/gcc/lto/lto.c:3401
Comment 6 Jan Hubicka 2018-11-19 18:18:06 UTC
I think we have separate PR for this ICE.  It is the ODR violation
confusing the walk of duplicates. It needs to stop assuming that all
duplicates are same TREE_CODE of type.

I will cook up patch.

Honza
Comment 7 Jan Hubicka 2018-11-19 23:27:42 UTC
Author: hubicka
Date: Mon Nov 19 23:27:10 2018
New Revision: 266289

URL: https://gcc.gnu.org/viewcvs?rev=266289&root=gcc&view=rev
Log:

	PR lto/87957
	* ipa-devirt.c (free_enum_values): Do not ICE on ODR vilations.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-devirt.c
Comment 8 Jan Hubicka 2018-11-20 16:22:51 UTC
Author: hubicka
Date: Tue Nov 20 16:22:19 2018
New Revision: 266322

URL: https://gcc.gnu.org/viewcvs?rev=266322&root=gcc&view=rev
Log:

	PR lto/87957
	* ipa-devirt.c (odr_subtypes_equivalent_p): Report ODR violation
	when sybtype already violates ODR.
	(get_odr_type): Do not ICE when insert is false and type duplicate
	is not registered yet.
	(register_odr_type): Be sure to register subtypes first.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-devirt.c
Comment 9 Jan Hubicka 2018-11-21 17:31:51 UTC
Author: hubicka
Date: Wed Nov 21 17:31:19 2018
New Revision: 266350

URL: https://gcc.gnu.org/viewcvs?rev=266350&root=gcc&view=rev
Log:

	PR lto/87957
	* tree.c (fld_decl_context): Break out from ...
	(free_lang_data_in_decl): ... here; free TREE_PUBLIC, TREE_PRIVATE
	DECL_ARTIFICIAL of TYPE_DECL; do not free TREE_TYPE of TYPE_DECL.
	(fld_incomplete_type_of): Build copy of TYP_DECL.
	* ipa-devirt.c (free_enum_values): Rename to ...
	(free_odr_warning_data): ... this one; free also duplicated TYPE_DECLs
	and TREE_TYPEs of TYPE_DECLs.
	(get_odr_type): Initialize odr_vtable_hash if needed.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-devirt.c
    trunk/gcc/tree.c
Comment 10 Jan Hubicka 2018-11-21 17:32:51 UTC
Author: hubicka
Date: Wed Nov 21 17:32:19 2018
New Revision: 266351

URL: https://gcc.gnu.org/viewcvs?rev=266351&root=gcc&view=rev
Log:

	PR lto/87957
	* g++.dg/lto/odr-1_0.C: Extend by mismatched enum.
	* g++.dg/lto/odr-1_1.C: Extend by mismatched enum.
	* g++.dg/lto/odr-2_0.C: New.
	* g++.dg/lto/odr-2_0.C: New.
	* g++.dg/lto/odr-3_1.C: New.
	* g++.dg/lto/odr-3_1.C: New.

Added:
    trunk/gcc/testsuite/g++.dg/lto/odr-2_0.C
    trunk/gcc/testsuite/g++.dg/lto/odr-2_1.C
    trunk/gcc/testsuite/g++.dg/lto/odr-3_0.C
    trunk/gcc/testsuite/g++.dg/lto/odr-3_1.C
Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/lto/odr-1_0.C
    trunk/gcc/testsuite/g++.dg/lto/odr-1_1.C
Comment 11 Jan Hubicka 2018-11-21 17:33:19 UTC
Fixed.
Comment 12 Rainer Orth 2018-11-23 10:08:13 UTC
Your patch most likely caused a regression:

+FAIL: gnat.dg/lto8.adb (test for excess errors)
+FAIL: gnat.dg/lto8.adb 3 blank line(s) in output
+UNRESOLVED: gnat.dg/lto8.adb compilation failed to produce executable

+===========================GNAT BUG DETECTED==============================+
| 9.0.0 20181122 (experimental) [trunk revision 266382] (i386-pc-solaris2.11) GCC error:|
| in fld_incomplete_type_of, at tree.c:5300                                |
| Error detected around /var/gcc/regression/trunk/11-gcc-gas/build/i386-pc-solaris2.11/./libada/adainclude/s-atacco.ads:39:16|

Seen on i386-pc-solaris2.11 and sparc-sun-solaris2.11 (32 and 64-bit, gcc-testresults
reports also on aarch64-suse-linux-gnu, arm-unknown-linux-gnueabihf, s390x-ibm-linux-gnu default
Comment 13 Dominique d'Humieres 2018-11-29 16:28:20 UTC
> Seen on i386-pc-solaris2.11 and sparc-sun-solaris2.11 (32 and 64-bit,
> gcc-testresults
> reports also on aarch64-suse-linux-gnu, arm-unknown-linux-gnueabihf,
> s390x-ibm-linux-gnu default

Also seen on darwin.
Comment 14 Jakub Jelinek 2018-12-10 13:21:07 UTC
That would be the
             gcc_checking_assert (TREE_TYPE (name) == t);
assert then.  So, what is TREE_TYPE (name) and what is t when this happens?
Comment 15 ro@CeBiTec.Uni-Bielefeld.DE 2018-12-10 14:02:07 UTC
> --- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> That would be the
>              gcc_checking_assert (TREE_TYPE (name) == t);
> assert then.  So, what is TREE_TYPE (name) and what is t when this happens?

Here's what I find (after recompiling tree.c with -g3 -O0):

(gdb) up
#1  0x09777f85 in fld_incomplete_type_of (t=0xfa58cc60, fld=0xfeffd6a8)
    at /vol/gcc/src/hg/trunk/local/gcc/tree.c:5312
5312                  gcc_checking_assert (TREE_TYPE (name) == t);
(gdb) p name
$1 = (tree) 0xfa593000
(gdb) call TREE_TYPE (name)
$2 = (tree) 0xfa58ccc0
(gdb) pt
warning: Expression is not an assignment (and might have no effect)
 <record_type fa58ccc0 system__tasking__protected_objects__operations__communication_block sizes-gimplified volatile visited type_2 BLK
    size <integer_cst fa4058fc type <integer_type faefb0c0 bitsizetype> constant 64>
    unit-size <integer_cst fa405910 type <integer_type faefb060 sizetype> constant 8>
    align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type fa58ccc0
    fields <field_decl fa589e70 self
        type <pointer_type fa5a3a20 system__tasking__task_id type <record_type fa5a3960 system__tasking__ada_task_control_block>
            static unsigned SI
            size <integer_cst fa4058ac constant 32>
            unit-size <integer_cst fa4058c0 constant 4>
            align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type fa542900>
        side-effects volatile unsigned nonaddressable SI /var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/libada/adainclude/s-tpobop.ads:193:7 size <integer_cst fa4058ac 32> unit-size <integer_cst fa4058c0 4>
        align:32 warn_if_not_align:0 offset_align 128
        offset <integer_cst fa4058d4 constant 0>
        bit-offset <integer_cst fa405924 constant 0> context <record_type fa58cc60 system__tasking__protected_objects__operations__communication_block>
        chain <field_decl fa589ec8 enqueued type <boolean_type fa41b480 boolean>
            side-effects volatile unsigned nonaddressable QI /var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/libada/adainclude/s-tpobop.ads:194:7
            size <integer_cst fa4059b0 constant 8>
            unit-size <integer_cst fa4059c4 constant 1>
            align:8 warn_if_not_align:0 offset_align 128 offset <integer_cst fa4058d4 0> bit-offset <integer_cst fa4058ac 32> context <record_type fa58cc60 system__tasking__protected_objects__operations__communication_block> chain <field_decl fa589f20 cancelled>>> context <translation_unit_decl fa40f558 /vol/gcc/src/hg/trunk/local/gcc/testsuite/gnat.dg/lto8.adb>
    Ada size <integer_cst fa558d98 type <integer_type faefb0c0 bitsizetype> constant visited 48>
    pointer_to_this <pointer_type fa58ce40> reference_to_this <reference_type fa58cd20> chain <type_decl fa593000 system__tasking__protected_objects__operations__communication_block>>
(gdb) p t
$3 = (tree) 0xfa58cc60
(gdb) pt
warning: Expression is not an assignment (and might have no effect)
 <record_type fa58cc60 system__tasking__protected_objects__operations__communication_block type_2 BLK
    size <integer_cst fa4058fc type <integer_type faefb0c0 bitsizetype> constant 64>
    unit-size <integer_cst fa405910 type <integer_type faefb060 sizetype> constant 8>
    align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type fa58cc60
    fields <field_decl fa589e70 self
        type <pointer_type fa5a3a20 system__tasking__task_id type <record_type fa5a3960 system__tasking__ada_task_control_block>
            static unsigned SI
            size <integer_cst fa4058ac constant 32>
            unit-size <integer_cst fa4058c0 constant 4>
            align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type fa542900>
        side-effects volatile unsigned nonaddressable SI /var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/libada/adainclude/s-tpobop.ads:193:7 size <integer_cst fa4058ac 32> unit-size <integer_cst fa4058c0 4>
        align:32 warn_if_not_align:0 offset_align 128
        offset <integer_cst fa4058d4 constant 0>
        bit-offset <integer_cst fa405924 constant 0> context <record_type fa58cc60 system__tasking__protected_objects__operations__communication_block>
        chain <field_decl fa589ec8 enqueued type <boolean_type fa41b480 boolean>
            side-effects volatile unsigned nonaddressable QI /var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/libada/adainclude/s-tpobop.ads:194:7
            size <integer_cst fa4059b0 constant 8>
            unit-size <integer_cst fa4059c4 constant 1>
            align:8 warn_if_not_align:0 offset_align 128 offset <integer_cst fa4058d4 0> bit-offset <integer_cst fa4058ac 32> context <record_type fa58cc60 system__tasking__protected_objects__operations__communication_block> chain <field_decl fa589f20 cancelled>>> context <translation_unit_decl fa40f558 /vol/gcc/src/hg/trunk/local/gcc/testsuite/gnat.dg/lto8.adb>
    Ada size <integer_cst fa558d98 type <integer_type faefb0c0 bitsizetype> constant visited 48>
    chain <type_decl fa589f78 system__tasking__protected_objects__operations__communication_block>>
Comment 16 Eric Botcazou 2018-12-11 08:38:26 UTC
> (gdb) pt
> warning: Expression is not an assignment (and might have no effect)
>  <record_type fa58ccc0
> system__tasking__protected_objects__operations__communication_block
> sizes-gimplified volatile visited type_2 BLK

The problem is here ^^^^^^^ I think, i.e. we have a volatile variant.  The problem may come from the Ada front-end, but it would be nice to have Jan's viewpoint.
Comment 17 Eric Botcazou 2018-12-11 08:59:38 UTC
> The problem is here ^^^^^^^ I think, i.e. we have a volatile variant.  The
> problem may come from the Ada front-end, but it would be nice to have Jan's
> viewpoint.

It comes from libgnarl/s-tpobop.ads:

   type Communication_Block is record
      Self      : Task_Id;
      Enqueued  : Boolean := True;
      Cancelled : Boolean := False;
   end record;
   pragma Volatile (Communication_Block);

i.e. the declared type is volatile (but of course the main variant is not).
Comment 18 Jakub Jelinek 2018-12-11 13:54:39 UTC
I guess the middle-end relies on TYPE_NAME to have the TYPE_MAIN_VARIANT type rather than be qualified.  The question if just here and it is possible to cope with that, or elsewhere too.
Comment 19 Jan Hubicka 2018-12-11 16:12:59 UTC
Yeap, the warnings was written at the time all C++ types had TYPE_NAMEs
and other types used IDENTIFIER_NODE. I have chnaged it for memory use
reaosns so only main variants have IDENTIFIER_NODE.  Usually we want to
just look for main variant and complain about that.  I will look into
it.

Honza
Comment 20 Eric Botcazou 2018-12-11 17:57:58 UTC
> I guess the middle-end relies on TYPE_NAME to have the TYPE_MAIN_VARIANT
> type rather than be qualified.  The question if just here and it is possible
> to cope with that, or elsewhere too.

Yes, but only here and only for a couple of weeks, i.e. it's a novelty.
Comment 21 Richard Biener 2018-12-21 11:58:00 UTC
(In reply to Eric Botcazou from comment #20)
> > I guess the middle-end relies on TYPE_NAME to have the TYPE_MAIN_VARIANT
> > type rather than be qualified.  The question if just here and it is possible
> > to cope with that, or elsewhere too.
> 
> Yes, but only here and only for a couple of weeks, i.e. it's a novelty.

Enforced as novelty, yes, but it was my understanding all the time?

OTOH typedef variants can have different qualifiers so I guess it's either
TREE_TYPE (TYPE_NAME (t)) == t [or DECL_ORIGINAL_TYPE?] or
== TYPE_MAIN_VARIANT (t).  But I guess even that restriction is too strict
for variants of the typedef variants...

Still the bug morphed into sth else, the gnat regression.
Comment 22 Eric Botcazou 2018-12-21 15:15:26 UTC
> Enforced as novelty, yes, but it was my understanding all the time?

Why?  In Ada, the type declared in the source code is volatile so the TYPE_DECL points to it.  That's necessary for correct debug info I think.
Comment 23 Jan Hubicka 2019-01-06 22:58:55 UTC
Hello,
sorry for late response. The assert which trigger is there so we do not copy one DECL_NAME multiple times which code would do when there are multiple types that points to same TYPE_NAME being simplified.

Because early debug info is already output we are supposed to translate all TYPE_NAMEs to IDENTIFIER_NODE with exception of main variants of C++ ODR types where TYPE_NAME is preserved and we compute its DECL_ASSEMBLER_NAME so we can identify same types cross-module later.

The name is supposed to be simplified by fld_simplified_type_name.
I am not sure how to get command line for debugger but I assume that it does not simplify type name because type_with_linkage_p returns true which is wrong for Ada types (it tests whether it is C++ ODR type). type_with_linkage_p basically assumes that only C++ types have DECL_NAME of TYPE_DECL which is not true for Ada, so we need to find way to tell these types apart.
Comment 24 Eric Botcazou 2019-01-07 07:51:45 UTC
> I am not sure how to get command line for debugger but I assume that it does
> not simplify type name because type_with_linkage_p returns true which is
> wrong for Ada types (it tests whether it is C++ ODR type).

Run the gnat.dg testsuite and copy-and-paste the command line from the log file without the -q option in the middle.  You'll get:

/home/eric/build/gcc/native/gcc/xgcc -c -I/home/eric/svn/gcc/gcc/testsuite/gnat.dg/ -B/home/eric/build/gcc/native/gcc --RTS=/home/eric/build/gcc/native/x86_64-suse-linux/./libada -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never -gnatws -flto -lm -I- /home/eric/svn/gcc/gcc/testsuite/gnat.dg/lto8.adb
+===========================GNAT BUG DETECTED==============================+
| 9.0.0 20190104 (experimental) [trunk revision 267574] (x86_64-suse-linux) GCC error:|
| in fld_incomplete_type_of, at tree.c:5348                                |
Comment 25 Eric Botcazou 2019-01-07 07:55:53 UTC
Or, alternatively, from the top-level build directory where you have copied the gnat.dg/lto8* files, just: gcc/xgcc -Bgcc -S lto8.adb -flto -I gcc/ada/rts
Comment 26 Jan Hubicka 2019-01-07 15:31:35 UTC
> Run the gnat.dg testsuite and copy-and-paste the command line from the log file
> without the -q option in the middle.  You'll get:
> 
> /home/eric/build/gcc/native/gcc/xgcc -c
> -I/home/eric/svn/gcc/gcc/testsuite/gnat.dg/ -B/home/eric/build/gcc/native/gcc
> --RTS=/home/eric/build/gcc/native/x86_64-suse-linux/./libada
> -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers
> -fdiagnostics-color=never -gnatws -flto -lm -I-
> /home/eric/svn/gcc/gcc/testsuite/gnat.dg/lto8.adb
> +===========================GNAT BUG DETECTED==============================+
> | 9.0.0 20190104 (experimental) [trunk revision 267574] (x86_64-suse-linux) GCC
> error:|
> | in fld_incomplete_type_of, at tree.c:5348                                |

Aha, i did that but ignored the command line. It was long bughunting
week, sorry for that :)

I already did printf debugging and indeed we only need to decide how to
adjust type_with_linkage_p so it returns false for all Ada types.  Maybe
cleanest would be to add flag to TYPE_DECL which C++ FE will set and
other frontends won't, but perhaps there is easier way around. Jason?

Honza
Comment 27 Eric Botcazou 2019-01-07 23:08:19 UTC
> I already did printf debugging and indeed we only need to decide how to
> adjust type_with_linkage_p so it returns false for all Ada types.  Maybe
> cleanest would be to add flag to TYPE_DECL which C++ FE will set and
> other frontends won't, but perhaps there is easier way around. Jason?

What does it mean exactly for a type to have or not have a linkage?
Comment 28 Jan Hubicka 2019-01-07 23:39:34 UTC
> > I already did printf debugging and indeed we only need to decide how to
> > adjust type_with_linkage_p so it returns false for all Ada types.  Maybe
> > cleanest would be to add flag to TYPE_DECL which C++ FE will set and
> > other frontends won't, but perhaps there is easier way around. Jason?
> 
> What does it mean exactly for a type to have or not have a linkage?

Types with linkage are C++ ODR types. They have associated mangled name
(which is after free lang data available by DECL_ASSEMBLER_NAME of
the TYPE_DECL of TYPE_NAME) and if the names match in different units,
one knows that they are same types even if tree representation diverges.

In C++ those are records, unions and enums.

Honza
Comment 29 Eric Botcazou 2019-01-08 00:08:23 UTC
> Types with linkage are C++ ODR types. They have associated mangled name
> (which is after free lang data available by DECL_ASSEMBLER_NAME of
> the TYPE_DECL of TYPE_NAME) and if the names match in different units,
> one knows that they are same types even if tree representation diverges.
> 
> In C++ those are records, unions and enums.

In Ada every user-defined type has this property and the ODR cannot be violated.
Comment 30 Eric Botcazou 2019-02-04 10:27:57 UTC
We cannot change the TYPE_DECL setting in Ada, it has been like this for 25 years and is the root of the translation process for entities in gigi:

/* Similar, but GNAT_ENTITY is assumed to refer to a GNAT type.  Return
   the GCC type corresponding to that entity.  */

tree
gnat_to_gnu_type (Entity_Id gnat_entity)
{
  tree gnu_decl;

  /* The back end never attempts to annotate generic types.  */
  if (Is_Generic_Type (gnat_entity) && type_annotate_only)
     return void_type_node;

  gnu_decl = gnat_to_gnu_entity (gnat_entity, NULL_TREE, false);
  gcc_assert (TREE_CODE (gnu_decl) == TYPE_DECL);

  return TREE_TYPE (gnu_decl);
}

So what happens in fld_incomplete_type_of if the TYPE_DECL doesn't point to the main variant?  The comment reads:

	  /* Build copy of TYPE_DECL in TYPE_NAME if necessary.
	     This is needed for ODR violation warnings to come out right (we
	     want duplicate TYPE_DECLs whenever the type is duplicated because
	     of ODR violation.  Because lang data in the TYPE_DECL may not
	     have been freed yet, rebuild it from scratch and copy relevant
	     fields.  */

There can be no ODR violations for user-defined types in Ada so can we skip it?
Comment 31 Richard Biener 2019-02-04 10:44:40 UTC
I guess we might end up streaming stuff we don't need.  Can't we simply
remove the assert?  We do build the copy using the main variant type
so this seems to be just a consistency check.
Comment 32 Jan Hubicka 2019-02-04 14:49:16 UTC
> I guess we might end up streaming stuff we don't need.  Can't we simply
> remove the assert?  We do build the copy using the main variant type
> so this seems to be just a consistency check.
The consistency check prevents code from creating duplicated copies of
TYPE_DECL (if multiple types could reffer to one TYPE_DECL we would copy
it each time we copy a type).
We really should rewrite it to IDENTIFIER_TYPE for middle-end purposes
because TYPE_DECLs are useful only for C++ ODR types.  It just appeared
that tings works otherwise (i.e. we do not try to produce mangled name
for Ada types) because need_assembler_name_p return false or Ada
frontend makes assembler names of TYPE_DECLs NULL anyway. I will try
adding that check.

Honza
Comment 33 Jan Hubicka 2019-02-08 13:34:02 UTC
Hi,
I am testing the following fix: since we already decided about mangling
we are in fact safe to remove everything that does not have assembler
name on it.

Honza

Index: tree.c
===================================================================
--- tree.c	(revision 268579)
+++ tree.c	(working copy)
@@ -5152,7 +5152,8 @@ fld_simplified_type_name (tree type)
   /* Drop TYPE_DECLs in TYPE_NAME in favor of the identifier in the
      TYPE_DECL if the type doesn't have linkage.
      this must match fld_  */
-  if (type != TYPE_MAIN_VARIANT (type) || ! type_with_linkage_p (type))
+  if (type != TYPE_MAIN_VARIANT (type)
+      || !DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (type)))
     return DECL_NAME (TYPE_NAME (type));
   return TYPE_NAME (type);
 }
Comment 34 Jan Hubicka 2019-02-09 18:10:50 UTC
Fixed.