public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [C frontend] Fix construction of TYPE_STUB_DECL
@ 2015-05-10 17:34 Jan Hubicka
  2015-05-11  7:53 ` Richard Biener
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Jan Hubicka @ 2015-05-10 17:34 UTC (permalink / raw)
  To: gcc-patches, joseph, rth

Hi,
TREE_PUBLIC of TYPE_DECL is defined to say if the type is public:
/* In a VAR_DECL, FUNCTION_DECL, NAMESPACE_DECL or TYPE_DECL,                   
   nonzero means name is to be accessible from outside this translation unit.   
   In an IDENTIFIER_NODE, nonzero means an external declaration                 
   accessible from outside this translation unit was previously seen            
   for this name in an inner scope.  */                                         
#define TREE_PUBLIC(NODE) ((NODE)->base.public_flag)                            

This is properly honored by C++ FE but other FEs are bit random, which in turn
confuses type_in_anonymous_namespace_p predicate that leads to flase poistives
on type mismatch warnings.  I used to be able to get around by checking only
C++ types at LTO time, but with type checking in lto-symtab I can not, because
I do want to compare type compatibility cross translation units and cross languages
and we have no reliable way to say what type originated as C++ and what did not.

This fixed TYPE_STUB_DECl construction in C frontend. I will check other
FEs separately. I can also add way to recognize C++ types, but I think it is
good idea to make type representation consistent across FEs.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* c-decl.c (pushtag): Declare type as public.
Index: c/c-decl.c
===================================================================
--- c/c-decl.c	(revision 222981)
+++ c/c-decl.c	(working copy)
@@ -1563,6 +1563,7 @@ pushtag (location_t loc, tree name, tree
 
   TYPE_STUB_DECL (type) = pushdecl (build_decl (loc,
 						TYPE_DECL, NULL_TREE, type));
+  TREE_PUBLIC (TYPE_STUB_DECL (type)) = 1;
 
   /* An approximation for now, so we can tell this is a function-scope tag.
      This will be updated in pop_scope.  */

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-10 17:34 [C frontend] Fix construction of TYPE_STUB_DECL Jan Hubicka
@ 2015-05-11  7:53 ` Richard Biener
  2015-05-11 10:57   ` Jan Hubicka
  2015-05-11 18:40 ` Jeff Law
  2015-05-11 21:26 ` Jason Merrill
  2 siblings, 1 reply; 15+ messages in thread
From: Richard Biener @ 2015-05-11  7:53 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: GCC Patches, Joseph S. Myers, Richard Henderson

On Sun, May 10, 2015 at 7:33 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> TREE_PUBLIC of TYPE_DECL is defined to say if the type is public:
> /* In a VAR_DECL, FUNCTION_DECL, NAMESPACE_DECL or TYPE_DECL,
>    nonzero means name is to be accessible from outside this translation unit.
>    In an IDENTIFIER_NODE, nonzero means an external declaration
>    accessible from outside this translation unit was previously seen
>    for this name in an inner scope.  */
> #define TREE_PUBLIC(NODE) ((NODE)->base.public_flag)
>
> This is properly honored by C++ FE but other FEs are bit random, which in turn
> confuses type_in_anonymous_namespace_p predicate that leads to flase poistives
> on type mismatch warnings.  I used to be able to get around by checking only
> C++ types at LTO time, but with type checking in lto-symtab I can not, because
> I do want to compare type compatibility cross translation units and cross languages
> and we have no reliable way to say what type originated as C++ and what did not.

The idea was you can walk up the context chain until you reach a
TRANSLATION_UNIT_DECL
where the source language is encoded in...  of course most FEs are
quite lazy here and
eventually end up at NULL_TREE instead.

Richard.

> This fixed TYPE_STUB_DECl construction in C frontend. I will check other
> FEs separately. I can also add way to recognize C++ types, but I think it is
> good idea to make type representation consistent across FEs.
>
> Bootstrapped/regtested x86_64-linux, OK?
>
> Honza
>
>         * c-decl.c (pushtag): Declare type as public.
> Index: c/c-decl.c
> ===================================================================
> --- c/c-decl.c  (revision 222981)
> +++ c/c-decl.c  (working copy)
> @@ -1563,6 +1563,7 @@ pushtag (location_t loc, tree name, tree
>
>    TYPE_STUB_DECL (type) = pushdecl (build_decl (loc,
>                                                 TYPE_DECL, NULL_TREE, type));
> +  TREE_PUBLIC (TYPE_STUB_DECL (type)) = 1;
>
>    /* An approximation for now, so we can tell this is a function-scope tag.
>       This will be updated in pop_scope.  */

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-11  7:53 ` Richard Biener
@ 2015-05-11 10:57   ` Jan Hubicka
  0 siblings, 0 replies; 15+ messages in thread
From: Jan Hubicka @ 2015-05-11 10:57 UTC (permalink / raw)
  To: Richard Biener
  Cc: Jan Hubicka, GCC Patches, Joseph S. Myers, Richard Henderson

> On Sun, May 10, 2015 at 7:33 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Hi,
> > TREE_PUBLIC of TYPE_DECL is defined to say if the type is public:
> > /* In a VAR_DECL, FUNCTION_DECL, NAMESPACE_DECL or TYPE_DECL,
> >    nonzero means name is to be accessible from outside this translation unit.
> >    In an IDENTIFIER_NODE, nonzero means an external declaration
> >    accessible from outside this translation unit was previously seen
> >    for this name in an inner scope.  */
> > #define TREE_PUBLIC(NODE) ((NODE)->base.public_flag)
> >
> > This is properly honored by C++ FE but other FEs are bit random, which in turn
> > confuses type_in_anonymous_namespace_p predicate that leads to flase poistives
> > on type mismatch warnings.  I used to be able to get around by checking only
> > C++ types at LTO time, but with type checking in lto-symtab I can not, because
> > I do want to compare type compatibility cross translation units and cross languages
> > and we have no reliable way to say what type originated as C++ and what did not.
> 
> The idea was you can walk up the context chain until you reach a
> TRANSLATION_UNIT_DECL
> where the source language is encoded in...  of course most FEs are
> quite lazy here and
> eventually end up at NULL_TREE instead.

Yep, I can walk up and look for NAMESPACE_DECL without TREE_PUBLIC flag, too,
or cleanup the flag in free_lang_data when I know if we do C++ (probably).
The predicate for anonymous_namespace is called on few quite hot paths in
devirt code, but that is not my main concern either.

In general I think we ought to fix frontends to agree with each other (and
docs) on how to represent semantics instead of adding special cases and
workarounds to middle-end/LTO.  I am trying to keep the anonymous namespace
thing indepent of C++. While I am not sure we support other language with
similar notion, it seems to make sense independently of C++. After all, it
gives the type escape info for free :)

Honza
> 
> Richard.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-10 17:34 [C frontend] Fix construction of TYPE_STUB_DECL Jan Hubicka
  2015-05-11  7:53 ` Richard Biener
