Don't write COMDAT group id to LTO output on PE/COFF

Message ID CAEZ8vhkW8BhOR2y1VyPwctYbNAH22Zf6G2cERm6tqsvSD1AzJw@mail.gmail.com
State New
Headers show
Series
  • Don't write COMDAT group id to LTO output on PE/COFF
Related show

Commit Message

Ian Lance Taylor via Gcc-patches July 16, 2020, 1:04 p.m.
COFF targets currently do not support COMDAT groups. On MinGW targets
GCC instead puts symbols part of a COMDAT group inside of sections
annotated with the .linkonce GAS directive. This leads to GAS
generating a section so that the COMDAT name is the same as the name
of the actual symbol.

When using LTO however we never go through any of those mechanisms and
instead output the COMDAT into the LTO IR. This patch fixes this by
basically replicating the above chain by instead writing the name into
the IR file.

This case only occurs in cases where multiple inheritance is used and
non virtual thunks are created. This problem was found while trying to
compile Qt using LTO on a MinGW target. In the patch a minimal
reproducible testcase is added which fails to link without the patch
and links successfully with the patch. No regressions were observed in
the gcc and g++ testsuite after the patch has been added.

gcc/ChangeLog:

2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

    * lto-streamer-out.c (write_symol): Write symbol name instead of
COMDAT group
    on PE/COFF Targets

gcc/testsuite/ChangeLog:

2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

    * g++.dg/lto/virtual-thunk-comdat_0.C: New test.
    * g++.dg/lto/virtual-thunk-comdat.h: New test.
    * g++.dg/lto/virtual-thunk-comdat_1.C: New test.

------

Comments

Ian Lance Taylor via Gcc-patches July 17, 2020, 7:31 a.m. | #1
On Thu, Jul 16, 2020 at 3:05 PM Markus Böck via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>

> COFF targets currently do not support COMDAT groups. On MinGW targets

> GCC instead puts symbols part of a COMDAT group inside of sections

> annotated with the .linkonce GAS directive. This leads to GAS

> generating a section so that the COMDAT name is the same as the name

> of the actual symbol.

>

> When using LTO however we never go through any of those mechanisms and

> instead output the COMDAT into the LTO IR.


I think the intent is that the very same mechanism is then triggered during
LTRANS - why does that not happen?

Richard.

> This patch fixes this by

> basically replicating the above chain by instead writing the name into

> the IR file.

>

> This case only occurs in cases where multiple inheritance is used and

> non virtual thunks are created. This problem was found while trying to

> compile Qt using LTO on a MinGW target. In the patch a minimal

> reproducible testcase is added which fails to link without the patch

> and links successfully with the patch. No regressions were observed in

> the gcc and g++ testsuite after the patch has been added.

>

> gcc/ChangeLog:

>

> 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

>

>     * lto-streamer-out.c (write_symol): Write symbol name instead of

> COMDAT group

>     on PE/COFF Targets

>

> gcc/testsuite/ChangeLog:

>

> 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

>

>     * g++.dg/lto/virtual-thunk-comdat_0.C: New test.

>     * g++.dg/lto/virtual-thunk-comdat.h: New test.

>     * g++.dg/lto/virtual-thunk-comdat_1.C: New test.

>

> ------

>

> Index: gcc/lto-streamer-out.c

> ===================================================================

> --- gcc/lto-streamer-out.c (revision 932e9140d3268cf2033c1c3e93219541c53fcd29)

> +++ gcc/lto-streamer-out.c (date 1594903384922)

> @@ -2779,7 +2779,12 @@

>      size = 0;

>

>    if (DECL_ONE_ONLY (t))

> +    {

> +      if (TARGET_PECOFF)

> +    comdat = name;

> +      else

>      comdat = IDENTIFIER_POINTER (decl_comdat_group_id (t));

> +    }

>    else

>      comdat = "";

>

> Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C

> ===================================================================

> --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> @@ -0,0 +1,15 @@

> +// { dg-lto-do link }

> +#include "virtual-thunk-comdat.h"

> +

> +QAccessibleInterface::~QAccessibleInterface() {}

> +

> +QAccessibleActionInterface::~QAccessibleActionInterface() {}

> +

> +QAccessibleEditableTextInterface::~QAccessibleEditableTextInterface() {}

> +

> +bool QAccessibleObject::isValid() const

> +{

> +  return false;

> +}

> +

> +void QAccessibleLineEdit::deleteText(const char* string) {}

> Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h

> ===================================================================

> --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> @@ -0,0 +1,39 @@

> +

> +class QAccessibleInterface

> +{

> +protected:

> +  virtual ~QAccessibleInterface();

> +

> +public:

> +  virtual bool isValid() const = 0;

> +};

> +

> +class QAccessibleActionInterface

> +{

> +public:

> +  virtual ~QAccessibleActionInterface();

> +};

> +

> +class QAccessibleEditableTextInterface

> +{

> +public:

> +  virtual ~QAccessibleEditableTextInterface();

> +

> +  virtual void deleteText(const char*) = 0;

> +};

