Message ID | 20221115204821.1260141-1-legouguec@adacore.com |
---|---|
State | Committed |
Commit | 995a34b1772f1c04d6a98641c6d29d68628b9063 |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 49287393BA4A for <patchwork@sourceware.org>; Tue, 15 Nov 2022 20:50:16 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 49287393BA4A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1668545416; bh=7ECA3WqypXdijlMoUFL8Ht6MIYhIvVuVrvqF0nHVJYg=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=MGLD/QOn4wZKccz+Ox17V45M8GkZWQYr2DAxFK9JAzXwwPZR+Pveh40066xxX7yCU oaCiUlC2do9uDsiffFtwt+unhjD+/oD1t2128lZFAh6hPoI1U+wUxN0MmErb2wKfp9 rZPbNfckQ7G2ocWcAT6RvOlA0aO8E7A6KbfsbJJU= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-wm1-x329.google.com (mail-wm1-x329.google.com [IPv6:2a00:1450:4864:20::329]) by sourceware.org (Postfix) with ESMTPS id 48CE9392AC37 for <gdb-patches@sourceware.org>; Tue, 15 Nov 2022 20:49:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 48CE9392AC37 Received: by mail-wm1-x329.google.com with SMTP id fn7-20020a05600c688700b003b4fb113b86so27869wmb.0 for <gdb-patches@sourceware.org>; Tue, 15 Nov 2022 12:49:50 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=7ECA3WqypXdijlMoUFL8Ht6MIYhIvVuVrvqF0nHVJYg=; b=IMEB01Ib6hBf/dA+MPiePWP4QmEvX0ostIAA8UfRVhRYv095DT1UnFiZ8UmSt/d/vM H0W/54rYSrNAuSyZf8ZysL3sZE070+8r/GGzKRiYPHBl6X4vKaCNBVhfNMfP+st7875c z11IFwVdhTXrLQ2yKeKTYHkquSP/XLB7FET9KuUwH9XKWr3yfMl+mHta2sBPDZTuaib+ zHKDjl/eYsQxHYEwMd9t1SJXd4eFHmwMrj0EeQvsJIj+svTJgAnTyrKESz7a8zJ0uCtV aLRfVyqLkDyCt4QRwtjmhvPjG9wp8aMpAUMW2/+Hql+xabl7Ql2TX6ntUtJJPrc7K+3K btDg== X-Gm-Message-State: ANoB5pkQmmPMCAtsCYmLuNFWnHaOQwxfchI2RSShKc9bN7m96MZar0Mz Prz5gUGvJ2Sb6knimoLOEBWwATAxRKp2qA== X-Google-Smtp-Source: AA0mqf7SU6v3qYfw0j6UGSvZ8w21FL/9QON6MdeXxjElolK0m3M8SegK15OmlVNAUquPis/q85waYA== X-Received: by 2002:a1c:cc07:0:b0:3cf:5b8a:a7cd with SMTP id h7-20020a1ccc07000000b003cf5b8aa7cdmr51751wmb.136.1668545388517; Tue, 15 Nov 2022 12:49:48 -0800 (PST) Received: from localhost.localdomain ([2a01:e0a:253:fe0:8cac:2ed7:853b:7ff9]) by smtp.gmail.com with ESMTPSA id t5-20020adfe445000000b00236740c6e6fsm13204197wrm.100.2022.11.15.12.49.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 15 Nov 2022 12:49:47 -0800 (PST) To: gdb-patches@sourceware.org Cc: simon.marchi@polymtl.ca, =?utf-8?q?K=C3=A9vin_Le_Gouguec?= <legouguec@adacore.com> Subject: [PATCH] Guard against frame.c destructors running before frame-info.c's Date: Tue, 15 Nov 2022 21:48:21 +0100 Message-Id: <20221115204821.1260141-1-legouguec@adacore.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: =?utf-8?q?K=C3=A9vin_Le_Gouguec_via_Gdb-patches?= <gdb-patches@sourceware.org> Reply-To: =?utf-8?q?K=C3=A9vin_Le_Gouguec?= <legouguec@adacore.com> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
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
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
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.
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)
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
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.
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
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.
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 --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)