@ 2015-05-11 18:40 ` Jeff Law
  2015-05-11 18:53   ` Jan Hubicka
  2015-05-11 21:26 ` Jason Merrill
  2 siblings, 1 reply; 15+ messages in thread
From: Jeff Law @ 2015-05-11 18:40 UTC (permalink / raw)
  To: Jan Hubicka, gcc-patches, joseph, rth

On 05/10/2015 11:33 AM, Jan Hubicka wrote:
> Hi,
> TREE_PUBLIC of TYPE_DECL is defined to say if the type is public:
> /* In a VAR_DECL, FUNCTION_DECL, NAMESPACE_DECL or TYPE_DECL,
>     nonzero means name is to be accessible from outside this translation unit.
>     In an IDENTIFIER_NODE, nonzero means an external declaration
>     accessible from outside this translation unit was previously seen
>     for this name in an inner scope.  */
> #define TREE_PUBLIC(NODE) ((NODE)->base.public_flag)
>
> This is properly honored by C++ FE but other FEs are bit random, which in turn
> confuses type_in_anonymous_namespace_p predicate that leads to flase poistives
> on type mismatch warnings.  I used to be able to get around by checking only
> C++ types at LTO time, but with type checking in lto-symtab I can not, because
> I do want to compare type compatibility cross translation units and cross languages
> and we have no reliable way to say what type originated as C++ and what did not.
>
> This fixed TYPE_STUB_DECl construction in C frontend. I will check other
> FEs separately. I can also add way to recognize C++ types, but I think it is
> good idea to make type representation consistent across FEs.
>
> Bootstrapped/regtested x86_64-linux, OK?
>
> Honza
>
> 	* c-decl.c (pushtag): Declare type as public.
What I'm struggling with here is how do you know the stub decl is 
public?  I realize these things are a bit special, but I don't see the 
C++ front-end doing anything similar.  What am I missing?

jeff

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-11 18:40 ` Jeff Law
@ 2015-05-11 18:53   ` Jan Hubicka
  0 siblings, 0 replies; 15+ messages in thread
From: Jan Hubicka @ 2015-05-11 18:53 UTC (permalink / raw)
  To: Jeff Law; +Cc: Jan Hubicka, gcc-patches, joseph, rth, jason

> >Bootstrapped/regtested x86_64-linux, OK?
> >
> >Honza
> >
> >	* c-decl.c (pushtag): Declare type as public.
> What I'm struggling with here is how do you know the stub decl is
> public?  I realize these things are a bit special, but I don't see
> the C++ front-end doing anything similar.  What am I missing?

It is used by type_in_anonymous_namespace_p. C++ FE definitely sets them accordingly,
but I have no idea how it is done.  Jason, can you help?

Honza
> 
> jeff

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-10 17:34 [C frontend] Fix construction of TYPE_STUB_DECL Jan Hubicka
  2015-05-11  7:53 ` Richard Biener
  2015-05-11 18:40 ` Jeff Law
@ 2015-05-11 21:26 ` Jason Merrill
  2015-05-11 21:39   ` Jason Merrill
  2015-05-11 21:59   ` Jan Hubicka
  2 siblings, 2 replies; 15+ messages in thread