> +

> +class QAccessibleObject : public QAccessibleInterface

> +{

> +public:

> +  bool isValid() const override;

> +};

> +

> +class QAccessibleWidget : public QAccessibleObject, public

> QAccessibleActionInterface

> +{

> +};

> +

> +class QAccessibleLineEdit : public QAccessibleWidget, public

> QAccessibleEditableTextInterface

> +{

> +public:

> +  void deleteText(const char* string) override;

> +};

> Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C

> ===================================================================

> --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> @@ -0,0 +1,3 @@

> +#include "virtual-thunk-comdat.h"

> +

> +int main(int argc, char **argv) { QAccessibleLineEdit lineEdit; }
Ian Lance Taylor via Gcc-patches July 17, 2020, 11:41 a.m. | #2
The COFF linker errors with a multiple reference error before the
lto-wrapper process is started. If I understand correctly this is due
to how COMDAT works in PE/COFF as the associated string to the COMDAT
section is stored in the symbol table. When using the
--allow-multiple-definition flag as a workaround linkage succeeds.

On Fri, Jul 17, 2020 at 9:31 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On Thu, Jul 16, 2020 at 3:05 PM Markus Böck via Gcc-patches

> <gcc-patches@gcc.gnu.org> wrote:

> >

> > COFF targets currently do not support COMDAT groups. On MinGW targets

> > GCC instead puts symbols part of a COMDAT group inside of sections

> > annotated with the .linkonce GAS directive. This leads to GAS

> > generating a section so that the COMDAT name is the same as the name

> > of the actual symbol.

> >

> > When using LTO however we never go through any of those mechanisms and

> > instead output the COMDAT into the LTO IR.

>

> I think the intent is that the very same mechanism is then triggered during

> LTRANS - why does that not happen?

>

> Richard.

>

> > This patch fixes this by

> > basically replicating the above chain by instead writing the name into

> > the IR file.

> >

> > This case only occurs in cases where multiple inheritance is used and

> > non virtual thunks are created. This problem was found while trying to

> > compile Qt using LTO on a MinGW target. In the patch a minimal

> > reproducible testcase is added which fails to link without the patch

> > and links successfully with the patch. No regressions were observed in

> > the gcc and g++ testsuite after the patch has been added.

> >

> > gcc/ChangeLog:

> >

> > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> >

> >     * lto-streamer-out.c (write_symol): Write symbol name instead of

> > COMDAT group

> >     on PE/COFF Targets

> >

> > gcc/testsuite/ChangeLog:

> >

> > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> >

> >     * g++.dg/lto/virtual-thunk-comdat_0.C: New test.

> >     * g++.dg/lto/virtual-thunk-comdat.h: New test.

> >     * g++.dg/lto/virtual-thunk-comdat_1.C: New test.

> >

> > ------

> >

> > Index: gcc/lto-streamer-out.c

> > ===================================================================

> > --- gcc/lto-streamer-out.c (revision 932e9140d3268cf2033c1c3e93219541c53fcd29)

> > +++ gcc/lto-streamer-out.c (date 1594903384922)

> > @@ -2779,7 +2779,12 @@

> >      size = 0;

> >

> >    if (DECL_ONE_ONLY (t))

> > +    {

> > +      if (TARGET_PECOFF)

> > +    comdat = name;

> > +      else

> >      comdat = IDENTIFIER_POINTER (decl_comdat_group_id (t));

> > +    }

> >    else

> >      comdat = "";

> >

> > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C

> > ===================================================================

> > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > @@ -0,0 +1,15 @@

> > +// { dg-lto-do link }

> > +#include "virtual-thunk-comdat.h"

> > +

> > +QAccessibleInterface::~QAccessibleInterface() {}

> > +

> > +QAccessibleActionInterface::~QAccessibleActionInterface() {}

> > +

> > +QAccessibleEditableTextInterface::~QAccessibleEditableTextInterface() {}

> > +

> > +bool QAccessibleObject::isValid() const

> > +{

> > +  return false;

> > +}

> > +

> > +void QAccessibleLineEdit::deleteText(const char* string) {}

> > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h

> > ===================================================================

> > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > @@ -0,0 +1,39 @@

> > +

> > +class QAccessibleInterface

> > +{

> > +protected:

> > +  virtual ~QAccessibleInterface();

> > +

> > +public:

> > +  virtual bool isValid() const = 0;

> > +};

> > +

> > +class QAccessibleActionInterface

> > +{

> > +public:

> > +  virtual ~QAccessibleActionInterface();

> > +};

> > +

> > +class QAccessibleEditableTextInterface

> > +{

> > +public:

> > +  virtual ~QAccessibleEditableTextInterface();

> > +

> > +  virtual void deleteText(const char*) = 0;

> > +};

> > +

> > +class QAccessibleObject : public QAccessibleInterface

> > +{

> > +public:

> > +  bool isValid() const override;

> > +};

> > +

> > +class QAccessibleWidget : public QAccessibleObject, public

