diff mbox series

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

Message ID 20221115204821.1260141-1-legouguec@adacore.com
State Committed
Commit 995a34b1772f1c04d6a98641c6d29d68628b9063
Headers
Series Guard against frame.c destructors running before frame-info.c's |

Commit Message

Kévin Le Gouguec Nov. 15, 2022, 8:48 p.m. UTC 
  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(-)

Comments

Simon Marchi Nov. 15, 2022, 9:57 p.m. UTC | #1 
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
Kévin Le Gouguec Nov. 15, 2022, 10:32 p.m. UTC | #2 
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.
Tom Tromey Nov. 16, 2022, 1:03 a.m. UTC | #3 
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)
Simon Marchi Nov. 16, 2022, 1:19 a.m. UTC | #4 
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
Kévin Le Gouguec Nov. 16, 2022, 7:05 a.m. UTC | #5 
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.
Simon Marchi Nov. 16, 2022, 2:39 p.m. UTC | #6 
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
Kévin Le Gouguec Nov. 16, 2022, 4:25 p.m. UTC | #7 
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.
Simon Marchi Nov. 16, 2022, 4:28 p.m. UTC | #8 
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
diff mbox series

Patch

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)