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

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

[Markus Böck]

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  <markus.boeck02@gmail.com>

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

gcc/testsuite/ChangeLog:

2020-03-23  Markus Böck  <markus.boeck02@gmail.com>

        * 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 00000000000..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 00000000000..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 00000000000..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();
+}

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

[Richard Biener]

On Tue, Mar 23, 2021 at 5:29 PM Markus Böck via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> 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.

Honza - can you have a look here?

Thanks,
Richard.

> gcc/ChangeLog:
>
> 2020-03-23  Markus Böck  <markus.boeck02@gmail.com>
>
>         * ipa-visibility.c (function_and_variable_visibility): Split
> COMDAT groups on targets not supporting them
>
> gcc/testsuite/ChangeLog:
>
> 2020-03-23  Markus Böck  <markus.boeck02@gmail.com>
>
>         * 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 00000000000..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 00000000000..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 00000000000..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();
> +}