> > QAccessibleActionInterface

> > +{

> > +};

> > +

> > +class QAccessibleLineEdit : public QAccessibleWidget, public

> > QAccessibleEditableTextInterface

> > +{

> > +public:

> > +  void deleteText(const char* string) override;

> > +};

> > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C

> > ===================================================================

> > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > @@ -0,0 +1,3 @@

> > +#include "virtual-thunk-comdat.h"

> > +

> > +int main(int argc, char **argv) { QAccessibleLineEdit lineEdit; }
Jan Hubicka July 17, 2020, 12:03 p.m. | #3
> The COFF linker errors with a multiple reference error before the

> lto-wrapper process is started. If I understand correctly this is due

> to how COMDAT works in PE/COFF as the associated string to the COMDAT

> section is stored in the symbol table. When using the

> --allow-multiple-definition flag as a workaround linkage succeeds.


So the problem is caused by fact that we give linker different comdat
group names from LTO symtab then the ones from ltrans since in first
case we do ELF style comdat group that is keyed differently than the
linkonce extension?

Honza
> 

> On Fri, Jul 17, 2020 at 9:31 AM Richard Biener

> <richard.guenther@gmail.com> wrote:

> >

> > On Thu, Jul 16, 2020 at 3:05 PM Markus Böck via Gcc-patches

> > <gcc-patches@gcc.gnu.org> wrote:

> > >

> > > COFF targets currently do not support COMDAT groups. On MinGW targets

> > > GCC instead puts symbols part of a COMDAT group inside of sections

> > > annotated with the .linkonce GAS directive. This leads to GAS

> > > generating a section so that the COMDAT name is the same as the name

> > > of the actual symbol.

> > >

> > > When using LTO however we never go through any of those mechanisms and

> > > instead output the COMDAT into the LTO IR.

> >

> > I think the intent is that the very same mechanism is then triggered during

> > LTRANS - why does that not happen?

> >

> > Richard.

> >

> > > This patch fixes this by

> > > basically replicating the above chain by instead writing the name into

> > > the IR file.

> > >

> > > This case only occurs in cases where multiple inheritance is used and

> > > non virtual thunks are created. This problem was found while trying to

> > > compile Qt using LTO on a MinGW target. In the patch a minimal

> > > reproducible testcase is added which fails to link without the patch

> > > and links successfully with the patch. No regressions were observed in

> > > the gcc and g++ testsuite after the patch has been added.

> > >

> > > gcc/ChangeLog:

> > >

> > > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> > >

> > >     * lto-streamer-out.c (write_symol): Write symbol name instead of

> > > COMDAT group

> > >     on PE/COFF Targets

> > >

> > > gcc/testsuite/ChangeLog:

> > >

> > > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> > >

> > >     * g++.dg/lto/virtual-thunk-comdat_0.C: New test.

> > >     * g++.dg/lto/virtual-thunk-comdat.h: New test.

> > >     * g++.dg/lto/virtual-thunk-comdat_1.C: New test.

> > >

> > > ------

> > >

> > > Index: gcc/lto-streamer-out.c

> > > ===================================================================

> > > --- gcc/lto-streamer-out.c (revision 932e9140d3268cf2033c1c3e93219541c53fcd29)

> > > +++ gcc/lto-streamer-out.c (date 1594903384922)

> > > @@ -2779,7 +2779,12 @@

> > >      size = 0;

> > >

> > >    if (DECL_ONE_ONLY (t))

> > > +    {

> > > +      if (TARGET_PECOFF)

> > > +    comdat = name;

> > > +      else

> > >      comdat = IDENTIFIER_POINTER (decl_comdat_group_id (t));

> > > +    }

> > >    else

> > >      comdat = "";

> > >

> > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C

> > > ===================================================================

> > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > > @@ -0,0 +1,15 @@

> > > +// { dg-lto-do link }

> > > +#include "virtual-thunk-comdat.h"

> > > +

> > > +QAccessibleInterface::~QAccessibleInterface() {}

> > > +

> > > +QAccessibleActionInterface::~QAccessibleActionInterface() {}

> > > +

> > > +QAccessibleEditableTextInterface::~QAccessibleEditableTextInterface() {}

> > > +

> > > +bool QAccessibleObject::isValid() const

> > > +{

> > > +  return false;

> > > +}

> > > +

> > > +void QAccessibleLineEdit::deleteText(const char* string) {}

> > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h

> > > ===================================================================

> > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > > @@ -0,0 +1,39 @@

> > > +

> > > +class QAccessibleInterface

> > > +{

> > > +protected:

> > > +  virtual ~QAccessibleInterface();

> > > +

> > > +public:

> > > +  virtual bool isValid() const = 0;

> > > +};

> > > +

> > > +class QAccessibleActionInterface

> > > +{

> > > +public:

> > > +  virtual ~QAccessibleActionInterface();

> > > +};

> > > +

> > > +class QAccessibleEditableTextInterface

