[PATCH] Guard against frame.c destructors running before frame-info.c's

[PATCH] Guard against frame.c destructors running before frame-info.c's

[Kévin Le Gouguec]

On x86_64-windows, since 04e2ac7b2a7, we observe this internal error:

  [...]/gdbsupport/intrusive_list.h:458: internal-error: erase_element:
  Assertion `elem_node->prev != INTRUSIVE_LIST_UNLINKED_VALUE' failed.

Breaking in the destructors for intrusive_list and frame_info_ptr shows that
in this configuration, the destructors for frame.c's statically-stored
objects are run before frame-info.c's:

  Thread 1 hit Breakpoint 7, intrusive_list<frame_info_ptr,
  intrusive_base_node<frame_info_ptr> >::~intrusive_list (this=0x7ff69c418c90
  <frame_info_ptr::frame_list>, __in_chrg=<optimized out>)
  [...]/../gdbsupport/intrusive_list.h:250
  250	    clear ();
  (gdb) bt
  #0  intrusive_list<frame_info_ptr, intrusive_base_node<frame_info_ptr> >
      ::~intrusive_list (this=0x7ff69c418c90 <frame_info_ptr::frame_list>,
      __in_chrg=<optimized out>) [...]/../gdbsupport/intrusive_list.h:250
  #1  0x00007ff69b78edba in __tcf_1 () [...]/frame-info.c:27
  #2  0x00007ff9c457aa9f in msvcrt!_initterm_e ()
      from C:\Windows\System32\msvcrt.dll
  #3  0x00007ff69b8246a6 in captured_main_1 (context=0x5ffe00)
      [...]/main.c:1111
  #4  0x00007ff69b825149 in captured_main (data=0x5ffe00) [...]/main.c:1320
  #5  0x00007ff69b8251b1 in gdb_main (args=0x5ffe00) [...]/main.c:1345
  #6  0x00007ff69b5d1730 in main (argc=2, argv=0x751730) [...]/gdb.c:32
  (gdb) c
  Continuing.

  Thread 1 hit Breakpoint 8, frame_info_ptr::~frame_info_ptr
  (this=0x7ff69c418e20 <selected_frame>, __in_chrg=<optimized out>)
  [...]/frame-info.h:74
  74	    if (is_linked ())
  (gdb) bt
  #0  frame_info_ptr::~frame_info_ptr (this=0x7ff69c418e20 <selected_frame>,
      __in_chrg=<optimized out>) [...]/frame-info.h:74
  #1  0x00007ff69b79a643 in __tcf_1 () [...]/frame.c:1675
  #2  0x00007ff9c457aa9f in msvcrt!_initterm_e () from
      C:\Windows\System32\msvcrt.dll
  #3  0x00007ff69b8246a6 in captured_main_1 (context=0x5ffe00)
      [...]/main.c:1111
  #4  0x00007ff69b825149 in captured_main (data=0x5ffe00) [...]/main.c:1320
  #5  0x00007ff69b8251b1 in gdb_main (args=0x5ffe00) [...]/main.c:1345
  #6  0x00007ff69b5d1730 in main (argc=2, argv=0x751730) [...]/gdb.c:32
---
 gdb/frame-info.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/frame-info.h b/gdb/frame-info.h
index 3369b114326..893b6632363 100644
--- a/gdb/frame-info.h
+++ b/gdb/frame-info.h
@@ -76,7 +76,11 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
 
   ~frame_info_ptr ()
   {
-    frame_list.erase (frame_list.iterator_to (*this));
+    /* If this node has static storage, it may be deleted after
+       frame_list.  Attempting to erase ourselves would then trigger
+       internal errors, so make sure we are still linked first.  */
+    if (is_linked ())
+      frame_list.erase (frame_list.iterator_to (*this));
   }
 
   frame_info_ptr &operator= (const frame_info_ptr &other)
-- 
2.25.1

Re: [PATCH] Guard against frame.c destructors running before frame-info.c's

[Simon Marchi]

On 11/15/22 15:48, Kévin Le Gouguec wrote:
> On x86_64-windows, since 04e2ac7b2a7, we observe this internal error:
> 
>   [...]/gdbsupport/intrusive_list.h:458: internal-error: erase_element:
>   Assertion `elem_node->prev != INTRUSIVE_LIST_UNLINKED_VALUE' failed.
> 
> Breaking in the destructors for intrusive_list and frame_info_ptr shows that
> in this configuration, the destructors for frame.c's statically-stored
> objects are run before frame-info.c's:
> 
>   Thread 1 hit Breakpoint 7, intrusive_list<frame_info_ptr,
>   intrusive_base_node<frame_info_ptr> >::~intrusive_list (this=0x7ff69c418c90
>   <frame_info_ptr::frame_list>, __in_chrg=<optimized out>)
>   [...]/../gdbsupport/intrusive_list.h:250
>   250	    clear ();
>   (gdb) bt
>   #0  intrusive_list<frame_info_ptr, intrusive_base_node<frame_info_ptr> >
>       ::~intrusive_list (this=0x7ff69c418c90 <frame_info_ptr::frame_list>,
>       __in_chrg=<optimized out>) [...]/../gdbsupport/intrusive_list.h:250
>   #1  0x00007ff69b78edba in __tcf_1 () [...]/frame-info.c:27
>   #2  0x00007ff9c457aa9f in msvcrt!_initterm_e ()
>       from C:\Windows\System32\msvcrt.dll
>   #3  0x00007ff69b8246a6 in captured_main_1 (context=0x5ffe00)
>       [...]/main.c:1111
>   #4  0x00007ff69b825149 in captured_main (data=0x5ffe00) [...]/main.c:1320
>   #5  0x00007ff69b8251b1 in gdb_main (args=0x5ffe00) [...]/main.c:1345
>   #6  0x00007ff69b5d1730 in main (argc=2, argv=0x751730) [...]/gdb.c:32
>   (gdb) c
>   Continuing.
> 
>   Thread 1 hit Breakpoint 8, frame_info_ptr::~frame_info_ptr
>   (this=0x7ff69c418e20 <selected_frame>, __in_chrg=<optimized out>)
>   [...]/frame-info.h:74
>   74	    if (is_linked ())
>   (gdb) bt
>   #0  frame_info_ptr::~frame_info_ptr (this=0x7ff69c418e20 <selected_frame>,
>       __in_chrg=<optimized out>) [...]/frame-info.h:74
>   #1  0x00007ff69b79a643 in __tcf_1 () [...]/frame.c:1675
>   #2  0x00007ff9c457aa9f in msvcrt!_initterm_e () from
>       C:\Windows\System32\msvcrt.dll
>   #3  0x00007ff69b8246a6 in captured_main_1 (context=0x5ffe00)
>       [...]/main.c:1111
>   #4  0x00007ff69b825149 in captured_main (data=0x5ffe00) [...]/main.c:1320
>   #5  0x00007ff69b8251b1 in gdb_main (args=0x5ffe00) [...]/main.c:1345
>   #6  0x00007ff69b5d1730 in main (argc=2, argv=0x751730) [...]/gdb.c:32
> ---
>  gdb/frame-info.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/frame-info.h b/gdb/frame-info.h
> index 3369b114326..893b6632363 100644
> --- a/gdb/frame-info.h
> +++ b/gdb/frame-info.h
> @@ -76,7 +76,11 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
>  
>    ~frame_info_ptr ()
>    {
> -    frame_list.erase (frame_list.iterator_to (*this));
> +    /* If this node has static storage, it may be deleted after
> +       frame_list.  Attempting to erase ourselves would then trigger
> +       internal errors, so make sure we are still linked first.  */
> +    if (is_linked ())
> +      frame_list.erase (frame_list.iterator_to (*this));
>    }
>  
>    frame_info_ptr &operator= (const frame_info_ptr &other)
> -- 
> 2.25.1
> 

