On Mon, Jul 11, 2016 at 5:14 PM, Jan Hubicka <hubi...@ucw.cz> wrote: > Hi, > Sorry for jumping in late, only now I had chance to read through the whole > discussion. > I was looking into similar problem some time ago. > >> Index: lto-streamer-out.c >> =================================================================== >> --- lto-streamer-out.c (revision 238156) >> +++ lto-streamer-out.c (working copy) >> @@ -996,7 +996,9 @@ hash_tree (struct streamer_tree_cache_d >> else >> hstate.add_flag (TREE_NO_WARNING (t)); >> hstate.add_flag (TREE_NOTHROW (t)); >> - hstate.add_flag (TREE_STATIC (t)); >> + /* We want to unify DECL nodes in pointer fields of global types. */ >> + if (!(VAR_OR_FUNCTION_DECL_P (t))) >> + hstate.add_flag (TREE_STATIC (t)); >> hstate.add_flag (TREE_PROTECTED (t)); >> hstate.add_flag (TREE_DEPRECATED (t)); >> if (code != TREE_BINFO) >> @@ -1050,7 +1052,9 @@ hash_tree (struct streamer_tree_cache_d >> hstate.add_flag (DECL_ARTIFICIAL (t)); >> hstate.add_flag (DECL_USER_ALIGN (t)); >> hstate.add_flag (DECL_PRESERVE_P (t)); >> - hstate.add_flag (DECL_EXTERNAL (t)); >> + /* We want to unify DECL nodes in pointer fields of global types. */ >> + if (!(VAR_OR_FUNCTION_DECL_P (t))) >> + hstate.add_flag (DECL_EXTERNAL (t)); > > It is fine to merge decls across static and external flags, but I am not sure > this is > a safe solution to the problem. In C it is perfectly normal to have one decl > more specified > or with different attributes. Like: > > extern int a __attribute__ ((warning("bar")); > > int a=7 __attribute__ ((warning("foo"))); > > which must prevent merging otherwise the warnings will go out wrong way. In > GCC > 6 timeframe we decided to live with duplicated declarations and added support > for syntactic aliases. Tree merging should be optional WRT correctness, so I > think > bug is in the canonical type calculation: > > /* For array types hash the domain bounds and the string flag. */ > if (TREE_CODE (type) == ARRAY_TYPE && TYPE_DOMAIN (type)) > { > hstate.add_int (TYPE_STRING_FLAG (type)); > /* OMP lowering can introduce error_mark_node in place of > random local decls in types. */ > if (TYPE_MIN_VALUE (TYPE_DOMAIN (type)) != error_mark_node) > inchash::add_expr (TYPE_MIN_VALUE (TYPE_DOMAIN (type)), hstate); > if (TYPE_MAX_VALUE (TYPE_DOMAIN (type)) != error_mark_node) > inchash::add_expr (TYPE_MAX_VALUE (TYPE_DOMAIN (type)), hstate); > } > > which boils down to: > > if (tclass == tcc_declaration) > { > /* DECL's have a unique ID */ > hstate.add_wide_int (DECL_UID (t)); > } > > (in asd_expr)
Well, I think the patch wouldn't merge those it simply treats them the same for references to outside of a tree SCC. So it's kind of a hack... I think the real fix is to apply the linker resolution to the cgraph early (which means reading the cgraph before reading global decls), keeping a decl-ref to symtab node hash for the purpose of tree merging. > It is bit ugly, but I think for static/external/public decls we need to use > assembler name here (as we can't rely on symtab to be around all the time) > which will result in unstability WRT symbol renaming and also give false > positives for static symbols. But even for static symbols it is not guaranteed > they are not duplicated. It may be possible to use combination of assembler > name and translation unit that is available from symtab node, but not all > decls > will have it defined. Yeah, but assembler name is in the cgraph which is not read in yet... (so I wonder how same_global_decl_p in the patch works ...). Oh, it isn't yet in the symbol table... Still I guess it should only compare if DECL_ASSEMBLER_NAME_SET_P. And I think the patch (without the LTO_SET_PREVAIL bits) is sensible. Richard. > > Honza