From: Jason Merrill @ 2015-05-11 21:26 UTC (permalink / raw)
  To: gcc-patches

On 05/10/2015 12:33 PM, Jan Hubicka wrote:
> This is properly honored by C++ FE but other FEs are bit random, which in turn
> confuses type_in_anonymous_namespace_p predicate that leads to flase poistives
> on type mismatch warnings.  I used to be able to get around by checking only
> C++ types at LTO time, but with type checking in lto-symtab I can not, because
> I do want to compare type compatibility cross translation units and cross languages
> and we have no reliable way to say what type originated as C++ and what did not.

I think we should, as only C++ declarations are subject to the ODR.  C 
has different (structural) compatibility rules, and I think they should 
apply when comparing C and C++ types.

Since C struct names have no linkage, I don't think it's right to set 
TREE_PUBLIC on them.

Jason


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-11 21:26 ` Jason Merrill
@ 2015-05-11 21:39   ` Jason Merrill
  2015-05-11 21:59   ` Jan Hubicka
  1 sibling, 0 replies; 15+ messages in thread
From: Jason Merrill @ 2015-05-11 21:39 UTC (permalink / raw)
  To: Jan Hubicka, gcc-patches, joseph, rth

On 05/10/2015 12:33 PM, Jan Hubicka wrote:
> This is properly honored by C++ FE but other FEs are bit random, which in turn
> confuses type_in_anonymous_namespace_p predicate that leads to flase poistives
> on type mismatch warnings.  I used to be able to get around by checking only
> C++ types at LTO time, but with type checking in lto-symtab I can not, because
> I do want to compare type compatibility cross translation units and cross languages
> and we have no reliable way to say what type originated as C++ and what did not.

I think we should, as only C++ declarations are subject to the ODR.  C 
has different (structural) compatibility rules, and I think they should 
apply when comparing C and C++ types.

Since C struct names have no linkage, I don't think it's right to set 
TREE_PUBLIC on them.

Jason

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-11 21:26 ` Jason Merrill
  2015-05-11 21:39   ` Jason Merrill
@ 2015-05-11 21:59   ` Jan Hubicka
  2015-05-13 16:58     ` Jan Hubicka
  1 sibling, 1 reply; 15+ messages in thread
From: Jan Hubicka @ 2015-05-11 21:59 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Jan Hubicka, gcc-patches, joseph, rth

> On 05/10/2015 12:33 PM, Jan Hubicka wrote:
> >This is properly honored by C++ FE but other FEs are bit random, which in turn
> >confuses type_in_anonymous_namespace_p predicate that leads to flase poistives
> >on type mismatch warnings.  I used to be able to get around by checking only
> >C++ types at LTO time, but with type checking in lto-symtab I can not, because
> >I do want to compare type compatibility cross translation units and cross languages
> >and we have no reliable way to say what type originated as C++ and what did not.
> 
> I think we should, as only C++ declarations are subject to the ODR.
> C has different (structural) compatibility rules, and I think they
> should apply when comparing C and C++ types.
> 
> Since C struct names have no linkage, I don't think it's right to
> set TREE_PUBLIC on them.

Yes, I need safe way to tell what type is subject to ODR and what is not.
I am not quite sure what is the best approach here. This is what the code is doing
currently:

To detect ODR types at LTO time I use combination of presence of mangled name
and type_in_anonymous_namespace_p check. (The idea is that we do not really need
to mangle anonymous type as we do not need to deal with cross-module merging.):

odr_type_p (const_tree t)
{
  if (type_in_anonymous_namespace_p (t))
    return true;
  /* We do not have this information when not in LTO, but we do not need
     to care, since it is used only for type merging.  */
  gcc_assert (in_lto_p || flag_lto);

  return (TYPE_NAME (t)
          && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t))));
}
bool
type_in_anonymous_namespace_p (const_tree t)
{
  /* TREE_PUBLIC of TYPE_STUB_DECL may not be properly set for
     bulitin types; those have CONTEXT NULL.  */
  if (!TYPE_CONTEXT (t))
    return false;
  return (TYPE_STUB_DECL (t) && !TREE_PUBLIC (TYPE_STUB_DECL (t)));
}

the catch is that type_in_anonymous_namespace_p returns true for some C types.
The TYPE_CONTEXT test is already hack as I run into cases of pre-streamed LTO
types to get injected into C++ classes (i.e. if you refere to int, LTO
streaming will replace C++ int with TREE_PUBLIC (TYPE_STUB_DECL (t)) by its own
int that is !TREE_PUBLIC (TYPE_STUB_DECL (t)).  The hack to avoid builtin types
made type_in_anonymous_namespace_p to work well in cases I needed for class
types. (I believe it is because C builds record with
 TREE_PUBLIC (TYPE_STUB_DECL (t))=1 but it is a while I checked this)

We could certainly just add a flag TYPE_ODR_P that says "this type is
controlled by odr rule".  I considered that but it is generally not so nice
to introduce new flags and it seemed to me that because C
standard allows to match all types cross-module on structural basis, it makes
sense to consider them all as having public linkage because they are
"accessible from outside this translation unit." as tree.h defines the flag.

I would be happy with TYPE_ODR_P or any other solution that looks better.

Honza

> 
> Jason

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-11 21:59   ` Jan Hubicka
@ 2015-05-13 16:58     ` Jan Hubicka
  2015-05-21 21:12       ` Jan Hubicka
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Hubicka @ 2015-05-13 16:58 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Jason Merrill, gcc-patches, joseph, rth