Do you have a way to reproduce, so we can experiment with it?

Simon

Re: [PATCH] Guard against frame.c destructors running before frame-info.c's

[Kévin Le Gouguec]

Simon Marchi <simon.marchi@polymtl.ca> writes:

> Do you have a way to reproduce, so we can experiment with it?

On our Windows machines, IIRC the internal error was triggered reliably
on every GDB exit; "IIRC" because at some point I started testing just
by running --version (which is what this backtrace corresponds to).

Let me know if more information about this configuration would help, or
if there are more tests I should run.  FWIW that GDB was built with GCC
11.3.1 (+ a couple of AdaCore patches); --configuration says:

> This GDB was configured as follows:
>    configure --host=x86_64-w64-mingw32 --target=x86_64-w64-mingw32
> 	     --with-auto-load-dir=$debugdir:$datadir/auto-load
> 	     --with-auto-load-safe-path=$debugdir:$datadir/auto-load
> 	     --with-expat
> 	     --with-gdb-datadir=[…] (relocatable)
> 	     --with-jit-reader-dir=[…] (relocatable)
> 	     --without-libunwind-ia64
> 	     --with-lzma
> 	     --without-babeltrace
> 	     --without-intel-pt
> 	     --with-mpfr
> 	     --without-xxhash
> 	     --with-python=[…]
> 	     --with-python-libdir=[…]
> 	     --without-debuginfod
> 	     --without-guile
> 	     --disable-source-highlight
> 	     --enable-threading
> 	     --with-separate-debug-dir=[…]

