diff mbox series

Avoid depending on destructor order

Message ID 5aba5553-ed71-50a8-e934-6e94f3a1057e@in.tum.de
State New, archived
Headers
Series Avoid depending on destructor order |

Commit Message

Thomas Neumann Sept. 19, 2022, 4:20 p.m. UTC 
  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(-)

  static void
@@ -282,7 +284,7 @@ __deregister_frame_info_bases (const void *begin)
    __gthread_mutex_unlock (&object_mutex);
  #endif

-  gcc_assert (ob);
+  gcc_assert (in_shutdown || ob);
    return (void *) ob;
  }

Comments

Jason Merrill Sept. 22, 2022, 10:22 p.m. UTC | #1 
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 (&registered_frames);
> +  in_shutdown = true;
>   }
> 
>   static void
> @@ -282,7 +284,7 @@ __deregister_frame_info_bases (const void *begin)
>     __gthread_mutex_unlock (&object_mutex);
>   #endif
> 
> -  gcc_assert (ob);
> +  gcc_assert (in_shutdown || ob);
>     return (void *) ob;
>   }
>
Claudiu Zissulescu Ianculescu Sept. 26, 2022, 11:46 a.m. UTC | #2 
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
Thomas Neumann Sept. 26, 2022, 11:49 a.m. UTC | #3 
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
Claudiu Zissulescu Ianculescu Sept. 26, 2022, 11:50 a.m. UTC | #4 
Thanks, I haven't observed it.

Waiting for it,
Claudiu

On Mon, Sep 26, 2022 at 2:49 PM Thomas Neumann <neumann@in.tum.de> 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
Iain Sandoe Sept. 26, 2022, 12:53 p.m. UTC | #5 
> On 26 Sep 2022, at 12:49, Thomas Neumann via Gcc-patches <gcc-patches@gcc.gnu.org> 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
Thomas Neumann Sept. 26, 2022, 1:14 p.m. UTC | #6 
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
diff mbox series

Patch

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 (&registered_frames);
+  in_shutdown = true;
  }