On Thu, 27 Jun 2019, Jan Hubicka wrote:
> > > + if (RECORD_OR_UNION_TYPE_P (t)
> > > + && odr_type_p (t) && !odr_type_violation_reported_p (t))
> > > + {
> > > + /* Here we rely on fact that all non-ODR types was inserted into
> > > + canonical type hash and thus we can safely detect conflicts between
> > > + ODR types and interoperable non-ODR types. */
> > > + gcc_checking_assert (type_streaming_finished
> > > + && TYPE_MAIN_VARIANT (t) == t);
> > > + slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash,
> > > + NO_INSERT);
> > > + if (slot && !TYPE_CXX_ODR_P (*(tree *)slot))
> > > + {
> > > + tree nonodr = *(tree *)slot;
> > > + if (symtab->dump_file)
> > > + {
> > > + char *name = cplus_demangle_v3
> > > + (IDENTIFIER_POINTER
> > > + (DECL_ASSEMBLER_NAME (TYPE_NAME (t))), 0);
> >
> > Ugh - do we really want to demangle here? I think not. Surely
> > not without -details or with -slim. Readers can c++filt this
> > easily.
>
> OK, i can dump mangled name.
> Just print_generic_expr prints all instantiations of the tempalte same
> way which makes it look odd that there are so many types of same name.
> > > + /* Set canonical for T and all other ODR equivalent duplicates
> > > + including incomplete structures. */
> > > + set_type_canonical_for_odr_type (t, prevail);
> > > + enable_odr_based_tbaa (t);
> >
> > I suppose there never will be a set of ODR types with the same
> > prevailing type but some of them having a conflict with a nonodr
> > type and some not?
>
> No, we check them by structural equality while verifying ODR violations
> that is more strict than gimple_canonical_types_compatible.
> >
> > > + if (!type_in_anonymous_namespace_p (t))
> > > + hash = htab_hash_string (IDENTIFIER_POINTER
> > > + (DECL_ASSEMBLER_NAME
> > > + (TYPE_NAME (prevail))));
> > > + else
> > > + hash = TYPE_UID (TYPE_MAIN_VARIANT (t));
> > > + num_canonical_type_hash_entries++;
> > > + bool existed_p = canonical_type_hash_cache->put (prevail, hash);
> >
> > but those hash differently? I think you wanted to put t, not prevail
> > here. And you want to use the TYPE_UID of prevail as well?
>
> Actually this was intended to be prevail in both cases, but it is
> harmless. The difference here is that anonymous types have
> DECL_ASSEMBLED_NAME <anon>, so if we inserted them to the hash
> table based on names they will all conflict.
>
> Anonymous namespace types never have duplicated main variants, so
> t==prevail here, but I will update the patch to use prevail since it is
> much more obvious (I am also not sure what I was thinking of when
> I typed TYPE_MAIN_VARIANT)
I think using 't' is more obvious here since that's what we should cache
the hash value for. That is, even in the !type_in_anonymous_namespace_p
(t) case you want to cache 't's hash, not prevails. But yes,
TYPE_UID (prevail) might be more obvious to you.
> >
> > Otherwise looks good.
> >
> > You can commit the C++ FE change with the adjustment in case it
> > fixes the reported verification ICEs.
>
> It only fixes ICE WRT the sanity checking that all non-ODR types
> are inserted first. W/o that change the as-base type will get inserted
> during streaming in and recursively we will insert ODR types early.
>
> I am waiting for testcase WRT the other ODR ICE reported today. I think
> tree.c when creating type variants wants to copy the flag: the wrong
> type is va_list which is likely created by a backend bypassing the C++
> hook.
>
> I will re-test with these changes.
>
> Honza
>
--
Richard Biener <rguent...@suse.de>
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
