Re: [PATCH v2] PR105169 Fix references to discarded sections

[Richard Biener <rguenther@suse.de>]

> Am 17.05.2022 um 19:37 schrieb Giuliano Belinassi via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> On Mon, 2022-05-09 at 13:39 +0200, Richard Biener wrote:
>>> On Sat, 7 May 2022, Giuliano Belinassi wrote:
>>> 
>>> When -fpatchable-function-entry= is enabled, certain C++ codes
>>> fails to
>>> link because of generated references to discarded sections in
>>> __patchable_function_entry section. This commit fixes this problem
>>> by
>>> puting those references in a COMDAT section.
>>> 
>>> On the previous patch, GCC fails to compile with --enable-vtable-
>>> verify
>>> enabled. This version compiles fine with it.
>> 
>> OK for trunk.
>> 
> 
> Pushed to trunk. Is it okay if I backport it to older versions as well?

Ok for the GCC 12 branch after a week with no reported issues. Since it’s not a regression further backporting requires good reasons.

Richard.

> Giuliano.
> 
>> Thanks,
>> Richard.
>> 
>>> 2022-05-06  Giuliano Belinassi  <gbelinassi@suse.de>
>>> 
>>>    PR c++/105169
>>>    * targhooks.cc (default_print_patchable_function_entry_1):
>>> Handle COMDAT case.
>>>    * varasm.cc (switch_to_comdat_section): New
>>>    (handle_vtv_comdat_section): Call switch_to_comdat_section.
>>>    * varasm.h: Declare switch_to_comdat_section.
>>> 
>>> gcc/testsuite/ChangeLog
>>> 2022-05-06  Giuliano Belinassi  <gbelinassi@suse.de>
>>> 
>>>    PR c++/105169
>>>    * g++.dg/modules/pr105169.h: New file.
>>>    * g++.dg/modules/pr105169_a.C: New test.
>>>    * g++.dg/modules/pr105169_b.C: New file.
>>> ---
>>> gcc/targhooks.cc                          |  8 ++++--
>>> gcc/testsuite/g++.dg/modules/pr105169.h   | 22 +++++++++++++++
>>> gcc/testsuite/g++.dg/modules/pr105169_a.C | 25 +++++++++++++++++
>>> gcc/testsuite/g++.dg/modules/pr105169_b.C | 12 +++++++++
>>> gcc/varasm.cc                             | 33 ++++++++++++++-----
>>> ----
>>> gcc/varasm.h                              |  2 ++
>>> 6 files changed, 87 insertions(+), 15 deletions(-)
>>> create mode 100644 gcc/testsuite/g++.dg/modules/pr105169.h
>>> create mode 100644 gcc/testsuite/g++.dg/modules/pr105169_a.C
>>> create mode 100644 gcc/testsuite/g++.dg/modules/pr105169_b.C
>>> 
>>> diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
>>> index 399d6f874dc..b15ae19bcb6 100644
>>> --- a/gcc/targhooks.cc
>>> +++ b/gcc/targhooks.cc
>>> @@ -2009,8 +2009,12 @@ default_print_patchable_function_entry_1
>>> (FILE *file,
>>>       patch_area_number++;
>>>       ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE",
>>> patch_area_number);
>>> 
>>> -      switch_to_section (get_section
>>> ("__patchable_function_entries",
>>> -                      flags, current_function_decl));
>>> +      section *sect = get_section ("__patchable_function_entries",
>>> +                  flags, current_function_decl);
>>> +      if (HAVE_COMDAT_GROUP && DECL_COMDAT_GROUP
>>> (current_function_decl))
>>> +    switch_to_comdat_section (sect, current_function_decl);
>>> +      else
>>> +    switch_to_section (sect);
>>>       assemble_align (POINTER_SIZE);
>>>       fputs (asm_op, file);
>>>       assemble_name_raw (file, buf);
>>> diff --git a/gcc/testsuite/g++.dg/modules/pr105169.h
>>> b/gcc/testsuite/g++.dg/modules/pr105169.h
>>> new file mode 100644
>>> index 00000000000..a7e76270531
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/modules/pr105169.h
>>> @@ -0,0 +1,22 @@
>>> +class IPXAddressClass
>>> +{
>>> +public:
>>> +    IPXAddressClass(void);
>>> +};
>>> +
>>> +class WinsockInterfaceClass
>>> +{
>>> +
>>> +public:
>>> +    WinsockInterfaceClass(void);
>>> +
>>> +    virtual void Set_Broadcast_Address(void*){};
>>> +
>>> +    virtual int Get_Protocol(void)
>>> +    {
>>> +        return 0;
>>> +    };
>>> +
>>> +protected:
>>> +};
>>> +
>>> diff --git a/gcc/testsuite/g++.dg/modules/pr105169_a.C
>>> b/gcc/testsuite/g++.dg/modules/pr105169_a.C
>>> new file mode 100644
>>> index 00000000000..66dc4b7901f
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/modules/pr105169_a.C
>>> @@ -0,0 +1,25 @@
>>> +/* { dg-module-do link } */
>>> +/* { dg-options "-std=c++11 -fpatchable-function-entry=1 -O2" } */
>>> +/* { dg-additional-options "-std=c++11 -fpatchable-function-
>>> entry=1 -O2" } */
>>> +
>>> +/* This test is in the "modules" package because it supports
>>> multiple files
>>> +   linkage.  */
>>> +
>>> +#include "pr105169.h"
>>> +
>>> +WinsockInterfaceClass* PacketTransport;
>>> +
>>> +IPXAddressClass::IPXAddressClass(void)
>>> +{
>>> +}
>>> +
>>> +int function()
>>> +{
>>> +  return PacketTransport->Get_Protocol();
>>> +}
>>> +
>>> +int main()
>>> +{
>>> +  IPXAddressClass ipxaddr;
>>> +  return 0;
>>> +}
>>> diff --git a/gcc/testsuite/g++.dg/modules/pr105169_b.C
>>> b/gcc/testsuite/g++.dg/modules/pr105169_b.C
>>> new file mode 100644
>>> index 00000000000..5f8b00dfe51
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/modules/pr105169_b.C
>>> @@ -0,0 +1,12 @@
>>> +/* { dg-module-do link } */
>>> +/* { dg-options "-std=c++11 -fpatchable-function-entry=1 -O2" } */
>>> +/* { dg-additional-options "-std=c++11 -fpatchable-function-
>>> entry=1 -O2" } */
>>> +
>>> +/* This test is in the "modules" package because it supports
>>> multiple files
>>> +   linkage.  */
>>> +
>>> +#include "pr105169.h"
>>> +
>>> +WinsockInterfaceClass::WinsockInterfaceClass(void)
>>> +{
>>> +}
>>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc
>>> index c41f17d64f7..6454f1ce519 100644
>>> --- a/gcc/varasm.cc
>>> +++ b/gcc/varasm.cc
>>> @@ -8457,25 +8457,21 @@ default_asm_output_ident_directive (const
>>> char *ident_str)
>>>     fprintf (asm_out_file, "%s\"%s\"\n", ident_asm_op, ident_str);
>>> }
>>> 
>>> -
>>> -/* This function ensures that vtable_map variables are not only
>>> -   in the comdat section, but that each variable has its own
>>> unique
>>> -   comdat name.  Without this the variables end up in the same
>>> section
>>> -   with a single comdat name.
>>> -
>>> +/* Switch to a COMDAT section with COMDAT name of decl.
>>> +   
>>>    FIXME:  resolve_unique_section needs to deal better with
>>>    decls with both DECL_SECTION_NAME and DECL_ONE_ONLY.  Once
>>>    that is fixed, this if-else statement can be replaced with
>>>    a single call to "switch_to_section (sect)".  */
>>> 
>>> -static void
>>> -handle_vtv_comdat_section (section *sect, const_tree decl
>>> ATTRIBUTE_UNUSED)
>>> +void
>>> +switch_to_comdat_section (section *sect, tree decl)
>>> {
>>> #if defined (OBJECT_FORMAT_ELF)
>>>   targetm.asm_out.named_section (sect->named.name,
>>>                 sect->named.common.flags
>>>                 | SECTION_LINKONCE,
>>> -                 DECL_NAME (decl));
>>> +                 decl);
>>>   in_section = sect;
>>> #else
>>>   /* Neither OBJECT_FORMAT_PE, nor OBJECT_FORMAT_COFF is set here.
>>> @@ -8490,18 +8486,18 @@ handle_vtv_comdat_section (section *sect,
>>> const_tree decl ATTRIBUTE_UNUSED)
>>>     {
>>>       char *name;
>>> 
>>> -      if (TREE_CODE (DECL_NAME (decl)) == IDENTIFIER_NODE)
>>> +      if (TREE_CODE (decl) == IDENTIFIER_NODE)
>>>    name = ACONCAT ((sect->named.name, "$",
>>> -             IDENTIFIER_POINTER (DECL_NAME (decl)), NULL));
>>> +             IDENTIFIER_POINTER (decl), NULL));
>>>       else
>>>    name = ACONCAT ((sect->named.name, "$",
>>> -             IDENTIFIER_POINTER (DECL_COMDAT_GROUP
>>> (DECL_NAME (decl))),
>>> +             IDENTIFIER_POINTER (DECL_COMDAT_GROUP (decl)),
>>>             NULL));
>>> 
>>>       targetm.asm_out.named_section (name,
>>>                     sect->named.common.flags
>>>                     | SECTION_LINKONCE,
>>> -                     DECL_NAME (decl));
>>> +                     decl);
>>>       in_section = sect;
>>>     }
>>>   else
>>> @@ -8509,4 +8505,15 @@ handle_vtv_comdat_section (section *sect,
>>> const_tree decl ATTRIBUTE_UNUSED)
>>> #endif
>>> }
>>> 
>>> +/* This function ensures that vtable_map variables are not only
>>> +   in the comdat section, but that each variable has its own
>>> unique
>>> +   comdat name.  Without this the variables end up in the same
>>> section
>>> +   with a single comdat name.  */
>>> +
>>> +static void
>>> +handle_vtv_comdat_section (section *sect, const_tree decl
>>> ATTRIBUTE_UNUSED)
>>> +{
>>> +  switch_to_comdat_section(sect, DECL_NAME (decl));
>>> +}
>>> +
>>> #include "gt-varasm.h"
>>> diff --git a/gcc/varasm.h b/gcc/varasm.h
>>> index d5d8c4e5578..8ba8374e779 100644
>>> --- a/gcc/varasm.h
>>> +++ b/gcc/varasm.h
>>> @@ -79,4 +79,6 @@ extern rtx assemble_static_space (unsigned
>>> HOST_WIDE_INT);
>>> 
>>> extern rtx assemble_trampoline_template (void);
>>> 
>>> +extern void switch_to_comdat_section (section *, tree);
>>> +
>>> #endif  // GCC_VARASM_H
>>>