| Message ID | 1d8a0534-8204-97d3-b2ee-a5260fc4dde2@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | c++: cross-module __cxa_atexit use [PR 98531] | expand |
Hi Nathan, > Solaris tickled this bug as it has some mutex/sync/something primitive with > a destructor, hence wanted to generate a __cxa_atexit call inside an > inline/template function. But the problem is not solaris-specific. > > I tested this bootstrapping both x86_64-linux and aarch64-linux. I'll > commit in a couple of days if there are no further comments. I've tried the patch on Solaris last night, with mixed results (some regressions, remaining failures). I've reported the details in the PR. Rainer
As Rainer pointed out, there were some regressions in the library tests. That's because we didn't build the correct ehspec for __cxa_atexit. This adds that, but also, I realize we can use the 'hidden' flag on pushdecl to make this lazy-builtin not visible to user name lookup without them already declaring it. And I realized we should be setting the location as BUILTIN_LOCATION because that's what we're doing. Finally the duplicate_decl processing needed augmenting to allow such a lazy builtin's eh spec to differ from one already known. (The converse is already dealt with, as that's the only case we had before.) As I mentioned in the first patch, a cleanup of the 'lazily declare a builtin' API would be a nice stage-1 thing. I'll hold off applying this until next week. Rainer, I don't yet know if this resolves all of the issues you encountered in the PR. On 1/25/21 1:01 PM, Nathan Sidwell wrote: > [ARM has unique ABI here, hence cc'ing Richard] > > Solaris tickled this bug as it has some mutex/sync/something primitive > with a destructor, hence wanted to generate a __cxa_atexit call inside > an inline/template function. But the problem is not solaris-specific. > > I tested this bootstrapping both x86_64-linux and aarch64-linux. I'll > commit in a couple of days if there are no further comments. > > The compiler's use of lazily-declared library functions must insert > said functions into a symbol table, so that they can be correctly > merged across TUs at the module-level. We have too many different > ways of declaring such library functions. This fixes __cxa_atexit (or > its system-specific variations), pushing (or merging) the decl into > the appropriate namespace. > > PR c++/98531 > gcc/cp/ > * cp-tree.h (push_abi_namespace, pop_abi_namespace): Declare. > * decl.c (push_abi_namespace, pop_abi_namespace): Moved > from rtti.c, add default namespace arg. > (declare_global_var): Use push/pop_abi_namespace. > (get_atexit_node): Push the fndecl into a namespace. > * rtti.c (push_abi_namespace, pop_abi_namespace): Moved to > decl.c. > gcc/testsuite/ > * g++.dg/modules/pr98531.h: New. > * g++.dg/modules/pr98531_a.H: New. > * g++.dg/modules/pr98531_b.C: New. >
Hi Nathan, > As Rainer pointed out, there were some regressions in the library tests. > That's because we didn't build the correct ehspec for __cxa_atexit. > This adds that, but also, I realize we can use the 'hidden' flag on > pushdecl to make this lazy-builtin not visible to user name lookup > without them already declaring it. And I realized we should be setting the > location as BUILTIN_LOCATION because that's what we're doing. Finally the > duplicate_decl processing needed augmenting to allow such a lazy builtin's > eh spec to differ from one already known. (The converse is already dealt > with, as that's the only case we had before.) > > As I mentioned in the first patch, a cleanup of the 'lazily declare a > builtin' API would be a nice stage-1 thing. > > I'll hold off applying this until next week. Rainer, I don't yet know if > this resolves all of the issues you encountered in the PR. unfortunately, the results are not very encouraging: while the new libstdc++ failures caused by the v1 patch are gone, the g++.dg/modules ICEs remain unchanged, and on Solaris 11.3 (or with -fno-use-cxa-atexit), the new tests ICE, too. Full details in the PR. Rainer
[Dropping Richard, as I don't think his input is necessary further]
Hi Rainer,
here are two patches, to be applied in sequence (replacing earlier
versions of the patch).
The first is merely a testsuite correction of the earlier patch to
inhibit the cxa_atexit-specific tests where it matters. (I don't think
it matters in the ABI ones, if the library does not provide them, we
won't hit a declaration that could differ)
The second fixes the atexit issue (and array entity with cxa_atexit) you
encountered on solaris 11.3.
With these two patches I could build the xtreme-header-2_a.ii you
provided on an i386-solaris2.11 xcompiler (I can only compile there, not
got a complete runtime to run the tests).
Please let me know how these behave, thanks.
patch 3a:
c++: cross-module __cxa_atexit use [PR 98531]
The compiler's use of lazily-declared library functions must insert
said functions into a symbol table, so that they can be correctly
merged across TUs at the module-level. We have too many different
ways of declaring such library functions. This fixes __cxa_atexit (or
its system-specific variations), pushing (or merging) the decl into
the appropriate namespace. Because we're pushing a lazy builtin,
check_redeclaration_exception_specification needed a tweak to allow a
such a builtin's eh spec to differ from what the user may have already
declared. (I suspect no all headers declare atexit as noexcept.)
We can't test the -fno-use-cxa-atexit path with modules, as that
requires a followup patch to a closely related piece (which also
affects cxa_atexit targets in other circumstances).
PR c++/98531
gcc/cp/
* cp-tree.h (push_abi_namespace, pop_abi_namespace): Declare.
* decl.c (push_abi_namespace, pop_abi_namespace): Moved
from rtti.c, add default namespace arg.
(check_redeclaration_exception_specification): Allow a lazy
builtin's eh spec to differ from an lready-declared user
declaration.
(declare_global_var): Use push/pop_abi_namespace.
(get_atexit_node): Push the fndecl into a namespace.
* rtti.c (push_abi_namespace, pop_abi_namespace): Moved to
decl.c.
gcc/testsuite/
* g++.dg/modules/pr98531-1.h: New.
* g++.dg/modules/pr98531-1_a.H: New.
* g++.dg/modules/pr98531-1_b.C: New.
* g++.dg/abi/pr98531-1.C: New.
* g++.dg/abi/pr98531-2.C: New.
* g++.dg/abi/pr98531-3.C: New.
* g++.dg/abi/pr98531-4.C: New.
patch 3b:
c++: cleanup function name [PR 98531]
The next piece of 98531 is that in some cases we need to create a
cleanup function to do the work (when the object is an array, or we're
using regular atexit). We were not pushing that function's decl
anywhere (not giving it a context) so streaming it failed.
This is a partial fix. You'll notice we're naming these from a per-TU
counter. I've captured that in PR98893.
gcc/cp/
* decl.c (start_cleanup_fn): Push function into
namespace.
gcc/testsuite/
* g++.dg/modules/pr98531-2.h: New.
* g++.dg/modules/pr98531-2_a.H: New.
* g++.dg/modules/pr98531-2_b.C: New.
* g++.dg/modules/pr98531-3.h: New.
* g++.dg/modules/pr98531-3_a.H: New.
* g++.dg/modules/pr98531-3_b.C: New.
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 51139f4a4be..fefd870360b 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -193,7 +193,9 @@ enum cp_tree_index CPTI_MODULE_HWM, /* Nodes after here change during compilation, or should not be in - the module's global tree table. */ + the module's global tree table. Such nodes must be locatable + via name lookup or type-construction, as those are the only + cross-TU matching capabilities remaining. */ /* We must find these via the global namespace. */ CPTI_STD, @@ -6620,6 +6622,9 @@ extern tree make_typename_type (tree, tree, enum tag_types, tsubst_flags_t); extern tree build_typename_type (tree, tree, tree, tag_types); extern tree make_unbound_class_template (tree, tree, tree, tsubst_flags_t); extern tree make_unbound_class_template_raw (tree, tree, tree); +extern unsigned push_abi_namespace (tree node = abi_node); +extern void pop_abi_namespace (unsigned flags, + tree node = abi_node); extern tree build_library_fn_ptr (const char *, tree, int); extern tree build_cp_library_fn_ptr (const char *, tree, int); extern tree push_library_fn (tree, tree, tree, int); diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index 1a114a2e2d0..1f97bcf1e55 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -4696,6 +4696,30 @@ cxx_init_decl_processing (void) using_eh_for_cleanups (); } +/* Enter an abi node in global-module context. returns a cookie to + give to pop_abi_namespace. */ + +unsigned +push_abi_namespace (tree node) +{ + push_nested_namespace (node); + push_visibility ("default", 2); + unsigned flags = module_kind; + module_kind = 0; + return flags; +} + +/* Pop an abi namespace, FLAGS is the cookie push_abi_namespace gave + you. */ + +void +pop_abi_namespace (unsigned flags, tree node) +{ + module_kind = flags; + pop_visibility (2); + pop_nested_namespace (node); +} + /* Create the VAR_DECL for __FUNCTION__ etc. ID is the name to give the decl, LOC is the location to give the decl, NAME is the initialization string and TYPE_DEP indicates whether NAME depended @@ -8668,21 +8692,19 @@ cp_finish_decomp (tree decl, tree first, unsigned int count) static tree declare_global_var (tree name, tree type) { - tree decl; - - push_to_top_level (); - decl = build_decl (input_location, VAR_DECL, name, type); + auto cookie = push_abi_namespace (global_namespace); + tree decl = build_decl (input_location, VAR_DECL, name, type); TREE_PUBLIC (decl) = 1; DECL_EXTERNAL (decl) = 1; DECL_ARTIFICIAL (decl) = 1; - DECL_CONTEXT (decl) = FROB_CONTEXT (global_namespace); + DECL_CONTEXT (decl) = FROB_CONTEXT (current_namespace); /* If the user has explicitly declared this variable (perhaps because the code we are compiling is part of a low-level runtime library), then it is possible that our declaration will be merged with theirs by pushdecl. */ decl = pushdecl (decl); cp_finish_decl (decl, NULL_TREE, false, NULL_TREE, 0); - pop_from_top_level (); + pop_abi_namespace (cookie, global_namespace); return decl; } @@ -8727,6 +8749,7 @@ get_atexit_node (void) tree fn_ptr_type; const char *name; bool use_aeabi_atexit; + tree ctx = global_namespace; if (atexit_node) return atexit_node; @@ -8762,9 +8785,20 @@ get_atexit_node (void) argtype0, argtype1, argtype2, NULL_TREE); if (use_aeabi_atexit) - name = "__aeabi_atexit"; + { + name = "__aeabi_atexit"; + push_to_top_level (); + int n = push_namespace (get_identifier ("__aeabiv1"), false); + ctx = current_namespace; + while (n--) + pop_namespace (); + pop_from_top_level (); + } else - name = "__cxa_atexit"; + { + name = "__cxa_atexit"; + ctx = abi_node; + } } else { @@ -8783,7 +8817,11 @@ get_atexit_node (void) /* Now, build the function declaration. */ push_lang_context (lang_name_c); + auto cookie = push_abi_namespace (ctx); atexit_fndecl = build_library_fn_ptr (name, fn_type, ECF_LEAF | ECF_NOTHROW); + DECL_CONTEXT (atexit_fndecl) = FROB_CONTEXT (current_namespace); + atexit_fndecl = pushdecl (atexit_fndecl); + pop_abi_namespace (cookie, ctx); mark_used (atexit_fndecl); pop_lang_context (); atexit_node = decay_conversion (atexit_fndecl, tf_warning_or_error); diff --git a/gcc/cp/rtti.c b/gcc/cp/rtti.c index d3cb9ad4425..b41d95469c6 100644 --- a/gcc/cp/rtti.c +++ b/gcc/cp/rtti.c @@ -143,24 +143,6 @@ static bool typeinfo_in_lib_p (tree); static int doing_runtime = 0; -static unsigned -push_abi_namespace (void) -{ - push_nested_namespace (abi_node); - push_visibility ("default", 2); - unsigned flags = module_kind; - module_kind = 0; - return flags; -} - -static void -pop_abi_namespace (unsigned flags) -{ - module_kind = flags; - pop_visibility (2); - pop_nested_namespace (abi_node); -} - /* Declare language defined type_info type and a pointer to const type_info. This is incomplete here, and will be completed when the user #includes <typeinfo>. There are language defined diff --git a/gcc/testsuite/g++.dg/modules/pr98531.h b/gcc/testsuite/g++.dg/modules/pr98531.h new file mode 100644 index 00000000000..62d4c1d1f90 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr98531.h @@ -0,0 +1,13 @@ + +struct __waiters +{ + __waiters() noexcept; + ~__waiters () noexcept; + + static __waiters &_S_for() + { + static __waiters w; + + return w; + } +}; diff --git a/gcc/testsuite/g++.dg/modules/pr98531_a.H b/gcc/testsuite/g++.dg/modules/pr98531_a.H new file mode 100644 index 00000000000..81fd00fb6a5 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr98531_a.H @@ -0,0 +1,5 @@ +// { dg-additional-options -fmodule-header } +// PR c++ 98531 no-context __cxa_atexit +// { dg-module-cmi {} } + +#include "pr98531.h" diff --git a/gcc/testsuite/g++.dg/modules/pr98531_b.C b/gcc/testsuite/g++.dg/modules/pr98531_b.C new file mode 100644 index 00000000000..5b2d96ddfe2 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr98531_b.C @@ -0,0 +1,4 @@ +// { dg-additional-options "-fmodules-ts -fno-module-lazy" } + +#include "pr98531.h" +import "pr98531_a.H";
[ARM has unique ABI here, hence cc'ing Richard] Solaris tickled this bug as it has some mutex/sync/something primitive with a destructor, hence wanted to generate a __cxa_atexit call inside an inline/template function. But the problem is not solaris-specific. I tested this bootstrapping both x86_64-linux and aarch64-linux. I'll commit in a couple of days if there are no further comments. The compiler's use of lazily-declared library functions must insert said functions into a symbol table, so that they can be correctly merged across TUs at the module-level. We have too many different ways of declaring such library functions. This fixes __cxa_atexit (or its system-specific variations), pushing (or merging) the decl into the appropriate namespace. PR c++/98531 gcc/cp/ * cp-tree.h (push_abi_namespace, pop_abi_namespace): Declare. * decl.c (push_abi_namespace, pop_abi_namespace): Moved from rtti.c, add default namespace arg. (declare_global_var): Use push/pop_abi_namespace. (get_atexit_node): Push the fndecl into a namespace. * rtti.c (push_abi_namespace, pop_abi_namespace): Moved to decl.c. gcc/testsuite/ * g++.dg/modules/pr98531.h: New. * g++.dg/modules/pr98531_a.H: New. * g++.dg/modules/pr98531_b.C: New.