Re: Remove RMS from the GCC Steering Committee

2021-03-30 Thread Markus Böck via Gcc
Hello Giacomo and everyone else!

As a neighbour to your north (Austria), and another potential
newcomer, I would also like to point out that I do not believe the
views given by Nathan and others in support of him are very
US-centric. At least I would hope that most countries are in pursuit
or see value in having an inclusive environment where no one has to
feel treated unfairly due to either their gender, race or other
things. For what it's worth, Nathan may have simply picked the USA as
an example due familiarity. We don't know that.
As far as I am aware many of the people who have been participating in
this thread are also not from the USA.

I am also of the opinion that legally wrong does not equal morally
wrong. RMS does not have to have committed a crime for the developers
of GCC, the SC or whoever, to feel like he is not representing their
values as a member of the SC well.

Best regards
Markus

On Tue, Mar 30, 2021 at 3:20 PM Giacomo Tesio  wrote:
>
> Hi Nathan and hello everybody,
>
> On Fri, 26 Mar 2021 16:02:30 -0400 Nathan Sidwell wrote:
>
> > The USA is not the world and the SC is not the US government.  For
> > those in the USA, the (inapplicable) first amendment provides 5
> > rights, including showing an unwelcome guest the door. [...]
> >
> > If we fail to do so, it will continue to be harder and harder to
> > attract new talent to GCC development.
>
> I do not know if I qualify to speak here because I'm Italian and
> I ported GCC 9.2.0 to Jehanne (a Plan 9 fork, see
> http://jehanne.io/2021/01/06/gcc_on_jehanne.html), but due to the
> pandemic I wasn't able to align it with the new developments and
> contribute the port upstream. Also, I have no idea if you would be
> interested in running GCC on a Plan 9 fork and thus accept my
> contribution.
>
>
> Yet, after a careful read of this thread I realized that I might
> be considered the kind of "new talent" Nathan is talking about.
>
> So here is my perspective on this topic, "in the hope it helps but
> without any warranty". :-D
>
>
> I do not share many of Stallman's opinions (we are VERY different), but
> when I write free software and contribute to a free software community,
> what I want is long term assurances about one and only one topic: that
> the software will stay free as in freedom, as a common good for the
> whole humanity.
>
> As of today, GPLv3 is the legal tool that best suit this goal.
> I don't think it's perfect in this regards, but that's another story.
>
>
> As an Italian I'm having a hard time trying to follow your reasoning
> about Stallman being a problem to attract new talents.
>
> I could understand such statement if he had committed actual crimes,
> was legally persecuted, processed and condemned like Reiser.
>
> But while I try, I cannot really understand why you think that his name
> in the Steering Committee would drive away people from contributing GCC
>
>
> I ported GCC to Plan 9 because I want a free compiler suite for my OS.
>
> Porting CLANG would have been easier (to some extent) BUT my choice was
> political and Stallman in the Steering Committee is a long term
> warranty that GCC development will not steer away from the Free
> Software conception that I know, betraying my trust.
>
>
> My impression is that you are, in absolute good faith, projecting your
> own culture (quite US-centric, as far as I can deduce by this thread)
> to the whole world.
>
>
> I do not really know if the removing Stallman from the Steering
> Committee would attract more US people in GCC development. Or if it
> would attract more US people that now prefer to work in LLVM only
> because of they feel somehow bad working with Stallman in the SC.
>
>
> But I can assure you that, as Pankaj Jangid said before me, many many
> people are attracted to GCC, as users and developers, BECAUSE of
> Stallman presence, because they know that something like this
> https://medium.com/@giacomo_59737/what-i-wish-i-knew-before-contributing-to-open-source-dd63acd20696
> will not happen to them.
>
>
> World wide, people do not LIKE Stallman, but we TRUST him on this.
> Just like the GPLv3, RMS is not perfect, but it does ONE THING well.
>
>
> So, since you care about demographics, please consider that.
>
> Removing RMS you might attract more of certain US demographics,
> but you will certainly alienate a lot of people world wide that
> do not align your political values (despite respecting them a lot!)
> and do not think that a compiler suite can fix US systemic issues
> anyway.
>
>
> As for me, I would NOT trust GCC (or FSF) in the long term, had
> you to distance Stallman, because I've already seen with my eyes
> what happen when people do not have anything to loose to betray your
> trust, and Stallman has all to lose by betraying Free Software.
>
>
> Maybe I'm not the "new talent" you are looking for.
>
> But please, do not turn GCC into a US-centric project.
>
>
>
> Giacomo


[PATCH][PR94156] Split COMDAT groups on target that do not support them

2021-03-23 Thread Markus Böck via Gcc-patches
GCC at the moment uses COMDAT groups for things like virtual thunks,
even on targets that do not support COMDAT groups. This has not been a
problem as on platforms not supporting these (such as PE COFF on
Windows), the backend handled it through directives to GAS. GCC would
simply use a .linkonce directive telling the assembler that this
symbol may occur multiple times, and GAS would translate that into a
"select any" COMDAT, containing only the symbol itself (as Windows
does support COMDAT, just not groups).

When using LTO on Windows however, a few problems occur: The COMDAT
group is transmitted as part of the IR and the linker (ld) will try to
resolve symbols. On Windows the COMDAT information is put into the
symbol table, instead of in sections, leading to the linker to error
out with a multiple reference error before even calling the
lto-wrapper and LTRANS on the IR, which would otherwise resolve the
use of COMDAT groups.

This patch removes comdat groups for symbols in the ipa-visibility
pass and instead puts them into their own comdat. An exception to this
rule are aliases which (at least on Windows) are also allowed to be in
the same comdat group as the symbol they are referencing.

This fixes PR94156: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94156
A previous discussion on the problems this patch attempts to fix were
had here: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/550148.html

I tested this patch with a x86_64-w64-mingw32 target on a Linux host.
No regressions between before and after of this patch were noted. The
Test cases provided with this patch have been confirmed to reproduce
before this patch, and link and work after the application of the
patch.

Feedback is very welcome, especially on implications I might not be aware of.

gcc/ChangeLog:

2020-03-23  Markus Böck  

* ipa-visibility.c (function_and_variable_visibility): Split
COMDAT groups on targets not supporting them

gcc/testsuite/ChangeLog:

2020-03-23  Markus Böck  

* g++.dg/lto/pr94156.h: New test.
* g++.dg/lto/pr94156_0.C: New test.
* g++.dg/lto/pr94156_1.C: New test.

--
diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
index eb0ebf770e3..76f1a8ff72a 100644
--- a/gcc/ipa-visibility.c
+++ b/gcc/ipa-visibility.c
@@ -709,6 +709,14 @@ function_and_variable_visibility (bool whole_program)
  }
