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

[Richard Biener <rguenther@suse.de>]

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.

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
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)