Thanks.

Re: [PATCH] Guard against frame.c destructors running before frame-info.c's

[Tom Tromey]

Simon> Do you have a way to reproduce, so we can experiment with it?

Since it's sort of like a C++ static initializer problem, it's dependent
on how the linker happens to order the relevant destructors.

For me, the appended reproduces the problem on Linux, because it changes
the relative ordering of frame.o and frame-info.o in the link.

Tom

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index fb4d42c7baa..72527f4363f 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -2138,7 +2138,7 @@ stamp-init: $(INIT_FILES) config.status $(srcdir)/make-init-c
 # against that.
 #
 # init.o is very important.  It pulls in the rest of GDB.
-LIBGDB_OBS = $(sort $(COMMON_OBS)) init.o
+LIBGDB_OBS = $(COMMON_OBS) init.o
 libgdb.a: $(LIBGDB_OBS)
 	-rm -f libgdb.a
 	$(AR) q libgdb.a $(LIBGDB_OBS)

Re: [PATCH] Guard against frame.c destructors running before frame-info.c's

[Simon Marchi]

On 11/15/22 20:03, Tom Tromey wrote:
> Simon> Do you have a way to reproduce, so we can experiment with it?
> 
> Since it's sort of like a C++ static initializer problem, it's dependent
> on how the linker happens to order the relevant destructors.
> 
> For me, the appended reproduces the problem on Linux, because it changes
> the relative ordering of frame.o and frame-info.o in the link.
> 
> Tom
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index fb4d42c7baa..72527f4363f 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -2138,7 +2138,7 @@ stamp-init: $(INIT_FILES) config.status $(srcdir)/make-init-c
>  # against that.
>  #
>  # init.o is very important.  It pulls in the rest of GDB.
> -LIBGDB_OBS = $(sort $(COMMON_OBS)) init.o
> +LIBGDB_OBS = $(COMMON_OBS) init.o
>  libgdb.a: $(LIBGDB_OBS)
>  	-rm -f libgdb.a
>  	$(AR) q libgdb.a $(LIBGDB_OBS)

Thanks, this reproduces for me too.  The patch looks good to me:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH] Guard against frame.c destructors running before frame-info.c's

[Kévin Le Gouguec]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

Simon Marchi <simon.marchi@polymtl.ca> writes:

> Thanks, this reproduces for me too.  The patch looks good to me:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Thanks!  Trailer appended in the attached.

I think I'll need help to install this; it's my first patch for GDB
AFAIR, so I don't believe I have push rights yet.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Guard-against-frame.c-destructors-running-before-fra.patch --]
[-- Type: text/x-diff, Size: 3224 bytes --]

From af4c192fd2d598836aa208d88035aa0dab5aa89e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <legouguec@adacore.com>
Date: Tue, 15 Nov 2022 16:08:04 +0100
Subject: [PATCH] Guard against frame.c destructors running before
 frame-info.c's