Hello,
it seems that the discussion here got stuck without arriving to a consensus.
I generally see three options here
 1) make TYPE_PUBLIC flag of TYPE_STUB_DECL to work consistently across frontends
    in a sense that types with flag !TYPE_PUBLIC (TYPE_STUB_DECL (t)) can be
    considered local to the translation unit and thus we can consider these types
    non-escaping (possibly changing the layout) and TBAA incompatible with types
    from other units for LTO.
 2) Add TYPE_ODR flag that will mark all types that comply the ODR rule.  This
    would be ideally set on C++ types by the FE.
 3) Make type_in_anonymous_namespace to walk context and look for non-public
    namespace instead of relying on TYPE_STUB_DECL alone and if TREE_PUBLIC
    flag on namespaces turns out to be not consistent across FEs, it may
    walk up to check TRANSLATION_UNIT_DECL and work out if it is C++ lang.

In general I still lean toward 1 as it has hope to be useful for non-C++
languages that have notion of local types. I am trying to keep all the
ODR/devirt code as generic as posisble to make it useful for non-C++ based
languages, too. 1) is one of very few cases where we can "solve" type escape.

3) feels like a hack and would make type_in_anonymous_namespace
non-constant (probably still cheap enough for my needs).  It seems like
something useful for verify_type to check that 1) or 2) works as expected.

