Hi,
On Tue, 2022-04-19 at 10:11 +0200, Richard Biener wrote:
> 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
Indeed, GCC doesn't even compile with this patch when --enable-vtable-
verify is enabled. The next patch does compile and I see no fails
regarding those tests.
As for testing on non-ELF targets, I was unabled
to reproduce this problem in Windows (test case attached to bugzilla
compiles fine there). I guess we will only see if this is also a
problem there when this feature gets widely used there.
Giuliano.
>
> > 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);
> >