On x86_64-windows, since 04e2ac7b2a7, we observe this internal error:

  [...]/gdbsupport/intrusive_list.h:458: internal-error: erase_element:
  Assertion `elem_node->prev != INTRUSIVE_LIST_UNLINKED_VALUE' failed.

Breaking in the destructors for intrusive_list and frame_info_ptr shows that
in this configuration, the destructors for frame.c's statically-stored
objects are run before frame-info.c's:

  Thread 1 hit Breakpoint 7, intrusive_list<frame_info_ptr,
  intrusive_base_node<frame_info_ptr> >::~intrusive_list (this=0x7ff69c418c90
  <frame_info_ptr::frame_list>, __in_chrg=<optimized out>)
  [...]/../gdbsupport/intrusive_list.h:250
  250	    clear ();
  (gdb) bt
  #0  intrusive_list<frame_info_ptr, intrusive_base_node<frame_info_ptr> >
      ::~intrusive_list (this=0x7ff69c418c90 <frame_info_ptr::frame_list>,
      __in_chrg=<optimized out>) [...]/../gdbsupport/intrusive_list.h:250
  #1  0x00007ff69b78edba in __tcf_1 () [...]/frame-info.c:27
  #2  0x00007ff9c457aa9f in msvcrt!_initterm_e ()
      from C:\Windows\System32\msvcrt.dll
  #3  0x00007ff69b8246a6 in captured_main_1 (context=0x5ffe00)
      [...]/main.c:1111
  #4  0x00007ff69b825149 in captured_main (data=0x5ffe00) [...]/main.c:1320
  #5  0x00007ff69b8251b1 in gdb_main (args=0x5ffe00) [...]/main.c:1345
  #6  0x00007ff69b5d1730 in main (argc=2, argv=0x751730) [...]/gdb.c:32
  (gdb) c
  Continuing.

  Thread 1 hit Breakpoint 8, frame_info_ptr::~frame_info_ptr
  (this=0x7ff69c418e20 <selected_frame>, __in_chrg=<optimized out>)
  [...]/frame-info.h:74
  74	    if (is_linked ())
  (gdb) bt
  #0  frame_info_ptr::~frame_info_ptr (this=0x7ff69c418e20 <selected_frame>,
      __in_chrg=<optimized out>) [...]/frame-info.h:74
  #1  0x00007ff69b79a643 in __tcf_1 () [...]/frame.c:1675
  #2  0x00007ff9c457aa9f in msvcrt!_initterm_e () from
      C:\Windows\System32\msvcrt.dll
  #3  0x00007ff69b8246a6 in captured_main_1 (context=0x5ffe00)
      [...]/main.c:1111
  #4  0x00007ff69b825149 in captured_main (data=0x5ffe00) [...]/main.c:1320
  #5  0x00007ff69b8251b1 in gdb_main (args=0x5ffe00) [...]/main.c:1345
  #6  0x00007ff69b5d1730 in main (argc=2, argv=0x751730) [...]/gdb.c:32

Approved-By: Simon Marchi <simon.marchi@efficios.com>
---
 gdb/frame-info.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/frame-info.h b/gdb/frame-info.h
index 3369b114326..893b6632363 100644
--- a/gdb/frame-info.h
+++ b/gdb/frame-info.h
@@ -76,7 +76,11 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
 
   ~frame_info_ptr ()
   {
-    frame_list.erase (frame_list.iterator_to (*this));
+    /* If this node has static storage, it may be deleted after
+       frame_list.  Attempting to erase ourselves would then trigger
+       internal errors, so make sure we are still linked first.  */
+    if (is_linked ())
+      frame_list.erase (frame_list.iterator_to (*this));
   }
 
   frame_info_ptr &operator= (const frame_info_ptr &other)
-- 
2.25.1

Re: [PATCH] Guard against frame.c destructors running before frame-info.c's

[Simon Marchi]

On 11/16/22 02:05, Kévin Le Gouguec wrote:
> Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>> Thanks, this reproduces for me too.  The patch looks good to me:
>>
>> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> Thanks!  Trailer appended in the attached.
> 
> I think I'll need help to install this; it's my first patch for GDB
> AFAIR, so I don't believe I have push rights yet.
> 

Do you plan on contributing further and regularly?  If so we'll get you
an account on Sourceware.  Otherwise I can push this patch for you.

Simon

Re: [PATCH] Guard against frame.c destructors running before frame-info.c's

[Kévin Le Gouguec]

Simon Marchi <simon.marchi@polymtl.ca> writes:

> Do you plan on contributing further and regularly?

Yep.

>                                                     If so we'll get you
> an account on Sourceware.  Otherwise I can push this patch for you.

Thanks; Joel invited me to fill in the sourceware Account Request Form,
so I did just now.

If that's OK, I plan on posting the patch to add myself to
gdb/MAINTAINERS under "Write After Approval" shortly, and on pushing
both patches myself when the account is set up.

Re: [PATCH] Guard against frame.c destructors running before frame-info.c's

[Simon Marchi]

On 11/16/22 11:25, Kévin Le Gouguec wrote:
> Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>> Do you plan on contributing further and regularly?
> 
> Yep.
> 
>>                                                     If so we'll get you
>> an account on Sourceware.  Otherwise I can push this patch for you.
> 
> Thanks; Joel invited me to fill in the sourceware Account Request Form,
> so I did just now.
> 
> If that's OK, I plan on posting the patch to add myself to
> gdb/MAINTAINERS under "Write After Approval" shortly, and on pushing
> both patches myself when the account is set up.

Perfect, thanks, that's the right process.

Simon