> > > +{

> > > +public:

> > > +  virtual ~QAccessibleEditableTextInterface();

> > > +

> > > +  virtual void deleteText(const char*) = 0;

> > > +};

> > > +

> > > +class QAccessibleObject : public QAccessibleInterface

> > > +{

> > > +public:

> > > +  bool isValid() const override;

> > > +};

> > > +

> > > +class QAccessibleWidget : public QAccessibleObject, public

> > > QAccessibleActionInterface

> > > +{

> > > +};

> > > +

> > > +class QAccessibleLineEdit : public QAccessibleWidget, public

> > > QAccessibleEditableTextInterface

> > > +{

> > > +public:

> > > +  void deleteText(const char* string) override;

> > > +};

> > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C

> > > ===================================================================

> > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > > @@ -0,0 +1,3 @@

> > > +#include "virtual-thunk-comdat.h"

> > > +

> > > +int main(int argc, char **argv) { QAccessibleLineEdit lineEdit; }
Ian Lance Taylor via Gcc-patches July 17, 2020, 12:15 p.m. | #4
Yes that is correct. GCC simply checks if a symbol is part of a comdat
group and if it is, emits .linkonce for it's section. GAS then sees
the directive and moves the symbol corresponding to the section name
to be the first symbol so it becomes the key. See:
https://github.com/bminor/binutils-gdb/blob/a26a62b1979400374d890811735a9c32f451ae31/bfd/coffcode.h#L3651

On Fri, Jul 17, 2020 at 2:03 PM Jan Hubicka <hubicka@ucw.cz> wrote:
>

> > The COFF linker errors with a multiple reference error before the

> > lto-wrapper process is started. If I understand correctly this is due

> > to how COMDAT works in PE/COFF as the associated string to the COMDAT

> > section is stored in the symbol table. When using the

> > --allow-multiple-definition flag as a workaround linkage succeeds.

>

> So the problem is caused by fact that we give linker different comdat

> group names from LTO symtab then the ones from ltrans since in first

> case we do ELF style comdat group that is keyed differently than the

> linkonce extension?

>

> Honza

> >

> > On Fri, Jul 17, 2020 at 9:31 AM Richard Biener

> > <richard.guenther@gmail.com> wrote:

> > >

> > > On Thu, Jul 16, 2020 at 3:05 PM Markus Böck via Gcc-patches

> > > <gcc-patches@gcc.gnu.org> wrote:

> > > >

> > > > COFF targets currently do not support COMDAT groups. On MinGW targets

> > > > GCC instead puts symbols part of a COMDAT group inside of sections

> > > > annotated with the .linkonce GAS directive. This leads to GAS

> > > > generating a section so that the COMDAT name is the same as the name

> > > > of the actual symbol.

> > > >

> > > > When using LTO however we never go through any of those mechanisms and

> > > > instead output the COMDAT into the LTO IR.

> > >

> > > I think the intent is that the very same mechanism is then triggered during

> > > LTRANS - why does that not happen?

> > >

> > > Richard.

> > >

> > > > This patch fixes this by

> > > > basically replicating the above chain by instead writing the name into

> > > > the IR file.

> > > >

> > > > This case only occurs in cases where multiple inheritance is used and

> > > > non virtual thunks are created. This problem was found while trying to

> > > > compile Qt using LTO on a MinGW target. In the patch a minimal

> > > > reproducible testcase is added which fails to link without the patch

> > > > and links successfully with the patch. No regressions were observed in

> > > > the gcc and g++ testsuite after the patch has been added.

> > > >

> > > > gcc/ChangeLog:

> > > >

> > > > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> > > >

> > > >     * lto-streamer-out.c (write_symol): Write symbol name instead of

> > > > COMDAT group

> > > >     on PE/COFF Targets

> > > >

> > > > gcc/testsuite/ChangeLog:

> > > >

> > > > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> > > >

> > > >     * g++.dg/lto/virtual-thunk-comdat_0.C: New test.

> > > >     * g++.dg/lto/virtual-thunk-comdat.h: New test.

> > > >     * g++.dg/lto/virtual-thunk-comdat_1.C: New test.

> > > >

> > > > ------

> > > >

> > > > Index: gcc/lto-streamer-out.c

> > > > ===================================================================

> > > > --- gcc/lto-streamer-out.c (revision 932e9140d3268cf2033c1c3e93219541c53fcd29)

> > > > +++ gcc/lto-streamer-out.c (date 1594903384922)

> > > > @@ -2779,7 +2779,12 @@

> > > >      size = 0;

> > > >

> > > >    if (DECL_ONE_ONLY (t))

> > > > +    {

> > > > +      if (TARGET_PECOFF)

> > > > +    comdat = name;

> > > > +      else

> > > >      comdat = IDENTIFIER_POINTER (decl_comdat_group_id (t));

> > > > +    }

> > > >    else

> > > >      comdat = "";

> > > >

> > > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C

> > > > ===================================================================

> > > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > > > @@ -0,0 +1,15 @@

> > > > +// { dg-lto-do link }