node->dissolve_same_comdat_group_list ();
  }
+
+  if (!HAVE_COMDAT_GROUP && node->same_comdat_group
+  && !node->alias && !node->has_aliases_p())
+{
+  node->remove_from_same_comdat_group();
+  node->set_comdat_group(DECL_ASSEMBLER_NAME_RAW(node->decl));
+}
+
   gcc_assert ((!DECL_WEAK (node->decl)
&& !DECL_COMDAT (node->decl))
  || TREE_PUBLIC (node->decl)
@@ -742,8 +750,11 @@ function_and_variable_visibility (bool whole_program)
  {
gcc_checking_assert (DECL_COMDAT (node->decl)
 == DECL_COMDAT (decl_node->decl));
-   gcc_checking_assert (node->in_same_comdat_group_p (decl_node));
-   gcc_checking_assert (node->same_comdat_group);
+   if (HAVE_COMDAT_GROUP)
+{
+  gcc_checking_assert (node->in_same_comdat_group_p
(decl_node));
+  gcc_checking_assert (node->same_comdat_group);
+}
  }
node->forced_by_abi = decl_node->forced_by_abi;
if (DECL_EXTERNAL (decl_node->decl))
diff --git a/gcc/testsuite/g++.dg/lto/pr94156.h
b/gcc/testsuite/g++.dg/lto/pr94156.h
new file mode 100644
index 000..3990ac46fcb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr94156.h
@@ -0,0 +1,20 @@
+class Base0 {
+public:
+  virtual ~Base0() {}
+};
+
+class Base1 {
+public:
+  virtual void foo() = 0;
+};
+
+class Base2 {
+public:
+  virtual void foo() = 0;
+};
+
+class Derived : public Base0, public Base1, public Base2 {
+public:
+  virtual ~Derived();
+  virtual void foo() override {}
+};
diff --git a/gcc/testsuite/g++.dg/lto/pr94156_0.C
b/gcc/testsuite/g++.dg/lto/pr94156_0.C
new file mode 100644
index 000..1a2e30badc7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr94156_0.C
@@ -0,0 +1,6 @@
+// { dg-lto-do link }
+#include "pr94156.h"
+
+Derived::~Derived() {}
+
+int main() {}
diff --git a/gcc/testsuite/g++.dg/lto/pr94156_1.C
b/gcc/testsuite/g++.dg/lto/pr94156_1.C
new file mode 100644
index 000..d7a40efa96c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr94156_1.C
@@ -0,0 +1,6 @@
+#include "pr94156.h"
+
+void bar(Derived* p)
+{
+  p->foo();
+}


[PATCH] [PR96608] analyzer: Change cast from long to intptr_t

2020-09-30 Thread Markus Böck via Gcc-patches
Casting to intptr_t states the intent of an integer to pointer cast
more clearly and ensures that the cast causes no loss of precision on
any platforms. LLP64 platforms eg. have a long value of 4 bytes and
pointer values of 8 bytes which may even cause compiler errors.

Fixes PR 96608

Would need this to be committed for me if accepted. (username
zero9178, email markus.boec...@gmail.com)

Markus

gcc/analyzer/ChangeLog:
PR analyzer/96608

* store.h (hash): Cast to intptr_t instead of long

---
 gcc/analyzer/store.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h
index 0f4e7ab2a56..9589c566e1b 100644
--- a/gcc/analyzer/store.h
+++ b/gcc/analyzer/store.h
@@ -269,7 +269,7 @@ public:

   hashval_t hash () const
   {
-return (binding_key::impl_hash () ^ (long)m_region);
+return (binding_key::impl_hash () ^ (intptr_t)m_region);
   }
   bool operator== (const symbolic_binding ) const
   {
-- 
2.17.1


Re: [PATCH] Don't write COMDAT group id to LTO output on PE/COFF

2020-07-17 Thread Markus Böck via Gcc-patches
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  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  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
> > > >  wrote:
> > > > >
> > > > > On Thu, Jul 16, 2020 at 3:05 PM Markus Böck via Gcc-patches
> > > > >  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  
> > > > > >
> > > > > > * 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  
> > > > > >
> > > 

Re: [PATCH] Don't write COMDAT group id to LTO output on PE/COFF

2020-07-17 Thread Markus Böck via Gcc-patches
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  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
> >  wrote:
> > >
> > > On Thu, Jul 16, 2020 at 3:05 PM Markus Böck via Gcc-patches
> > >  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  
> > > >
> > > > * 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  
> > > >
> > > > * 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() {}
> > >

Re: [PATCH] Don't write COMDAT group id to LTO output on PE/COFF

2020-07-17 Thread Markus Böck via Gcc-patches
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
 wrote:
>
> On Thu, Jul 16, 2020 at 3:05 PM Markus Böck via Gcc-patches
>  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  
> >
> > * 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  
> >
> > * 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] Don't write COMDAT group id to LTO output on PE/COFF

2020-07-16 Thread Markus Böck via Gcc-patches
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  

* 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  

* 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; }