From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f49.google.com (mail-wr1-f49.google.com [209.85.221.49]) by sourceware.org (Postfix) with ESMTPS id DDDE7385800F for ; Mon, 11 Jul 2022 18:40:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DDDE7385800F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f49.google.com with SMTP id h17so8205338wrx.0 for ; Mon, 11 Jul 2022 11:40:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=TagOM0LmJk3BPc1xwtBmj3W7rEjl/ZRybHBbbpkhZVk=; b=yBtIiv2Vy0qO5hclfveTB5m5JhFtrbqUR4kM2XeRw/TsKaSum1jGoaQruEwL6KyGbP GbRSnAolYC4jyKx2rL4+Ra6BwgNr4iBs8iWCd0cTkTzc6XO6m9DCoQSHswSoY2XDICOl Qph1LlSKBMcGox0hobIMNlRdz3wPkBn6qo5e6wvsOrMlKOLjtZw7kKEB1RMEXK5QNvP0 DTl1gzm/jWIwSPjpxYVaqIARyce9eitTb16pENM8mxkSwjayscmnn4/4tZdDfaE8+yS5 t0MN1zsYgKohDjkAFEWEu/hUBFAOZOTC8iC/pigtKdHxmeU/yOOZhjhlhvD7Wd3nA9ww p5lQ== X-Gm-Message-State: AJIora8hR1FXxU1IE5JJFjeh0GzmOeATuZhr+by8Blmcr5OrIR0NctcD 9OG7+DE3hnjL6Th9Alv+fVoW1FL2sVQ= X-Google-Smtp-Source: AGRyM1uSbRmJh01KXXyYbbysQ0OC1yYeTYdwCx5DI/vxhZ+AFmOvMx93ZtLveG7WkvsQjuVHavm7Og== X-Received: by 2002:a05:6000:1f0c:b0:21d:997b:53fc with SMTP id bv12-20020a0560001f0c00b0021d997b53fcmr12481303wrb.89.1657564816030; Mon, 11 Jul 2022 11:40:16 -0700 (PDT) Received: from ?IPv6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id j40-20020a05600c48a800b003a2e278510csm6541516wmp.15.2022.07.11.11.40.14 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 11 Jul 2022 11:40:14 -0700 (PDT) Subject: Re: [PATCH] [master & gdb 12] Fix core-file -> detach -> crash (corefiles/29275) From: Pedro Alves To: gdb-patches@sourceware.org References: <20220622192039.1627458-1-pedro@palves.net> Message-ID: Date: Mon, 11 Jul 2022 19:40:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220622192039.1627458-1-pedro@palves.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Jul 2022 18:40:28 -0000 I've merged this to both master and gdb 12 branch. On 2022-06-22 8:20 p.m., Pedro Alves wrote: > After loading a core file, you're supposed to be able to use "detach" > to unload the core file. That unfortunately regressed starting with > GDB 11, with these commits: > > 1192f124a308 - gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses > 408f66864a1a - detach in all-stop with threads running > > resulting in a GDB crash: > > ... > Thread 1 "gdb" received signal SIGSEGV, Segmentation fault. > 0x0000555555e842bf in maybe_set_commit_resumed_all_targets () at ../../src/gdb/infrun.c:2899 > 2899 if (proc_target->commit_resumed_state) > (top-gdb) bt > #0 0x0000555555e842bf in maybe_set_commit_resumed_all_targets () at ../../src/gdb/infrun.c:2899 > #1 0x0000555555e848bf in scoped_disable_commit_resumed::reset (this=0x7fffffffd440) at ../../src/gdb/infrun.c:3023 > #2 0x0000555555e84a0c in scoped_disable_commit_resumed::reset_and_commit (this=0x7fffffffd440) at ../../src/gdb/infrun.c:3049 > #3 0x0000555555e739cd in detach_command (args=0x0, from_tty=1) at ../../src/gdb/infcmd.c:2791 > #4 0x0000555555c0ba46 in do_simple_func (args=0x0, from_tty=1, c=0x55555662a600) at ../../src/gdb/cli/cli-decode.c:95 > #5 0x0000555555c112b0 in cmd_func (cmd=0x55555662a600, args=0x0, from_tty=1) at ../../src/gdb/cli/cli-decode.c:2514 > #6 0x0000555556173b1f in execute_command (p=0x5555565c5916 "", from_tty=1) at ../../src/gdb/top.c:699 > > The code that crashes looks like: > > static void > maybe_set_commit_resumed_all_targets () > { > scoped_restore_current_thread restore_thread; > > for (inferior *inf : all_non_exited_inferiors ()) > { > process_stratum_target *proc_target = inf->process_target (); > > if (proc_target->commit_resumed_state) > ^^^^^^^^^^^ > > With 'proc_target' above being null. all_non_exited_inferiors filters > out inferiors that have pid==0. We get here at the end of > detach_command, after core_target::detach has already run, at which > point the inferior _should_ have pid==0 and no process target. It is > clear it no longer has a process target, but, it still has a pid!=0 > somehow. > > The reason the inferior still has pid!=0, is that core_target::detach > just unpushes, and relies on core_target::close to actually do the > getting rid of the core and exiting the inferior. The problem with > that is that detach_command grabs an extra strong reference to the > process stratum target, so the unpush_target inside > core_target::detach doesn't actually result in a call to > core_target::close. > > Fix this my moving the cleaning up the core inferior to a shared > routine called by both core_target::close and core_target::detach. We > still need to cleanup the inferior from within core_file::close > because there are paths to it that want to get rid of the core without > going through detach. E.g., "core-file" -> "run". > > This commit includes a new test added to gdb.base/corefile.exp to > cover the "core-file core" -> "detach" scenario. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29275 > > Change-Id: Ic42bdd03182166b19f598428b0dbc2ce6f67c893 > --- > gdb/corelow.c | 27 +++++++++++++++++++++------ > gdb/testsuite/gdb.base/corefile.exp | 12 ++++++++++++ > 2 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/gdb/corelow.c b/gdb/corelow.c > index 8c33fb7ebb2..2037ef3dc89 100644 > --- a/gdb/corelow.c > +++ b/gdb/corelow.c > @@ -122,6 +122,9 @@ class core_target final : public process_stratum_target > > private: /* per-core data */ > > + /* Get rid of the core inferior. */ > + void clear_core (); > + > /* The core's section table. Note that these target sections are > *not* mapped in the current address spaces' set of target > sections --- those should come only from pure executable or > @@ -308,10 +311,8 @@ core_target::build_file_mappings () > /* An arbitrary identifier for the core inferior. */ > #define CORELOW_PID 1 > > -/* Close the core target. */ > - > void > -core_target::close () > +core_target::clear_core () > { > if (core_bfd) > { > @@ -325,6 +326,14 @@ core_target::close () > > current_program_space->cbfd.reset (nullptr); > } > +} > + > +/* Close the core target. */ > + > +void > +core_target::close () > +{ > + clear_core (); > > /* Core targets are heap-allocated (see core_target_open), so here > we delete ourselves. */ > @@ -631,9 +640,15 @@ core_target_open (const char *arg, int from_tty) > void > core_target::detach (inferior *inf, int from_tty) > { > - /* Note that 'this' is dangling after this call. unpush_target > - closes the target, and our close implementation deletes > - 'this'. */ > + /* Get rid of the core. Don't rely on core_target::close doing it, > + because target_detach may be called with core_target's refcount > 1, > + meaning core_target::close may not be called yet by the > + unpush_target call below. */ > + clear_core (); > + > + /* Note that 'this' may be dangling after this call. unpush_target > + closes the target if the refcount reaches 0, and our close > + implementation deletes 'this'. */ > inf->unpush_target (this); > > /* Clear the register cache and the frame cache. */ > diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp > index 4ed92a02955..7f3d2efe3a2 100644 > --- a/gdb/testsuite/gdb.base/corefile.exp > +++ b/gdb/testsuite/gdb.base/corefile.exp > @@ -207,6 +207,16 @@ gdb_test "up" "#\[0-9\]* *\[0-9xa-fH'\]* in .* \\(.*\\).*" "up in corefile.exp ( > > gdb_test "core" "No core file now." > > +# Test that we can unload the core with the "detach" command. > + > +proc_with_prefix corefile_detach {} { > + clean_restart $::binfile > + > + gdb_test "core-file $::corefile" "Core was generated by .*" "load core" > + gdb_test "detach" "No core file now\\." "detach core" > +} > + > +corefile_detach > > # Test a run (start) command will clear any loaded core file. > > @@ -222,6 +232,8 @@ proc corefile_test_run {} { > return > } > > + clean_restart $::binfile > + > gdb_test "core-file $corefile" "Core was generated by .*" "run: load core again" > gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "run: sanity check we see the core file" > > > base-commit: 90b7a5df152a64d2bea20beb438e8b81049a5c30 >