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

[Richard Biener <richard.guenther@gmail.com>]

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