> > > > +#include "virtual-thunk-comdat.h"

> > > > +

> > > > +QAccessibleInterface::~QAccessibleInterface() {}

> > > > +

> > > > +QAccessibleActionInterface::~QAccessibleActionInterface() {}

> > > > +

> > > > +QAccessibleEditableTextInterface::~QAccessibleEditableTextInterface() {}

> > > > +

> > > > +bool QAccessibleObject::isValid() const

> > > > +{

> > > > +  return false;

> > > > +}

> > > > +

> > > > +void QAccessibleLineEdit::deleteText(const char* string) {}

> > > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h

> > > > ===================================================================

> > > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > > > @@ -0,0 +1,39 @@

> > > > +

> > > > +class QAccessibleInterface

> > > > +{

> > > > +protected:

> > > > +  virtual ~QAccessibleInterface();

> > > > +

> > > > +public:

> > > > +  virtual bool isValid() const = 0;

> > > > +};

> > > > +

> > > > +class QAccessibleActionInterface

> > > > +{

> > > > +public:

> > > > +  virtual ~QAccessibleActionInterface();

> > > > +};

> > > > +

> > > > +class QAccessibleEditableTextInterface

> > > > +{

> > > > +public:

> > > > +  virtual ~QAccessibleEditableTextInterface();

> > > > +

> > > > +  virtual void deleteText(const char*) = 0;

> > > > +};

> > > > +

> > > > +class QAccessibleObject : public QAccessibleInterface

> > > > +{

> > > > +public:

> > > > +  bool isValid() const override;

> > > > +};

> > > > +

> > > > +class QAccessibleWidget : public QAccessibleObject, public

> > > > QAccessibleActionInterface

> > > > +{

> > > > +};

> > > > +

> > > > +class QAccessibleLineEdit : public QAccessibleWidget, public

> > > > QAccessibleEditableTextInterface

> > > > +{

> > > > +public:

> > > > +  void deleteText(const char* string) override;

> > > > +};

> > > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C

> > > > ===================================================================

> > > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > > > @@ -0,0 +1,3 @@

> > > > +#include "virtual-thunk-comdat.h"

> > > > +

> > > > +int main(int argc, char **argv) { QAccessibleLineEdit lineEdit; }
Jan Hubicka July 17, 2020, 1:59 p.m. | #5
> Yes that is correct. GCC simply checks if a symbol is part of a comdat

> group and if it is, emits .linkonce for it's section. GAS then sees

> the directive and moves the symbol corresponding to the section name

> to be the first symbol so it becomes the key. See:

> https://github.com/bminor/binutils-gdb/blob/a26a62b1979400374d890811735a9c32f451ae31/bfd/coffcode.h#L3651


OK, I think in that case we should model it correctly in the symbol
table and not make GCC believe that the comdat groups has different
names.  Either in C++ frotnned or in ipa-visibility we want to check
whether we use linkonce or comdat and in first case I suppose it means
moving every symbol into its own comdat group called by its own name (as
opposed to what we do for ELF where we pack multiple ctors/dtros to
single comdat group).

This should make LTO symtab right and also tell GCC that it does not
need to prevent optimizations that break comdat group API (such as
removing one of the two constructors independently).

Honza
> 

> On Fri, Jul 17, 2020 at 2:03 PM Jan Hubicka <hubicka@ucw.cz> wrote:

> >

> > > The COFF linker errors with a multiple reference error before the

> > > lto-wrapper process is started. If I understand correctly this is due

> > > to how COMDAT works in PE/COFF as the associated string to the COMDAT

> > > section is stored in the symbol table. When using the

> > > --allow-multiple-definition flag as a workaround linkage succeeds.

> >

> > So the problem is caused by fact that we give linker different comdat

> > group names from LTO symtab then the ones from ltrans since in first

> > case we do ELF style comdat group that is keyed differently than the

> > linkonce extension?

> >

> > Honza

> > >

> > > On Fri, Jul 17, 2020 at 9:31 AM Richard Biener

> > > <richard.guenther@gmail.com> wrote:

> > > >

> > > > On Thu, Jul 16, 2020 at 3:05 PM Markus Böck via Gcc-patches

> > > > <gcc-patches@gcc.gnu.org> wrote:

> > > > >

> > > > > COFF targets currently do not support COMDAT groups. On MinGW targets

> > > > > GCC instead puts symbols part of a COMDAT group inside of sections

> > > > > annotated with the .linkonce GAS directive. This leads to GAS

> > > > > generating a section so that the COMDAT name is the same as the name

> > > > > of the actual symbol.

> > > > >

> > > > > When using LTO however we never go through any of those mechanisms and

> > > > > instead output the COMDAT into the LTO IR.

> > > >

> > > > I think the intent is that the very same mechanism is then triggered during

> > > > LTRANS - why does that not happen?

> > > >

> > > > Richard.

> > > >

> > > > > This patch fixes this by

> > > > > basically replicating the above chain by instead writing the name into

> > > > > the IR file.

> > > > >

