[LTO,RFA] PR c++/88049 - ICE with undefined destructor and anon namespace.

A type in an anonymous namespace can never be merged with one from
another translation unit, so a member of such a type is always its own
prevailing decl.

I don't really understand the LTO concept of prevailing decl, or why we don't
get here if the destructor is defined, but this seems reasonable and fixes the
bug.

Tested x86_64-pc-linux-gnu.  OK for trunk?

	* lto-symtab.c (lto_symtab_prevailing_virtual_decl): Return early
	for a type in an anonymous namespace.
---
 gcc/lto/lto-symtab.c                 |  8 ++++++--
 gcc/testsuite/g++.dg/lto/pr88049_0.C | 16 ++++++++++++++++
 gcc/lto/ChangeLog                    |  6 ++++++
 3 files changed, 28 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/lto/pr88049_0.C


base-commit: 79ae32275d4a19a1fc6ffebec9ac15a8c94b0b8f

On Wed, Feb 20, 2019 at 2:58 AM Jason Merrill <jason@redhat.com> wrote:
>
> A type in an anonymous namespace can never be merged with one from
> another translation unit, so a member of such a type is always its own
> prevailing decl.
>
> I don't really understand the LTO concept of prevailing decl, or why we don't
> get here if the destructor is defined, but this seems reasonable and fixes the
> bug.

We're probably not supposed to end up here but get the "precomputed"
prevailing decl from symtab merging.  Honza should know more precisely.
The resolution file does have an entry for the UNDEF destructor.

Does this make the testcase invalid btw?  There's no way another TU
can inject a definition into the same anonymous namespace, no?
And does the point of instantiation really determine the containing
namespace of the class?

Richard.

