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 <hubi...@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 <hubi...@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.guent...@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.boec...@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.boec...@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; }
