On Thu, 14 Apr 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.
>
> Boostrapped and regtested on x86_64 linux.
>
> OK for Stage4?
>
> 2022-04-13 Giuliano Belinassi <gbelina...@suse.de>
>
> PR c++/105169
> * targhooks.cc (default_print_patchable_function_entry_1): Handle
> COMDAT case.
> * varasm.cc (handle_vtv_comdat_section): Rename to...
> (switch_to_comdat_section): Generalize to also cover
> __patchable_function_entry case.
> (assemble_variable): Rename call from handle_vtv_comdat_section to
> switch_to_comdat_section.
> (output_object_block): Same as above.
> * varasm.h: Declare switch_to_comdat_section.
>
> 2022-04-13 Giuliano Belinassi <gbelina...@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.
>
> Signed-off-by: Giuliano Belinassi <gbelina...@suse.de>
> ---
> gcc/targhooks.cc | 8 ++++++--
> gcc/testsuite/ChangeLog | 7 +++++++
> 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 | 25 +++++++++++++----------
> gcc/varasm.h | 1 +
> 7 files changed, 87 insertions(+), 13 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 e22bc66a6c8..540460e7db9 100644
> --- a/gcc/targhooks.cc
> +++ b/gcc/targhooks.cc
> @@ -1995,8 +1995,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);
You are passing a decl here, but ...
> + else
> + switch_to_section (sect);
> assemble_align (POINTER_SIZE);
> fputs (asm_op, file);
> assemble_name_raw (file, buf);
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 9ab7a178bf8..524a546a832 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,10 @@
> +2022-04-13 Giuliano Belinassi <giuliano.belina...@suse.com>
> +
> + 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.
> +
> 2022-04-12 Antoni Boucher <boua...@zoho.com>
>
> PR jit/104293
> 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..7cd91e2bb56 100644
> --- a/gcc/varasm.cc
> +++ b/gcc/varasm.cc
> @@ -128,7 +128,6 @@ static void asm_output_aligned_bss (FILE *, tree, const
> char *,
> #endif /* BSS_SECTION_ASM_OP */
> static void mark_weak (tree);
> static void output_constant_pool (const char *, tree);
> -static void handle_vtv_comdat_section (section *, const_tree);
>
> /* Well-known sections, each one associated with some sort of *_ASM_OP. */
> section *text_section;
> @@ -2406,7 +2405,7 @@ assemble_variable (tree decl, int top_level
> ATTRIBUTE_UNUSED,
> /* Special-case handling of vtv comdat sections. */
> if (sect->named.name
> && (strcmp (sect->named.name, ".vtable_map_vars") == 0))
> - handle_vtv_comdat_section (sect, decl);
> + switch_to_comdat_section (sect, DECL_NAME (decl));
... possibly an IDENTIFIER_NODE here
> else
> switch_to_section (sect, decl);
> if (align > BITS_PER_UNIT)
> @@ -8037,7 +8036,7 @@ output_object_block (struct object_block *block)
> if (SECTION_STYLE (block->sect) == SECTION_NAMED
> && block->sect->named.name
> && (strcmp (block->sect->named.name, ".vtable_map_vars") == 0))
> - handle_vtv_comdat_section (block->sect, block->sect->named.decl);
> + switch_to_comdat_section (block->sect, DECL_NAME
> (block->sect->named.decl));
> else
> switch_to_section (block->sect, SYMBOL_REF_DECL ((*block->objects)[0]));
and here.
> @@ -8458,7 +8457,7 @@ default_asm_output_ident_directive (const char
> *ident_str)
> }
>
>
> -/* This function ensures that vtable_map variables are not only
> +/* This function ensures that 'section' 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.
> @@ -8468,14 +8467,18 @@ default_asm_output_ident_directive (const char
> *ident_str)
> that is fixed, this if-else statement can be replaced with
> a single call to "switch_to_section (sect)". */
Please update the functions overall comment.
> -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)
> + /* In case decl is not in a COMDAT group, we can not switch to the COMDAT
> + section. So assert that this condition is always true. */
> + gcc_assert (DECL_COMDAT_GROUP (decl));
> +
here you are relying on 'decl' being a decl.
> 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 +8493,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)
But here it can appearantly be a name. Maybe really always pass in the
decl?
Did you test on a non-ELF target? Did you configure with
--enable-vtable-verify to increase test coverage? There
seems to be only two testcases passing -fvtable-verify=std,
g++.dg/ubsan/pr59437.C and testsuite/g++.dg/ubsan/pr59415.C
> 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
> diff --git a/gcc/varasm.h b/gcc/varasm.h
> index d5d8c4e5578..233301d6952 100644
> --- a/gcc/varasm.h
> +++ b/gcc/varasm.h
> @@ -41,6 +41,7 @@ extern void process_pending_assemble_externals (void);
> extern bool decl_replaceable_p (tree, bool);
> extern bool decl_binds_to_current_def_p (const_tree);
> extern enum tls_model decl_default_tls_model (const_tree);
> +extern void switch_to_comdat_section (section *, tree);
>
> /* Declare DECL to be a weak symbol. */
> extern void declare_weak (tree);
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