Any solution here would let me to apply
https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01061.html which otherwise
triggers false positives on Firefox. Mixing C and C++ units makes us to
think that one of global vars (originating from C unit) is declared with type
in anonymous namespace and thus can not match declaration from other unit.
This is workaroundable, too, because C++ variables in anonymous namespaces
are never global so I know all those warnings are false positives, but I
would preffer to not go this way.

Together with the -Wodr warning on types the patch
https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01061.html tests that the ODR
equivalency as established by ipa-devrt at linktime exactly match what C++
standard says - if it was wrongly defining two different types equivalent, we
will eventually get ODR type mismatch warning.  If it was wrongly defining two
types different, we will get eventually incompatible declaration warning. I
would like to get this tested and keep an eye at ODR warnings double checking
that they really indicate bugs in compiled programs and not bugs in GCC.
(current implementation seems correct so far on the bigger apps I built)

Moreover the patch is useful to catch some real bugs in Firefox or Libreoffice.

Honza

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-13 16:58     ` Jan Hubicka
@ 2015-05-21 21:12       ` Jan Hubicka
  2015-05-21 21:34         ` Jason Merrill
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Hubicka @ 2015-05-21 21:12 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Jason Merrill, gcc-patches, joseph, rth

Hello,
I would like to ping this. Currently we have a problem with Ada ICE because we
consider a global variable produced by ada to have type in C++ anonymous
namespace and we get false ODR merging wraning compiling clang because we
consider instances of templates with parameter in anonymous namespace
non-anonymous. I do not think I can make type_in_anonymous_namespace_p to
work reliably at LTO time without frontend change.

This is not really hard thing to do - we only need to decide on a representation.
I think at LTO time it is useful to have two things
  - be able to say what type comply C++ ODR rule, because we special case these
    for ODR warnings
  - be able to say at LTO time what types are anonymous, that is they are not
    compatible with any type from other unit.

Either a special flag TYPE_ODR_P set by C++ FE on all types that comply ODR rule
or making TYPE_PUBLIC (TYPE_STUB_DECL (t)) to reliably identify that type is
anonymous (or an equivalent) would fix the two issues.

Honza

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-21 21:12       ` Jan Hubicka
@ 2015-05-21 21:34         ` Jason Merrill
  2015-05-21 21:58           ` Jan Hubicka
  2015-05-24  8:34           ` Jan Hubicka
  0 siblings, 2 replies; 15+ messages in thread
From: Jason Merrill @ 2015-05-21 21:34 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches, joseph, rth

On 05/21/2015 04:32 PM, Jan Hubicka wrote:
> I think at LTO time it is useful to have two things
>    - be able to say what type comply C++ ODR rule, because we special case these
>      for ODR warnings
>    - be able to say at LTO time what types are anonymous, that is they are not
>      compatible with any type from other unit.

...where the second group is a subset of the first; in languages that 
use structural compatibility, anonymous types can be compatible.

> Either a special flag TYPE_ODR_P set by C++ FE on all types that comply ODR rule
> or making TYPE_PUBLIC (TYPE_STUB_DECL (t)) to reliably identify that type is
> anonymous (or an equivalent) would fix the two issues.

It seems that we've been establishing which types have linkage by 
setting their DECL_ASSEMBLER_NAME.  It seems that the problem is coming 
from leaving it unset for types with internal (anonymous) linkage, so 
you're trying to check TREE_PUBLIC instead, which breaks with other 
front ends. Perhaps we should set DECL_ASSEMBLER_NAME for internal C++ 
types to some magic value?

Jason

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-21 21:34         ` Jason Merrill
@ 2015-05-21 21:58           ` Jan Hubicka
  2015-05-22  7:15             ` Jason Merrill
  2015-05-24  8:34           ` Jan Hubicka
  1 sibling, 1 reply; 15+ messages in thread
From: Jan Hubicka @ 2015-05-21 21:58 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Jan Hubicka, gcc-patches, joseph, rth

> On 05/21/2015 04:32 PM, Jan Hubicka wrote:
> >I think at LTO time it is useful to have two things
> >   - be able to say what type comply C++ ODR rule, because we special case these
> >     for ODR warnings
> >   - be able to say at LTO time what types are anonymous, that is they are not
> >     compatible with any type from other unit.
> 
> ...where the second group is a subset of the first; in languages
> that use structural compatibility, anonymous types can be
> compatible.
> 
> >Either a special flag TYPE_ODR_P set by C++ FE on all types that comply ODR rule
> >or making TYPE_PUBLIC (TYPE_STUB_DECL (t)) to reliably identify that type is
> >anonymous (or an equivalent) would fix the two issues.
> 
> It seems that we've been establishing which types have linkage by
> setting their DECL_ASSEMBLER_NAME.  It seems that the problem is
> coming from leaving it unset for types with internal (anonymous)
> linkage, so you're trying to check TREE_PUBLIC instead, which breaks
> with other front ends. Perhaps we should set DECL_ASSEMBLER_NAME for
> internal C++ types to some magic value?

This would also work, yes.  We can set it into something like "<anonymous>".
One problem would be that type_with_linkage_p/type_in_anonymous_namespace_p
would not work on non-C++ types without LTO (because then we do not produce the
type manglings). I suppose it is not really a problem because only
place I use them is the devirtualization machinery and that won't get any
polymorphic types for other languages.

I suppose I can drop type_in_anonymous_namespcae_p in tree.c and make
mangle_decl to set "anonymous" for those?

Honza

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-21 21:58           ` Jan Hubicka
@ 2015-05-22  7:15             ` Jason Merrill
  0 siblings, 0 replies; 15+ messages in thread
From: Jason Merrill @ 2015-05-22  7:15 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches, joseph, rth

On 05/21/2015 05:34 PM, Jan Hubicka wrote:
> This would also work, yes.  We can set it into something like "<anonymous>".

> One problem would be that type_with_linkage_p/type_in_anonymous_namespace_p
> would not work on non-C++ types without LTO (because then we do not produce the
> type manglings). I suppose it is not really a problem because only
> place I use them is the devirtualization machinery and that won't get any
> polymorphic types for other languages.

And I don't know whether types from any other languages have linkage 
like C++ times, so that's fine.

> I suppose I can drop type_in_anonymous_namespcae_p in tree.c and make
> mangle_decl to set "anonymous" for those?

Sounds good.

Jason


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-21 21:34         ` Jason Merrill
  2015-05-21 21:58           ` Jan Hubicka
@ 2015-05-24  8:34           ` Jan Hubicka
  2015-05-25 19:44             ` Jason Merrill
  1 sibling, 1 reply; 15+ messages in thread
From: Jan Hubicka @ 2015-05-24  8:34 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Jan Hubicka, gcc-patches, joseph, rth

Hi,
this patch implements the idea.  It uses "<anon>" for anonymous namespace
types and updates ipa-devirt to rely on it (and also sanity check that).
The patch has bootstrapped/regtested powerpc64le-linux, will commit it
tomorrow if there are no complains to unbreak the Ada LTO bootstrap.

Honza

	PR lto/66180
	* mangle.c (mangle_decl): Mangle anonymous namespace types as "<anon>"
	* ipa-devirt.c (type_with_linkage): CHeck that TYPE_STUB_DECL
	is set; check for assembler name at LTO time.
	(type_in_anonymous_namespace): Remove hacks, check that all
	anonymous types are called "<anon>"
	(odr_type_p): Simplify; add check for "<anon>"
	(odr_subtypes_equivalent): Add odr_type_p check.
	* tree.c (need_assembler_name_p): Even anonymous namespace
	needs assembler name.

	* g++.dg/lto/pr66180_0.C: New testcase.
	* g++.dg/lto/pr66180_1.C: New testcase.
Index: cp/mangle.c
===================================================================
--- cp/mangle.c	(revision 223628)
+++ cp/mangle.c	(working copy)
@@ -3511,7 +3511,20 @@
   if (dep)
     return;
 
-  id = get_mangled_id (decl);
+  /* During LTO we keep mangled names of TYPE_DECLs for ODR type merging.
+     It is not needed to assign names to anonymous namespace, but we use the
+     "<anon>" marker to be able to tell if type is C++ ODR type or type
+     produced by other language.  */
+  if (TREE_CODE (decl) == TYPE_DECL
+      && TYPE_STUB_DECL (TREE_TYPE (decl))
+      && !TREE_PUBLIC (TYPE_STUB_DECL (TREE_TYPE (decl))))
+    id = get_identifier ("<anon>");
+  else
+    {
+      gcc_assert (TREE_CODE (decl) != TYPE_DECL
+		  || !no_linkage_check (TREE_TYPE (decl), true));
+      id = get_mangled_id (decl);
+    }
   SET_DECL_ASSEMBLER_NAME (decl, id);
 
   if (G.need_abi_warning
Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c	(revision 223629)
+++ ipa-devirt.c	(working copy)
@@ -252,9 +252,15 @@
 {
   /* Builtin types do not define linkage, their TYPE_CONTEXT is NULL.  */
   if (!TYPE_CONTEXT (t)
-      || !TYPE_NAME (t) || TREE_CODE (TYPE_NAME (t)) != TYPE_DECL)
+      || !TYPE_NAME (t) || TREE_CODE (TYPE_NAME (t)) != TYPE_DECL
+      || !TYPE_STUB_DECL (t))
     return false;
 
+  /* In LTO do not get confused by non-C++ produced types or types built
+     with -fno-lto-odr-type-merigng.  */
+  if (in_lto_p && !DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))
+    return false;
+
   return (RECORD_OR_UNION_TYPE_P (t)
 	  || TREE_CODE (t) == ENUMERAL_TYPE);
 }
@@ -269,18 +275,14 @@
 
   if (TYPE_STUB_DECL (t) && !TREE_PUBLIC (TYPE_STUB_DECL (t)))
     {
-      if (DECL_ARTIFICIAL (TYPE_NAME (t)))
-	return true;
-      tree ctx = DECL_CONTEXT (TYPE_NAME (t));
-      while (ctx)
-	{
-	  if (TREE_CODE (ctx) == NAMESPACE_DECL)
-	    return !TREE_PUBLIC (ctx);
-	  if (TREE_CODE (ctx) == BLOCK)
-	    ctx = BLOCK_SUPERCONTEXT (ctx);
-	  else
-	    ctx = get_containing_scope (ctx);
-	}
+      /* C++ FE uses magic <anon> as assembler names of anonymous types.
+ 	 verify that this match with type_in_anonymous_namespace_p.  */
+#ifdef ENABLE_CHECKING
+      if (in_lto_p)
+	gcc_assert (!strcmp ("<anon>",
+		    IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (t)))));
+#endif
+      return true;
     }
   return false;
 }
@@ -292,14 +294,25 @@
 bool
 odr_type_p (const_tree t)
 {
-  if (type_with_linkage_p (t) && type_in_anonymous_namespace_p (t))
-    return true;
   /* We do not have this information when not in LTO, but we do not need
      to care, since it is used only for type merging.  */
   gcc_checking_assert (in_lto_p || flag_lto);
 
-  return (TYPE_NAME (t) && TREE_CODE (TYPE_NAME (t)) == TYPE_DECL
-          && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t))));
+  if (TYPE_NAME (t) && TREE_CODE (TYPE_NAME (t)) == TYPE_DECL
+      && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t))))
+    {
+#ifdef ENABLE_CHECKING
+      /* C++ FE uses magic <anon> as assembler names of anonymous types.
+ 	 verify that this match with type_in_anonymous_namespace_p.  */
+      gcc_assert (!type_with_linkage_p (t)
+		  || strcmp ("<anon>",
+			     IDENTIFIER_POINTER
+			        (DECL_ASSEMBLER_NAME (TYPE_NAME (t))))
+		  || type_in_anonymous_namespace_p (t));
+#endif
+      return true;
+    }
+  return false;
 }
 
 /* Return TRUE if all derived types of T are known and thus
@@ -774,7 +787,7 @@
         return false;
       /* Limit recursion: If subtypes are ODR types and we know
          that they are same, be happy.  */
-      if (!get_odr_type (t1, true)->odr_violated)
+      if (!odr_type_p (t1) || !get_odr_type (t1, true)->odr_violated)
         return true;
     }
 
Index: testsuite/g++.dg/lto/pr66180_0.C
===================================================================
--- testsuite/g++.dg/lto/pr66180_0.C	(revision 0)
+++ testsuite/g++.dg/lto/pr66180_0.C	(working copy)
@@ -0,0 +1,13 @@
+// { dg-lto-do link }
+// { dg-lto-options { { -flto -std=c++14 -r -nostdlib } } }
+#include <memory>
+namespace {
+class A {
+  int i;
+};
+}
+class G {
+  std::unique_ptr<A> foo() const;
+};
+std::unique_ptr<A> G::foo() const { return std::make_unique<A>(); }
+
Index: testsuite/g++.dg/lto/pr66180_1.C
===================================================================
--- testsuite/g++.dg/lto/pr66180_1.C	(revision 0)
+++ testsuite/g++.dg/lto/pr66180_1.C	(working copy)
@@ -0,0 +1,11 @@
+#include <memory>
+namespace {
+class A {
+  bool a;
+};
+}
+class H {
+  std::unique_ptr<A> bar() const;
+};
+std::unique_ptr<A> H::bar() const { return std::make_unique<A>(); }
+
Index: tree.c
===================================================================
--- tree.c	(revision 223629)
+++ tree.c	(working copy)
@@ -5182,8 +5182,7 @@
       && DECL_NAME (decl)
       && decl == TYPE_NAME (TREE_TYPE (decl))
       && !TYPE_ARTIFICIAL (TREE_TYPE (decl))
-      && ((type_with_linkage_p (TREE_TYPE (decl))
-	   && !type_in_anonymous_namespace_p (TREE_TYPE (decl)))
+      && (type_with_linkage_p (TREE_TYPE (decl))
 	  || TREE_CODE (TREE_TYPE (decl)) == INTEGER_TYPE)
       && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE))
     return !DECL_ASSEMBLER_NAME_SET_P (decl);

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [C frontend] Fix construction of TYPE_STUB_DECL
  2015-05-24  8:34           ` Jan Hubicka
@ 2015-05-25 19:44             ` Jason Merrill
  0 siblings, 0 replies; 15+ messages in thread
From: Jason Merrill @ 2015-05-25 19:44 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches, joseph, rth

Looks good.

Jason

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2015-05-25 19:38 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-10 17:34 [C frontend] Fix construction of TYPE_STUB_DECL Jan Hubicka
2015-05-11  7:53 ` Richard Biener
2015-05-11 10:57   ` Jan Hubicka
2015-05-11 18:40 ` Jeff Law
2015-05-11 18:53   ` Jan Hubicka
2015-05-11 21:26 ` Jason Merrill
2015-05-11 21:39   ` Jason Merrill
2015-05-11 21:59   ` Jan Hubicka
2015-05-13 16:58     ` Jan Hubicka
2015-05-21 21:12       ` Jan Hubicka
2015-05-21 21:34         ` Jason Merrill
2015-05-21 21:58           ` Jan Hubicka
2015-05-22  7:15             ` Jason Merrill
2015-05-24  8:34           ` Jan Hubicka
2015-05-25 19:44             ` Jason Merrill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).