Re: [PATCH] Avoid depending on destructor order
On 9/23/22 10:12, Thomas Neumann wrote: +static const bool in_shutdown = false; I'll let Jason or others decide if this is the right solution. It seems that in_shutdown also could be declared outside the #ifdef and initialized as "false". sure, either is fine. Moving it outside the #ifdef wastes one byte in the executable (while the compiler can eliminate the const), but it does not really matter. Might as well go with your patch, then, adding a comment to explain why the variable is defined in two places. I have verified that the patch below fixes builds for both fast-path and non-fast-path builds. But if you prefer I will move the in_shutdown definition instead. Best Thomas PS: in_shutdown is an int here instead of a bool because non-fast-path builds do not include stdbool. Not a good reason, of course, but I wanted to keep the patch minimal and it makes no difference in practice. When using the atomic fast path deregistering can fail during program shutdown if the lookup structures are already destroyed. The assert in __deregister_frame_info_bases takes that into account. In the non-fast-path case however is not aware of program shutdown, which caused a compiler error on such platforms. We fix that by introducing a constant for in_shutdown in non-fast-path builds. libgcc/ChangeLog: * unwind-dw2-fde.c: Introduce a constant for in_shutdown for the non-fast-path case. diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c index d237179f4ea..0bcd5061d76 100644 --- a/libgcc/unwind-dw2-fde.c +++ b/libgcc/unwind-dw2-fde.c @@ -67,6 +67,8 @@ static void init_object (struct object *ob); #else +/* Without fast path frame deregistration must always succeed. */ +static const int in_shutdown = 0; /* The unseen_objects list contains objects that have been registered but not yet categorized in any way. The seen_objects list has had
Re: [PATCH] Avoid depending on destructor order
Hi Iain,
You might also want to include Rainer’s patch,
AFAIR patches to fix bootstrap are allowed to proceed as an exception to
the usual rules,
I was not aware of that. I have pushed the patch below now (including
Rainer's change), I will update the code if requested.
Best
Thomas
fix assert in __deregister_frame_info_bases
When using the atomic fast path deregistering can fail during
program shutdown if the lookup structures are already destroyed.
The assert in __deregister_frame_info_bases takes that into
account. In the non-fast-path case however is not aware of
program shutdown, which caused a compiler error on such platforms.
We fix that by introducing a constant for in_shutdown in
non-fast-path builds.
We also drop the destructor priority, as it is not supported on
all platforms and we no longer rely upon the priority anyway.
libgcc/ChangeLog:
* unwind-dw2-fde.c: Introduce a constant for in_shutdown
for the non-fast-path case. Drop destructor priority.
diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c
index d237179f4ea..3c0cc654ec0 100644
--- a/libgcc/unwind-dw2-fde.c
+++ b/libgcc/unwind-dw2-fde.c
@@ -51,7 +51,7 @@ static struct btree registered_frames;
static bool in_shutdown;
static void
-release_registered_frames (void) __attribute__ ((destructor (110)));
+release_registered_frames (void) __attribute__ ((destructor));
static void
release_registered_frames (void)
{
@@ -67,6 +67,8 @@ static void
init_object (struct object *ob);
#else
+/* Without fast path frame deregistration must always succeed. */
+static const int in_shutdown = 0;
/* The unseen_objects list contains objects that have been registered
but not yet categorized in any way. The seen_objects list has had
Re: [PATCH] Avoid depending on destructor order
> On 26 Sep 2022, at 12:49, Thomas Neumann via Gcc-patches > wrote: > > Hi Claudiu, > >> This change prohibits compiling of ARC backend: >>> + gcc_assert (in_shutdown || ob); >> in_shutdown is only defined when ATOMIC_FDE_FAST_PATH is defined, >> while gcc_assert is outside of any ifdef. Please can you revisit this >> line and change it accordingly. > > I have a patch ready, I am waiting for someone to approve my patch: > > https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602130.html You might also want to include Rainer’s patch, AFAIR patches to fix bootstrap are allowed to proceed as an exception to the usual rules, Iain
Re: [PATCH] Avoid depending on destructor order
Thanks, I haven't observed it. Waiting for it, Claudiu On Mon, Sep 26, 2022 at 2:49 PM Thomas Neumann wrote: > > Hi Claudiu, > > > This change prohibits compiling of ARC backend: > > > >> + gcc_assert (in_shutdown || ob); > > > > in_shutdown is only defined when ATOMIC_FDE_FAST_PATH is defined, > > while gcc_assert is outside of any ifdef. Please can you revisit this > > line and change it accordingly. > > I have a patch ready, I am waiting for someone to approve my patch: > > https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602130.html > > Best > > Thomas
Re: [PATCH] Avoid depending on destructor order
Hi Claudiu, This change prohibits compiling of ARC backend: + gcc_assert (in_shutdown || ob); in_shutdown is only defined when ATOMIC_FDE_FAST_PATH is defined, while gcc_assert is outside of any ifdef. Please can you revisit this line and change it accordingly. I have a patch ready, I am waiting for someone to approve my patch: https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602130.html Best Thomas
Re: [PATCH] Avoid depending on destructor order
Hi Thomas, This change prohibits compiling of ARC backend: > + gcc_assert (in_shutdown || ob); in_shutdown is only defined when ATOMIC_FDE_FAST_PATH is defined, while gcc_assert is outside of any ifdef. Please can you revisit this line and change it accordingly. Thanks, Claudiu
Re: [PATCH] Avoid depending on destructor order
Hi Jeff,
>>> Thanks for the patch. I'll let you and Jason decide which style solution
>>> is preferred.
>> This also breaks bootstrap on Darwin at least, so an early solution would be
>> welcome (the fix here allows bootstrap to continue, testing on-going).
>> thanks,
>
> I'm using it in the automated tester as well -- without all the *-elf
> targets would fail to build libgcc.
things are even worse on targets that lack constructor priority support,
like Solaris 11.3 and Mac OS X 10.7/Darwin 11:
In file included from
/vol/gcc/src/hg/master/local/libgcc/unwind-dw2-fde-dip.c:97:
/vol/gcc/src/hg/master/local/libgcc/unwind-dw2-fde.c:54:1: error: destructor
priorities are not supported
54 | release_registered_frames (void) __attribute__ ((destructor (110)));
| ^
This is already checked for in libgcc/configure, and the situation
handled in libgcc/config/i386/cpuinfo.c. The following patch unbroke
bootstrap on both affected targets and I saw no apparent regressions.
However, I cannot tell if the destructor priority is actually required
for correctness.
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c
--- a/libgcc/unwind-dw2-fde.c
+++ b/libgcc/unwind-dw2-fde.c
@@ -47,11 +47,17 @@ typedef __UINTPTR_TYPE__ uintptr_type;
#ifdef ATOMIC_FDE_FAST_PATH
#include "unwind-dw2-btree.h"
+#ifdef HAVE_INIT_PRIORITY
+#define DESTRUCTOR_PRIORITY (110)
+#else
+#define DESTRUCTOR_PRIORITY
+#endif
+
static struct btree registered_frames;
static bool in_shutdown;
static void
-release_registered_frames (void) __attribute__ ((destructor (110)));
+release_registered_frames (void) __attribute__ ((destructor DESTRUCTOR_PRIORITY));
static void
release_registered_frames (void)
{
Re: [PATCH] Avoid depending on destructor order
On 9/25/22 00:29, Iain Sandoe wrote: On 23 Sep 2022, at 15:30, David Edelsohn via Gcc-patches wrote: On Fri, Sep 23, 2022 at 10:12 AM Thomas Neumann wrote: +static const bool in_shutdown = false; I'll let Jason or others decide if this is the right solution. It seems that in_shutdown also could be declared outside the #ifdef and initialized as "false". sure, either is fine. Moving it outside the #ifdef wastes one byte in the executable (while the compiler can eliminate the const), but it does not really matter. I have verified that the patch below fixes builds for both fast-path and non-fast-path builds. But if you prefer I will move the in_shutdown definition instead. Best Thomas PS: in_shutdown is an int here instead of a bool because non-fast-path builds do not include stdbool. Not a good reason, of course, but I wanted to keep the patch minimal and it makes no difference in practice. When using the atomic fast path deregistering can fail during program shutdown if the lookup structures are already destroyed. The assert in __deregister_frame_info_bases takes that into account. In the non-fast-path case however is not aware of program shutdown, which caused a compiler error on such platforms. We fix that by introducing a constant for in_shutdown in non-fast-path builds. libgcc/ChangeLog: * unwind-dw2-fde.c: Introduce a constant for in_shutdown for the non-fast-path case. diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c index d237179f4ea..0bcd5061d76 100644 --- a/libgcc/unwind-dw2-fde.c +++ b/libgcc/unwind-dw2-fde.c @@ -67,6 +67,8 @@ static void init_object (struct object *ob); #else +/* Without fast path frame deregistration must always succeed. */ +static const int in_shutdown = 0; /* The unseen_objects list contains objects that have been registered but not yet categorized in any way. The seen_objects list has had Thanks for the patch. I'll let you and Jason decide which style solution is preferred. This also breaks bootstrap on Darwin at least, so an early solution would be welcome (the fix here allows bootstrap to continue, testing on-going). thanks, I'm using it in the automated tester as well -- without all the *-elf targets would fail to build libgcc. jeff
Re: [PATCH] Avoid depending on destructor order
> On 23 Sep 2022, at 15:30, David Edelsohn via Gcc-patches > wrote: > > On Fri, Sep 23, 2022 at 10:12 AM Thomas Neumann wrote: > >>> >>>+static const bool in_shutdown = false; >>> >>> I'll let Jason or others decide if this is the right solution. It seems >>> that in_shutdown also could be declared outside the #ifdef and >>> initialized as "false". >> >> sure, either is fine. Moving it outside the #ifdef wastes one byte in >> the executable (while the compiler can eliminate the const), but it does >> not really matter. >> >> I have verified that the patch below fixes builds for both fast-path and >> non-fast-path builds. But if you prefer I will move the in_shutdown >> definition instead. >> >> Best >> >> Thomas >> >> PS: in_shutdown is an int here instead of a bool because non-fast-path >> builds do not include stdbool. Not a good reason, of course, but I >> wanted to keep the patch minimal and it makes no difference in practice. >> >> >> When using the atomic fast path deregistering can fail during >> program shutdown if the lookup structures are already destroyed. >> The assert in __deregister_frame_info_bases takes that into >> account. In the non-fast-path case however is not aware of >> program shutdown, which caused a compiler error on such platforms. >> We fix that by introducing a constant for in_shutdown in >> non-fast-path builds. >> >> libgcc/ChangeLog: >> * unwind-dw2-fde.c: Introduce a constant for in_shutdown >> for the non-fast-path case. >> >> diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c >> index d237179f4ea..0bcd5061d76 100644 >> --- a/libgcc/unwind-dw2-fde.c >> +++ b/libgcc/unwind-dw2-fde.c >> @@ -67,6 +67,8 @@ static void >> init_object (struct object *ob); >> >> #else >> +/* Without fast path frame deregistration must always succeed. */ >> +static const int in_shutdown = 0; >> >> /* The unseen_objects list contains objects that have been registered >> but not yet categorized in any way. The seen_objects list has had >> > > Thanks for the patch. I'll let you and Jason decide which style solution > is preferred. This also breaks bootstrap on Darwin at least, so an early solution would be welcome (the fix here allows bootstrap to continue, testing on-going). thanks, Iain > > Thanks, David
Re: [PATCH] Avoid depending on destructor order
On Fri, Sep 23, 2022 at 10:12 AM Thomas Neumann wrote: > > > > +static const bool in_shutdown = false; > > > > I'll let Jason or others decide if this is the right solution. It seems > > that in_shutdown also could be declared outside the #ifdef and > > initialized as "false". > > sure, either is fine. Moving it outside the #ifdef wastes one byte in > the executable (while the compiler can eliminate the const), but it does > not really matter. > > I have verified that the patch below fixes builds for both fast-path and > non-fast-path builds. But if you prefer I will move the in_shutdown > definition instead. > > Best > > Thomas > > PS: in_shutdown is an int here instead of a bool because non-fast-path > builds do not include stdbool. Not a good reason, of course, but I > wanted to keep the patch minimal and it makes no difference in practice. > > > When using the atomic fast path deregistering can fail during > program shutdown if the lookup structures are already destroyed. > The assert in __deregister_frame_info_bases takes that into > account. In the non-fast-path case however is not aware of > program shutdown, which caused a compiler error on such platforms. > We fix that by introducing a constant for in_shutdown in > non-fast-path builds. > > libgcc/ChangeLog: > * unwind-dw2-fde.c: Introduce a constant for in_shutdown > for the non-fast-path case. > > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c > index d237179f4ea..0bcd5061d76 100644 > --- a/libgcc/unwind-dw2-fde.c > +++ b/libgcc/unwind-dw2-fde.c > @@ -67,6 +67,8 @@ static void > init_object (struct object *ob); > > #else > +/* Without fast path frame deregistration must always succeed. */ > +static const int in_shutdown = 0; > > /* The unseen_objects list contains objects that have been registered > but not yet categorized in any way. The seen_objects list has had > Thanks for the patch. I'll let you and Jason decide which style solution is preferred. Thanks, David
Re: [PATCH] Avoid depending on destructor order
+static const bool in_shutdown = false; I'll let Jason or others decide if this is the right solution. It seems that in_shutdown also could be declared outside the #ifdef and initialized as "false". sure, either is fine. Moving it outside the #ifdef wastes one byte in the executable (while the compiler can eliminate the const), but it does not really matter. I have verified that the patch below fixes builds for both fast-path and non-fast-path builds. But if you prefer I will move the in_shutdown definition instead. Best Thomas PS: in_shutdown is an int here instead of a bool because non-fast-path builds do not include stdbool. Not a good reason, of course, but I wanted to keep the patch minimal and it makes no difference in practice. When using the atomic fast path deregistering can fail during program shutdown if the lookup structures are already destroyed. The assert in __deregister_frame_info_bases takes that into account. In the non-fast-path case however is not aware of program shutdown, which caused a compiler error on such platforms. We fix that by introducing a constant for in_shutdown in non-fast-path builds. libgcc/ChangeLog: * unwind-dw2-fde.c: Introduce a constant for in_shutdown for the non-fast-path case. diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c index d237179f4ea..0bcd5061d76 100644 --- a/libgcc/unwind-dw2-fde.c +++ b/libgcc/unwind-dw2-fde.c @@ -67,6 +67,8 @@ static void init_object (struct object *ob); #else +/* Without fast path frame deregistration must always succeed. */ +static const int in_shutdown = 0; /* The unseen_objects list contains objects that have been registered but not yet categorized in any way. The seen_objects list has had
Re: [PATCH] Avoid depending on destructor order
On Fri, Sep 23, 2022 at 9:38 AM Thomas Neumann wrote: > > This patch broke bootstrap on AIX and probably other targets. > > > > #ifdef ATOMIC_FDE_FAST_PATH > > #include "unwind-dw2-btree.h" > > > > static struct btree registered_frames; > > static bool in_shutdown; > > ... > > #else > > > > in_shutdown only is defined for ATOMIC_FDE_FAST_PATH but used in code / > > asserts not protected by that macro. > > > >gcc_assert (in_shutdown || ob); > >return (void *) ob; > > } > > I am sorry for that, I did not consider that my test machines all use > the fast path. > > I think the problem can be fixed by the trivial patch below, I will > commit that after I have tested builds both with and without fast path. > > Best > > Thomas > > > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c > index d237179f4ea..d6e347c5481 100644 > --- a/libgcc/unwind-dw2-fde.c > +++ b/libgcc/unwind-dw2-fde.c > @@ -67,6 +67,8 @@ static void > init_object (struct object *ob); > > #else > +/* Without fast path frame lookup must always succeed */ > +static const bool in_shutdown = false; > > /* The unseen_objects list contains objects that have been registered > but not yet categorized in any way. The seen_objects list has had > I tried the patch but it still failed because the type name "bool" is not known. This patch is the only use of "bool" in the libgcc source code, which is C, not C++. Thanks, David
Re: [PATCH] Avoid depending on destructor order
On Fri, Sep 23, 2022 at 9:38 AM Thomas Neumann wrote: > > This patch broke bootstrap on AIX and probably other targets. > > > > #ifdef ATOMIC_FDE_FAST_PATH > > #include "unwind-dw2-btree.h" > > > > static struct btree registered_frames; > > static bool in_shutdown; > > ... > > #else > > > > in_shutdown only is defined for ATOMIC_FDE_FAST_PATH but used in code / > > asserts not protected by that macro. > > > >gcc_assert (in_shutdown || ob); > >return (void *) ob; > > } > > I am sorry for that, I did not consider that my test machines all use > the fast path. > > I think the problem can be fixed by the trivial patch below, I will > commit that after I have tested builds both with and without fast path. > > Best > > Thomas > > > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c > index d237179f4ea..d6e347c5481 100644 > --- a/libgcc/unwind-dw2-fde.c > +++ b/libgcc/unwind-dw2-fde.c > @@ -67,6 +67,8 @@ static void > init_object (struct object *ob); > > #else > +/* Without fast path frame lookup must always succeed */ > The comment should end with full stop and two spaces. > +static const bool in_shutdown = false; > I'll let Jason or others decide if this is the right solution. It seems that in_shutdown also could be declared outside the #ifdef and initialized as "false". Thanks, David > > /* The unseen_objects list contains objects that have been registered > but not yet categorized in any way. The seen_objects list has had >
Re: [PATCH] Avoid depending on destructor order
This patch broke bootstrap on AIX and probably other targets. #ifdef ATOMIC_FDE_FAST_PATH #include "unwind-dw2-btree.h" static struct btree registered_frames; static bool in_shutdown; ... #else in_shutdown only is defined for ATOMIC_FDE_FAST_PATH but used in code / asserts not protected by that macro. gcc_assert (in_shutdown || ob); return (void *) ob; } I am sorry for that, I did not consider that my test machines all use the fast path. I think the problem can be fixed by the trivial patch below, I will commit that after I have tested builds both with and without fast path. Best Thomas diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c index d237179f4ea..d6e347c5481 100644 --- a/libgcc/unwind-dw2-fde.c +++ b/libgcc/unwind-dw2-fde.c @@ -67,6 +67,8 @@ static void init_object (struct object *ob); #else +/* Without fast path frame lookup must always succeed */ +static const bool in_shutdown = false; /* The unseen_objects list contains objects that have been registered but not yet categorized in any way. The seen_objects list has had
Re: [PATCH] Avoid depending on destructor order
On 9/19/22 12:20, Thomas Neumann wrote: In some scenarios (e.g., when mixing gcc and clang code), it can happen that frames are deregistered after the lookup structure has already been destroyed. That in itself would be fine, but it triggers an assert in __deregister_frame_info_bases that expects to find the frame. To avoid that, we now remember that the btree as already been destroyed and disable the assert in that case. OK. libgcc/ChangeLog: * unwind-dw2-fde.c: (release_register_frames) Remember when the btree has been destroyed. (__deregister_frame_info_bases) Disable the assert when shutting down. --- libgcc/unwind-dw2-fde.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c index 919abfe0664..d237179f4ea 100644 --- a/libgcc/unwind-dw2-fde.c +++ b/libgcc/unwind-dw2-fde.c @@ -48,6 +48,7 @@ typedef __UINTPTR_TYPE__ uintptr_type; #include "unwind-dw2-btree.h" static struct btree registered_frames; +static bool in_shutdown; static void release_registered_frames (void) __attribute__ ((destructor (110))); @@ -57,6 +58,7 @@ release_registered_frames (void) /* Release the b-tree and all frames. Frame releases that happen later are * silently ignored */ btree_destroy (_frames); + in_shutdown = true; } static void @@ -282,7 +284,7 @@ __deregister_frame_info_bases (const void *begin) __gthread_mutex_unlock (_mutex); #endif - gcc_assert (ob); + gcc_assert (in_shutdown || ob); return (void *) ob; }
[PATCH] Avoid depending on destructor order
In some scenarios (e.g., when mixing gcc and clang code), it can happen that frames are deregistered after the lookup structure has already been destroyed. That in itself would be fine, but it triggers an assert in __deregister_frame_info_bases that expects to find the frame. To avoid that, we now remember that the btree as already been destroyed and disable the assert in that case. libgcc/ChangeLog: * unwind-dw2-fde.c: (release_register_frames) Remember when the btree has been destroyed. (__deregister_frame_info_bases) Disable the assert when shutting down. --- libgcc/unwind-dw2-fde.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c index 919abfe0664..d237179f4ea 100644 --- a/libgcc/unwind-dw2-fde.c +++ b/libgcc/unwind-dw2-fde.c @@ -48,6 +48,7 @@ typedef __UINTPTR_TYPE__ uintptr_type; #include "unwind-dw2-btree.h" static struct btree registered_frames; +static bool in_shutdown; static void release_registered_frames (void) __attribute__ ((destructor (110))); @@ -57,6 +58,7 @@ release_registered_frames (void) /* Release the b-tree and all frames. Frame releases that happen later are * silently ignored */ btree_destroy (_frames); + in_shutdown = true; } static void @@ -282,7 +284,7 @@ __deregister_frame_info_bases (const void *begin) __gthread_mutex_unlock (_mutex); #endif - gcc_assert (ob); + gcc_assert (in_shutdown || ob); return (void *) ob; } -- 2.34.1