> > > > > This case only occurs in cases where multiple inheritance is used and

> > > > > non virtual thunks are created. This problem was found while trying to

> > > > > compile Qt using LTO on a MinGW target. In the patch a minimal

> > > > > reproducible testcase is added which fails to link without the patch

> > > > > and links successfully with the patch. No regressions were observed in

> > > > > the gcc and g++ testsuite after the patch has been added.

> > > > >

> > > > > gcc/ChangeLog:

> > > > >

> > > > > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> > > > >

> > > > >     * lto-streamer-out.c (write_symol): Write symbol name instead of

> > > > > COMDAT group

> > > > >     on PE/COFF Targets

> > > > >

> > > > > gcc/testsuite/ChangeLog:

> > > > >

> > > > > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> > > > >

> > > > >     * g++.dg/lto/virtual-thunk-comdat_0.C: New test.

> > > > >     * g++.dg/lto/virtual-thunk-comdat.h: New test.

> > > > >     * g++.dg/lto/virtual-thunk-comdat_1.C: New test.

> > > > >

> > > > > ------

> > > > >

> > > > > Index: gcc/lto-streamer-out.c

> > > > > ===================================================================

> > > > > --- gcc/lto-streamer-out.c (revision 932e9140d3268cf2033c1c3e93219541c53fcd29)

> > > > > +++ gcc/lto-streamer-out.c (date 1594903384922)

> > > > > @@ -2779,7 +2779,12 @@

> > > > >      size = 0;

> > > > >

> > > > >    if (DECL_ONE_ONLY (t))

> > > > > +    {

> > > > > +      if (TARGET_PECOFF)

> > > > > +    comdat = name;

> > > > > +      else

> > > > >      comdat = IDENTIFIER_POINTER (decl_comdat_group_id (t));

> > > > > +    }

> > > > >    else

> > > > >      comdat = "";

> > > > >

> > > > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C

> > > > > ===================================================================

> > > > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > > > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > > > > @@ -0,0 +1,15 @@

> > > > > +// { dg-lto-do link }

> > > > > +#include "virtual-thunk-comdat.h"

> > > > > +

> > > > > +QAccessibleInterface::~QAccessibleInterface() {}

> > > > > +

> > > > > +QAccessibleActionInterface::~QAccessibleActionInterface() {}

> > > > > +

> > > > > +QAccessibleEditableTextInterface::~QAccessibleEditableTextInterface() {}

> > > > > +

> > > > > +bool QAccessibleObject::isValid() const

> > > > > +{

> > > > > +  return false;

> > > > > +}

> > > > > +

> > > > > +void QAccessibleLineEdit::deleteText(const char* string) {}

> > > > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h

> > > > > ===================================================================

> > > > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > > > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > > > > @@ -0,0 +1,39 @@

> > > > > +

> > > > > +class QAccessibleInterface

> > > > > +{

> > > > > +protected:

> > > > > +  virtual ~QAccessibleInterface();

> > > > > +

> > > > > +public:

> > > > > +  virtual bool isValid() const = 0;

> > > > > +};

> > > > > +

> > > > > +class QAccessibleActionInterface

> > > > > +{

> > > > > +public:

> > > > > +  virtual ~QAccessibleActionInterface();

> > > > > +};

> > > > > +

> > > > > +class QAccessibleEditableTextInterface

> > > > > +{

> > > > > +public:

> > > > > +  virtual ~QAccessibleEditableTextInterface();

> > > > > +

> > > > > +  virtual void deleteText(const char*) = 0;

> > > > > +};

> > > > > +

> > > > > +class QAccessibleObject : public QAccessibleInterface

> > > > > +{

> > > > > +public:

> > > > > +  bool isValid() const override;

> > > > > +};

> > > > > +

> > > > > +class QAccessibleWidget : public QAccessibleObject, public

> > > > > QAccessibleActionInterface

> > > > > +{

> > > > > +};

> > > > > +

> > > > > +class QAccessibleLineEdit : public QAccessibleWidget, public

> > > > > QAccessibleEditableTextInterface

> > > > > +{

> > > > > +public:

> > > > > +  void deleteText(const char* string) override;

> > > > > +};

> > > > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C

> > > > > ===================================================================

> > > > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > > > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > > > > @@ -0,0 +1,3 @@

> > > > > +#include "virtual-thunk-comdat.h"

> > > > > +

> > > > > +int main(int argc, char **argv) { QAccessibleLineEdit lineEdit; }
Ian Lance Taylor via Gcc-patches July 17, 2020, 2:05 p.m. | #6
Sounds good to me! Don't think I'd be qualified or have the know how
to make this change but the testsuite above should hopefully be of
use.

On Fri, Jul 17, 2020 at 3:59 PM Jan Hubicka <hubicka@ucw.cz> wrote:
>

> > Yes that is correct. GCC simply checks if a symbol is part of a comdat

> > group and if it is, emits .linkonce for it's section. GAS then sees

> > the directive and moves the symbol corresponding to the section name