>
> Tested x86_64-pc-linux-gnu.  OK for trunk?
>
>         * lto-symtab.c (lto_symtab_prevailing_virtual_decl): Return early
>         for a type in an anonymous namespace.
> ---
>  gcc/lto/lto-symtab.c                 |  8 ++++++--
>  gcc/testsuite/g++.dg/lto/pr88049_0.C | 16 ++++++++++++++++
>  gcc/lto/ChangeLog                    |  6 ++++++
>  3 files changed, 28 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr88049_0.C
>
> diff --git a/gcc/lto/lto-symtab.c b/gcc/lto/lto-symtab.c
> index 22da4c78b8c..343915c3cec 100644
> --- a/gcc/lto/lto-symtab.c
> +++ b/gcc/lto/lto-symtab.c
> @@ -1085,8 +1085,12 @@ lto_symtab_prevailing_virtual_decl (tree decl)
>  {
>    if (DECL_ABSTRACT_P (decl))
>      return decl;
> -  gcc_checking_assert (!type_in_anonymous_namespace_p (DECL_CONTEXT (decl))
> -                      && DECL_ASSEMBLER_NAME_SET_P (decl));
> +
> +  if (type_in_anonymous_namespace_p (DECL_CONTEXT (decl)))
> +    /* There can't be any other declarations.  */
> +    return decl;
> +
> +  gcc_checking_assert (DECL_ASSEMBLER_NAME_SET_P (decl));
>
>    symtab_node *n = symtab_node::get_for_asmname
>                      (DECL_ASSEMBLER_NAME (decl));
> diff --git a/gcc/testsuite/g++.dg/lto/pr88049_0.C b/gcc/testsuite/g++.dg/lto/pr88049_0.C
> new file mode 100644
> index 00000000000..7ac3618c2c8
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr88049_0.C
> @@ -0,0 +1,16 @@
> +// PR c++/88049
> +// { dg-lto-do link }
> +// { dg-lto-options {{ -flto -O2 -w }} }
> +// { dg-extra-ld-options -r }
> +
> +template <typename> class a;
> +class b {};
> +template <typename e> a<e> d(char);
> +template <typename> class a : public b {
> +public:
> +  virtual ~a();
> +};
> +namespace {
> +  class f;
> +  b c = d<f>(int());
> +} // namespace
> diff --git a/gcc/lto/ChangeLog b/gcc/lto/ChangeLog
> index 6b183df3b0f..71a2a109e64 100644
> --- a/gcc/lto/ChangeLog
> +++ b/gcc/lto/ChangeLog
> @@ -1,3 +1,9 @@
> +2019-02-18  Jason Merrill  <jason@redhat.com>
> +
> +       PR c++/88049 - ICE with undefined destructor and anon namespace.
> +       * lto-symtab.c (lto_symtab_prevailing_virtual_decl): Return early
> +       for a type in an anonymous namespace.
> +
>  2019-01-09  Sandra Loosemore  <sandra@codesourcery.com>
>
>         PR other/16615
>
> base-commit: 79ae32275d4a19a1fc6ffebec9ac15a8c94b0b8f
> --
> 2.20.1
>

On Tue, Feb 19, 2019 at 10:53 PM Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Feb 20, 2019 at 2:58 AM Jason Merrill <jason@redhat.com> wrote:
> >
> > A type in an anonymous namespace can never be merged with one from
> > another translation unit, so a member of such a type is always its own
> > prevailing decl.
> >
> > I don't really understand the LTO concept of prevailing decl, or why we don't
> > get here if the destructor is defined, but this seems reasonable and fixes the
> > bug.
>
> We're probably not supposed to end up here but get the "precomputed"
> prevailing decl from symtab merging.  Honza should know more precisely.
> The resolution file does have an entry for the UNDEF destructor.
>
> Does this make the testcase invalid btw?  There's no way another TU
> can inject a definition into the same anonymous namespace, no?

Correct.  The destructor is needed to destroy 'c', and there's no way
it could be defined in another TU, so the testcase will fail to link.
So we could probably reject the testcase (in cgraph?) before we touch
LTO at all.

> And does the point of instantiation really determine the containing
> namespace of the class?

No.  a<f> isn't really in an anonymous namespace, but since f is, a<f>
also gets internal linkage.  So type_in_anonymous_namespace_p is a
somewhat misleading name; perhaps type_internal_p?  type_nonpublic_p?

Jason

On Wed, 20 Feb 2019 at 02:58, Jason Merrill <jason@redhat.com> wrote:
>
> A type in an anonymous namespace can never be merged with one from
> another translation unit, so a member of such a type is always its own
> prevailing decl.
>
> I don't really understand the LTO concept of prevailing decl, or why we don't
> get here if the destructor is defined, but this seems reasonable and fixes the
> bug.
>
> Tested x86_64-pc-linux-gnu.  OK for trunk?
>
>         * lto-symtab.c (lto_symtab_prevailing_virtual_decl): Return early
>         for a type in an anonymous namespace.
> ---
>  gcc/lto/lto-symtab.c                 |  8 ++++++--
>  gcc/testsuite/g++.dg/lto/pr88049_0.C | 16 ++++++++++++++++
>  gcc/lto/ChangeLog                    |  6 ++++++
>  3 files changed, 28 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr88049_0.C
>
> diff --git a/gcc/lto/lto-symtab.c b/gcc/lto/lto-symtab.c
> index 22da4c78b8c..343915c3cec 100644
> --- a/gcc/lto/lto-symtab.c
> +++ b/gcc/lto/lto-symtab.c
> @@ -1085,8 +1085,12 @@ lto_symtab_prevailing_virtual_decl (tree decl)
>  {
>    if (DECL_ABSTRACT_P (decl))
>      return decl;
> -  gcc_checking_assert (!type_in_anonymous_namespace_p (DECL_CONTEXT (decl))
> -                      && DECL_ASSEMBLER_NAME_SET_P (decl));
> +
> +  if (type_in_anonymous_namespace_p (DECL_CONTEXT (decl)))
> +    /* There can't be any other declarations.  */
> +    return decl;
> +
> +  gcc_checking_assert (DECL_ASSEMBLER_NAME_SET_P (decl));
>
>    symtab_node *n = symtab_node::get_for_asmname
>                      (DECL_ASSEMBLER_NAME (decl));
> diff --git a/gcc/testsuite/g++.dg/lto/pr88049_0.C b/gcc/testsuite/g++.dg/lto/pr88049_0.C
> new file mode 100644
> index 00000000000..7ac3618c2c8
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr88049_0.C
> @@ -0,0 +1,16 @@
> +// PR c++/88049
> +// { dg-lto-do link }
> +// { dg-lto-options {{ -flto -O2 -w }} }
> +// { dg-extra-ld-options -r }
> +
> +template <typename> class a;
> +class b {};
> +template <typename e> a<e> d(char);
> +template <typename> class a : public b {
> +public:
> +  virtual ~a();
> +};
> +namespace {
> +  class f;
> +  b c = d<f>(int());
> +} // namespace


Hi Jason,

On bare-metal targets (arm, aarch64 using newlib), I'm using dejagnu's
testglue, which makes this new test fail because it also uses
g++_tg.o, leading to:
/arm-none-eabi/bin/ld: warning: incremental linking of LTO and non-LTO
objects; using -flinker-output=nolto-rel which will bypass whole
program optimization

Is there a way to avoid that (besides not using testglue) ?

Thanks,

Christophe

> diff --git a/gcc/lto/ChangeLog b/gcc/lto/ChangeLog
> index 6b183df3b0f..71a2a109e64 100644
> --- a/gcc/lto/ChangeLog
> +++ b/gcc/lto/ChangeLog
> @@ -1,3 +1,9 @@
> +2019-02-18  Jason Merrill  <jason@redhat.com>
> +
> +       PR c++/88049 - ICE with undefined destructor and anon namespace.
> +       * lto-symtab.c (lto_symtab_prevailing_virtual_decl): Return early
> +       for a type in an anonymous namespace.
> +
>  2019-01-09  Sandra Loosemore  <sandra@codesourcery.com>
>
>         PR other/16615
>
> base-commit: 79ae32275d4a19a1fc6ffebec9ac15a8c94b0b8f
> --
> 2.20.1
>

On Mon, Mar 4, 2019 at 8:41 AM Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> On Wed, 20 Feb 2019 at 02:58, Jason Merrill <jason@redhat.com> wrote:
> >
> > A type in an anonymous namespace can never be merged with one from
> > another translation unit, so a member of such a type is always its own
> > prevailing decl.
> >
> > I don't really understand the LTO concept of prevailing decl, or why we don't
> > get here if the destructor is defined, but this seems reasonable and fixes the
> > bug.
> >
> > Tested x86_64-pc-linux-gnu.  OK for trunk?
> >
> >         * lto-symtab.c (lto_symtab_prevailing_virtual_decl): Return early
> >         for a type in an anonymous namespace.
> > ---
> >  gcc/lto/lto-symtab.c                 |  8 ++++++--
> >  gcc/testsuite/g++.dg/lto/pr88049_0.C | 16 ++++++++++++++++
> >  gcc/lto/ChangeLog                    |  6 ++++++
> >  3 files changed, 28 insertions(+), 2 deletions(-)
> >  create mode 100644 gcc/testsuite/g++.dg/lto/pr88049_0.C
> >
> > diff --git a/gcc/lto/lto-symtab.c b/gcc/lto/lto-symtab.c
> > index 22da4c78b8c..343915c3cec 100644
> > --- a/gcc/lto/lto-symtab.c
> > +++ b/gcc/lto/lto-symtab.c
> > @@ -1085,8 +1085,12 @@ lto_symtab_prevailing_virtual_decl (tree decl)
> >  {
> >    if (DECL_ABSTRACT_P (decl))
> >      return decl;
> > -  gcc_checking_assert (!type_in_anonymous_namespace_p (DECL_CONTEXT (decl))
> > -                      && DECL_ASSEMBLER_NAME_SET_P (decl));
> > +
> > +  if (type_in_anonymous_namespace_p (DECL_CONTEXT (decl)))
> > +    /* There can't be any other declarations.  */
> > +    return decl;
> > +
> > +  gcc_checking_assert (DECL_ASSEMBLER_NAME_SET_P (decl));
> >
> >    symtab_node *n = symtab_node::get_for_asmname
> >                      (DECL_ASSEMBLER_NAME (decl));
> > diff --git a/gcc/testsuite/g++.dg/lto/pr88049_0.C b/gcc/testsuite/g++.dg/lto/pr88049_0.C
> > new file mode 100644
> > index 00000000000..7ac3618c2c8
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/lto/pr88049_0.C
> > @@ -0,0 +1,16 @@
> > +// PR c++/88049
> > +// { dg-lto-do link }
> > +// { dg-lto-options {{ -flto -O2 -w }} }
> > +// { dg-extra-ld-options -r }
> > +
> > +template <typename> class a;
> > +class b {};
> > +template <typename e> a<e> d(char);
> > +template <typename> class a : public b {
> > +public:
> > +  virtual ~a();
> > +};
> > +namespace {
> > +  class f;
> > +  b c = d<f>(int());
> > +} // namespace
>
>
> Hi Jason,
>
> On bare-metal targets (arm, aarch64 using newlib), I'm using dejagnu's
> testglue, which makes this new test fail because it also uses
> g++_tg.o, leading to:
> /arm-none-eabi/bin/ld: warning: incremental linking of LTO and non-LTO
> objects; using -flinker-output=nolto-rel which will bypass whole
> program optimization
>
> Is there a way to avoid that (besides not using testglue) ?

Does adding

// { dg-require-effective-target lto_incremental }

to the testcase help?

Jason

On Mon, 4 Mar 2019 at 17:37, Jason Merrill <jason@redhat.com> wrote:
>
> On Mon, Mar 4, 2019 at 8:41 AM Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
> > On Wed, 20 Feb 2019 at 02:58, Jason Merrill <jason@redhat.com> wrote:
> > >
> > > A type in an anonymous namespace can never be merged with one from
> > > another translation unit, so a member of such a type is always its own
> > > prevailing decl.
> > >
> > > I don't really understand the LTO concept of prevailing decl, or why we don't
> > > get here if the destructor is defined, but this seems reasonable and fixes the
> > > bug.
> > >
> > > Tested x86_64-pc-linux-gnu.  OK for trunk?
> > >
> > >         * lto-symtab.c (lto_symtab_prevailing_virtual_decl): Return early
> > >         for a type in an anonymous namespace.
> > > ---
> > >  gcc/lto/lto-symtab.c                 |  8 ++++++--
> > >  gcc/testsuite/g++.dg/lto/pr88049_0.C | 16 ++++++++++++++++
> > >  gcc/lto/ChangeLog                    |  6 ++++++
> > >  3 files changed, 28 insertions(+), 2 deletions(-)
> > >  create mode 100644 gcc/testsuite/g++.dg/lto/pr88049_0.C
> > >
> > > diff --git a/gcc/lto/lto-symtab.c b/gcc/lto/lto-symtab.c
> > > index 22da4c78b8c..343915c3cec 100644
> > > --- a/gcc/lto/lto-symtab.c
> > > +++ b/gcc/lto/lto-symtab.c
> > > @@ -1085,8 +1085,12 @@ lto_symtab_prevailing_virtual_decl (tree decl)
> > >  {
> > >    if (DECL_ABSTRACT_P (decl))
> > >      return decl;
> > > -  gcc_checking_assert (!type_in_anonymous_namespace_p (DECL_CONTEXT (decl))
> > > -                      && DECL_ASSEMBLER_NAME_SET_P (decl));
> > > +
> > > +  if (type_in_anonymous_namespace_p (DECL_CONTEXT (decl)))
> > > +    /* There can't be any other declarations.  */
> > > +    return decl;
> > > +
> > > +  gcc_checking_assert (DECL_ASSEMBLER_NAME_SET_P (decl));
> > >
> > >    symtab_node *n = symtab_node::get_for_asmname
> > >                      (DECL_ASSEMBLER_NAME (decl));
> > > diff --git a/gcc/testsuite/g++.dg/lto/pr88049_0.C b/gcc/testsuite/g++.dg/lto/pr88049_0.C
> > > new file mode 100644
> > > index 00000000000..7ac3618c2c8
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/lto/pr88049_0.C
> > > @@ -0,0 +1,16 @@
> > > +// PR c++/88049
> > > +// { dg-lto-do link }
> > > +// { dg-lto-options {{ -flto -O2 -w }} }
> > > +// { dg-extra-ld-options -r }
> > > +
> > > +template <typename> class a;
> > > +class b {};
> > > +template <typename e> a<e> d(char);
> > > +template <typename> class a : public b {
> > > +public:
> > > +  virtual ~a();
> > > +};
> > > +namespace {
> > > +  class f;
> > > +  b c = d<f>(int());
> > > +} // namespace
> >
> >
> > Hi Jason,
> >
> > On bare-metal targets (arm, aarch64 using newlib), I'm using dejagnu's
> > testglue, which makes this new test fail because it also uses
> > g++_tg.o, leading to:
> > /arm-none-eabi/bin/ld: warning: incremental linking of LTO and non-LTO
> > objects; using -flinker-output=nolto-rel which will bypass whole
> > program optimization
> >
> > Is there a way to avoid that (besides not using testglue) ?
>
> Does adding
>
> // { dg-require-effective-target lto_incremental }
>
> to the testcase help?
>

Yes, it does, thanks. I was unaware if this effective-target.

> Jason