> > to be the first symbol so it becomes the key. See:

> > https://github.com/bminor/binutils-gdb/blob/a26a62b1979400374d890811735a9c32f451ae31/bfd/coffcode.h#L3651

>

> OK, I think in that case we should model it correctly in the symbol

> table and not make GCC believe that the comdat groups has different

> names.  Either in C++ frotnned or in ipa-visibility we want to check

> whether we use linkonce or comdat and in first case I suppose it means

> moving every symbol into its own comdat group called by its own name (as

> opposed to what we do for ELF where we pack multiple ctors/dtros to

> single comdat group).

>

> This should make LTO symtab right and also tell GCC that it does not

> need to prevent optimizations that break comdat group API (such as

> removing one of the two constructors independently).

>

> Honza

> >

> > On Fri, Jul 17, 2020 at 2:03 PM Jan Hubicka <hubicka@ucw.cz> wrote:

> > >

> > > > The COFF linker errors with a multiple reference error before the

> > > > lto-wrapper process is started. If I understand correctly this is due

> > > > to how COMDAT works in PE/COFF as the associated string to the COMDAT

> > > > section is stored in the symbol table. When using the

> > > > --allow-multiple-definition flag as a workaround linkage succeeds.

> > >

> > > So the problem is caused by fact that we give linker different comdat

> > > group names from LTO symtab then the ones from ltrans since in first

> > > case we do ELF style comdat group that is keyed differently than the

> > > linkonce extension?

> > >

> > > Honza

> > > >

> > > > On Fri, Jul 17, 2020 at 9:31 AM Richard Biener

> > > > <richard.guenther@gmail.com> wrote:

> > > > >

> > > > > On Thu, Jul 16, 2020 at 3:05 PM Markus Böck via Gcc-patches

> > > > > <gcc-patches@gcc.gnu.org> wrote:

> > > > > >

> > > > > > COFF targets currently do not support COMDAT groups. On MinGW targets

> > > > > > GCC instead puts symbols part of a COMDAT group inside of sections

> > > > > > annotated with the .linkonce GAS directive. This leads to GAS

> > > > > > generating a section so that the COMDAT name is the same as the name

> > > > > > of the actual symbol.

> > > > > >

> > > > > > When using LTO however we never go through any of those mechanisms and

> > > > > > instead output the COMDAT into the LTO IR.

> > > > >

> > > > > I think the intent is that the very same mechanism is then triggered during

> > > > > LTRANS - why does that not happen?

> > > > >

> > > > > Richard.

> > > > >

> > > > > > This patch fixes this by

> > > > > > basically replicating the above chain by instead writing the name into

> > > > > > the IR file.

> > > > > >

> > > > > > This case only occurs in cases where multiple inheritance is used and

> > > > > > non virtual thunks are created. This problem was found while trying to

> > > > > > compile Qt using LTO on a MinGW target. In the patch a minimal

> > > > > > reproducible testcase is added which fails to link without the patch

> > > > > > and links successfully with the patch. No regressions were observed in

> > > > > > the gcc and g++ testsuite after the patch has been added.

> > > > > >

> > > > > > gcc/ChangeLog:

> > > > > >

> > > > > > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> > > > > >

> > > > > >     * lto-streamer-out.c (write_symol): Write symbol name instead of

> > > > > > COMDAT group

> > > > > >     on PE/COFF Targets

> > > > > >

> > > > > > gcc/testsuite/ChangeLog:

> > > > > >

> > > > > > 2020-07-16  Markus Böck  <markus.boeck02@gmail.com>

> > > > > >

> > > > > >     * g++.dg/lto/virtual-thunk-comdat_0.C: New test.

> > > > > >     * g++.dg/lto/virtual-thunk-comdat.h: New test.

> > > > > >     * g++.dg/lto/virtual-thunk-comdat_1.C: New test.

> > > > > >

> > > > > > ------

> > > > > >

> > > > > > Index: gcc/lto-streamer-out.c

> > > > > > ===================================================================

> > > > > > --- gcc/lto-streamer-out.c (revision 932e9140d3268cf2033c1c3e93219541c53fcd29)

> > > > > > +++ gcc/lto-streamer-out.c (date 1594903384922)

> > > > > > @@ -2779,7 +2779,12 @@

> > > > > >      size = 0;

> > > > > >

> > > > > >    if (DECL_ONE_ONLY (t))

> > > > > > +    {

> > > > > > +      if (TARGET_PECOFF)

> > > > > > +    comdat = name;

> > > > > > +      else

> > > > > >      comdat = IDENTIFIER_POINTER (decl_comdat_group_id (t));

> > > > > > +    }

> > > > > >    else

> > > > > >      comdat = "";

> > > > > >

> > > > > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C

> > > > > > ===================================================================

> > > > > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > > > > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)

> > > > > > @@ -0,0 +1,15 @@

> > > > > > +// { dg-lto-do link }

> > > > > > +#include "virtual-thunk-comdat.h"

> > > > > > +

> > > > > > +QAccessibleInterface::~QAccessibleInterface() {}

> > > > > > +

> > > > > > +QAccessibleActionInterface::~QAccessibleActionInterface() {}

> > > > > > +

> > > > > > +QAccessibleEditableTextInterface::~QAccessibleEditableTextInterface() {}

> > > > > > +

> > > > > > +bool QAccessibleObject::isValid() const

> > > > > > +{

> > > > > > +  return false;

> > > > > > +}

> > > > > > +

> > > > > > +void QAccessibleLineEdit::deleteText(const char* string) {}

> > > > > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h

> > > > > > ===================================================================

> > > > > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > > > > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)

> > > > > > @@ -0,0 +1,39 @@

> > > > > > +

> > > > > > +class QAccessibleInterface

> > > > > > +{

> > > > > > +protected:

> > > > > > +  virtual ~QAccessibleInterface();

> > > > > > +

> > > > > > +public:

> > > > > > +  virtual bool isValid() const = 0;

> > > > > > +};

> > > > > > +

> > > > > > +class QAccessibleActionInterface

> > > > > > +{

> > > > > > +public:

> > > > > > +  virtual ~QAccessibleActionInterface();

> > > > > > +};

> > > > > > +

> > > > > > +class QAccessibleEditableTextInterface

> > > > > > +{

> > > > > > +public:

> > > > > > +  virtual ~QAccessibleEditableTextInterface();

> > > > > > +

> > > > > > +  virtual void deleteText(const char*) = 0;

> > > > > > +};

> > > > > > +

> > > > > > +class QAccessibleObject : public QAccessibleInterface

> > > > > > +{

> > > > > > +public:

> > > > > > +  bool isValid() const override;

> > > > > > +};

> > > > > > +

> > > > > > +class QAccessibleWidget : public QAccessibleObject, public

> > > > > > QAccessibleActionInterface

> > > > > > +{

> > > > > > +};

> > > > > > +

> > > > > > +class QAccessibleLineEdit : public QAccessibleWidget, public

> > > > > > QAccessibleEditableTextInterface

> > > > > > +{

> > > > > > +public:

> > > > > > +  void deleteText(const char* string) override;

> > > > > > +};

> > > > > > Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C

> > > > > > ===================================================================

> > > > > > --- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > > > > > +++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)

> > > > > > @@ -0,0 +1,3 @@

> > > > > > +#include "virtual-thunk-comdat.h"

> > > > > > +

> > > > > > +int main(int argc, char **argv) { QAccessibleLineEdit lineEdit; }

Patch

Index: gcc/lto-streamer-out.c
===================================================================
--- gcc/lto-streamer-out.c (revision 932e9140d3268cf2033c1c3e93219541c53fcd29)
+++ gcc/lto-streamer-out.c (date 1594903384922)
@@ -2779,7 +2779,12 @@ 
     size = 0;

   if (DECL_ONE_ONLY (t))
+    {
+      if (TARGET_PECOFF)
+    comdat = name;
+      else
     comdat = IDENTIFIER_POINTER (decl_comdat_group_id (t));
+    }
   else
     comdat = "";

Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C
===================================================================
--- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)
+++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_0.C  (date 1594903164805)
@@ -0,0 +1,15 @@ 
+// { dg-lto-do link }
+#include "virtual-thunk-comdat.h"
+
+QAccessibleInterface::~QAccessibleInterface() {}
+
+QAccessibleActionInterface::~QAccessibleActionInterface() {}
+
+QAccessibleEditableTextInterface::~QAccessibleEditableTextInterface() {}
+
+bool QAccessibleObject::isValid() const
+{
+  return false;
+}
+
+void QAccessibleLineEdit::deleteText(const char* string) {}
Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h
===================================================================
--- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)
+++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat.h    (date 1594901724581)
@@ -0,0 +1,39 @@ 
+
+class QAccessibleInterface
+{
+protected:
+  virtual ~QAccessibleInterface();
+
+public:
+  virtual bool isValid() const = 0;
+};
+
+class QAccessibleActionInterface
+{
+public:
+  virtual ~QAccessibleActionInterface();
+};
+
+class QAccessibleEditableTextInterface
+{
+public:
+  virtual ~QAccessibleEditableTextInterface();
+
+  virtual void deleteText(const char*) = 0;
+};
+
+class QAccessibleObject : public QAccessibleInterface
+{
+public:
+  bool isValid() const override;
+};
+
+class QAccessibleWidget : public QAccessibleObject, public
QAccessibleActionInterface
+{
+};
+
+class QAccessibleLineEdit : public QAccessibleWidget, public
QAccessibleEditableTextInterface
+{
+public:
+  void deleteText(const char* string) override;
+};
Index: gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C
===================================================================
--- gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)
+++ gcc/testsuite/g++.dg/lto/virtual-thunk-comdat_1.C  (date 1594903161383)
@@ -0,0 +1,3 @@ 
+#include "virtual-thunk-comdat.h"
+
+int main(int argc, char **argv) { QAccessibleLineEdit lineEdit; }