From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 7CF443858C62 for ; Mon, 28 Nov 2022 12:11:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7CF443858C62 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669637468; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=VyI+aBEopb6a6Nfd7d3W4BxRjeqlI0C/Gq++RPGnUhA=; b=WIVSGxI3AFjykDr7/CB1t4xSZiJYhQIQLLKpTgyTJYigL+gQ7sQIWnGVRfWxrOEsAMVxqt EwvShiDjaiIhGZZVV8ht2MMaSOPVzQfXmcZp9z5jRsLKlzOX2lv+iyybkrDeViJimDwCxD aTxZspEjWjLoXRUwmzhaLJ3FVFZ0P5c= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-16-0b8jP5shNh6u5Bi_gy9SfQ-1; Mon, 28 Nov 2022 07:11:07 -0500 X-MC-Unique: 0b8jP5shNh6u5Bi_gy9SfQ-1 Received: by mail-wr1-f72.google.com with SMTP id d4-20020adfa404000000b002421ca8cb07so366425wra.2 for ; Mon, 28 Nov 2022 04:11:06 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=VyI+aBEopb6a6Nfd7d3W4BxRjeqlI0C/Gq++RPGnUhA=; b=AHaatG6apPvrc8Gifu8DhyUR5mRPUZ+A3bdBBcumWHoqiYbS8SOT9kPx/FFhmeCDgu VZCO156F2lP78eGQ4GjlCs2Nk+CVMg6WpMeORQVPup5pjjvPEBw08YJVgwmoDwv/RD9e I7nw5z6F9W5Dy3osujvfPaPpcZurfnvBHPFU2047WbBna0NxXh1D9EuLO7nmO3MA/dvP 0PrjH0Wo/4fF6TmVPzXBYcvmfZazkJsFQeNzht5W8OL68274r3siLUmHCYJilsTe7Phb pQBZLrEbp5NYJzbwkkwDiiqTFQmenwItX4DNgYvlx6D3kDkSKC/DvOqs0o1+wFWMi8eP PXuQ== X-Gm-Message-State: ANoB5plsbw2ebT1S7NZqe3V+5B65OTaOI+IjIkgYkHq6mQmDU1iH1CIw hrmE/NtL8xfLznJHo36kQZB3PWOO7aLNRMKkwzbneyKzFzvbsYNMesLROD6cWQ4h3JNz6vmffld VAioJ01VtbCXcSR9oZmETXQ== X-Received: by 2002:adf:e50f:0:b0:22c:cc75:5aab with SMTP id j15-20020adfe50f000000b0022ccc755aabmr29987712wrm.143.1669637465684; Mon, 28 Nov 2022 04:11:05 -0800 (PST) X-Google-Smtp-Source: AA0mqf5e+0FrKysPB9gsEbDEIr9iQF3ccrP0cOHmQpuZHcWvI0ypSs6Zi3PKggqMJgb+uLYF+9SdJw== X-Received: by 2002:adf:e50f:0:b0:22c:cc75:5aab with SMTP id j15-20020adfe50f000000b0022ccc755aabmr29987699wrm.143.1669637465441; Mon, 28 Nov 2022 04:11:05 -0800 (PST) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id h12-20020a056000000c00b00241cfa9333fsm11049610wrx.5.2022.11.28.04.11.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Nov 2022 04:11:04 -0800 (PST) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Cc: Simon Marchi Subject: Re: [PATCH v2 3/5] gdb: fix assert when quitting GDB while a thread is stepping In-Reply-To: <20221121171213.1414366-4-simon.marchi@polymtl.ca> References: <20221121171213.1414366-1-simon.marchi@polymtl.ca> <20221121171213.1414366-4-simon.marchi@polymtl.ca> Date: Mon, 28 Nov 2022 12:11:03 +0000 Message-ID: <87fse32sa0.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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 List-Id: Simon Marchi writes: > From: Andrew Burgess > > This commit addresses one of the issues identified in PR gdb/28275. > > Bug gdb/28275 identifies a number of situations in which this assert: > > Assertion `!proc_target->commit_resumed_state' failed. > > could be triggered. There's actually a number of similar places where > this assert is found in GDB, the two of interest in gdb/28275 are in > target_wait and target_stop. > > In one of the comments: > > https://sourceware.org/bugzilla/show_bug.cgi?id=28275#c1 > > steps to trigger the assertion within target_stop were identified when > using a modified version of the gdb.threads/detach-step-over.exp test > script. > > In the gdb.threads/detach-step-over.exp test, we attach to a > multi-threaded inferior, and continue the inferior in asynchronous > (background) mode. Each thread is continuously hitting a conditional > breakpoint where the condition is always false. While the inferior is > running we detach. The goal is that we detach while GDB is performing a > step-over for the conditional breakpoint in at least one thread. > > While detaching, if a step-over is in progress, then GDB has to > complete the step over before we can detach. This involves calling > target_stop and target_wait (see prepare_for_detach). > > As far as gdb/28275 is concerned, the interesting part here, is the > the process_stratum_target::commit_resumed_state variable must be > false when target_stop and target_wait are called. > > This is currently ensured because, in detach_command (infrun.c), we > create an instance of scoped_disable_commit_resumed, this ensures that > when target_detach is called, ::commit_resumed_state will be false. > > The modification to the test that I propose here, and which exposed > the bug, is that, instead of using "detach" to detach from the > inferior, we instead use "quit". Quitting GDB after attaching to an > inferior will cause GDB to first detach, and then exit. > > When we quit GDB we end up calling target_detach via a different code > path, the stack looks like: > > #0 target_detach > #1 kill_or_detach > #2 quit_force > #3 quit_command > > Along this path there is no scoped_disable_commit_resumed created. > ::commit_resumed_state can be true when we reach prepare_for_detach, > which calls target_wait and target_stop, so the assertion will trigger. > > In this commit, I propose fixing this by adding the creation of a > scoped_disable_commit_resumed into target_detach. This will ensure > that ::commit_resumed_state is false when calling prepare_for_detach > from within target_detach. > > I did consider placing the scoped_disable_commit_resumed in > prepare_for_detach, however, placing it in target_detach ensures that > the target's commit_resumed_state flag is left to false if the detached > inferior was the last one for that target. It's the same rationale as > for patch "gdb: disable commit resumed in target_kill" that comes later > in this series, but for detach instead of kill. > > detach_command still includes a scoped_disable_commit_resumed too, but I > think it is still relevant to cover the resumption at the end of the > function. > > Co-Authored-By: Simon Marchi > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28275 > Change-Id: Ie128f7aba6ef0e018859275eca372e6ea738e96f LGTM. Thanks for taking this into your series. Thanks, Andrew > --- > gdb/target.c | 5 ++ > .../gdb.threads/detach-step-over.exp | 52 +++++++++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/gdb/target.c b/gdb/target.c > index 74925e139dc1..4a22885b82f1 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -2535,6 +2535,9 @@ target_preopen (int from_tty) > void > target_detach (inferior *inf, int from_tty) > { > + /* Thread's don't need to be resumed until the end of this function. */ > + scoped_disable_commit_resumed disable_commit_resumed ("detaching"); > + > /* After we have detached, we will clear the register cache for this inferior > by calling registers_changed_ptid. We must save the pid_ptid before > detaching, as the target detach method will clear inf->pid. */ > @@ -2565,6 +2568,8 @@ target_detach (inferior *inf, int from_tty) > inferior_ptid matches save_pid_ptid, but in our case, it does not > call it, as inferior_ptid has been reset. */ > reinit_frame_cache (); > + > + disable_commit_resumed.reset_and_commit (); > } > > void > diff --git a/gdb/testsuite/gdb.threads/detach-step-over.exp b/gdb/testsuite/gdb.threads/detach-step-over.exp > index ad9b08f549ea..d2cb52423d98 100644 > --- a/gdb/testsuite/gdb.threads/detach-step-over.exp > +++ b/gdb/testsuite/gdb.threads/detach-step-over.exp > @@ -284,6 +284,56 @@ proc_with_prefix test_detach_command {condition_eval target_non_stop non_stop di > kill_wait_spawned_process $test_spawn_id > } > > +# Similar to the proc above, but this time, instead of detaching using > +# the 'detach' command, we quit GDB, this will also trigger a detach, but > +# through a slightly different path, which can expose different bugs. > +proc_with_prefix test_detach_quit {condition_eval target_non_stop \ > + non_stop displaced} { > + # If debugging with target remote, check whether the all-stop variant > + # of the RSP is being used. If so, we can't run the background tests. > + if {!$non_stop > + && [target_info exists gdb_protocol] > + && ([target_info gdb_protocol] == "remote" > + || [target_info gdb_protocol] == "extended-remote")} { > + start_gdb_for_test $condition_eval $target_non_stop \ > + $non_stop $displaced > + > + gdb_test_multiple "maint show target-non-stop" "" { > + -wrap -re "(is|currently) on.*" { > + } > + -wrap -re "(is|currently) off.*" { > + return > + } > + } > + } > + > + set test_spawn_id [spawn_wait_for_attach $::binfile] > + set testpid [spawn_id_get_pid $test_spawn_id] > + > + set attempts 3 > + for {set attempt 1} { $attempt <= $attempts } { incr attempt } { > + with_test_prefix "iter $attempt" { > + > + start_gdb_for_test $condition_eval $target_non_stop \ > + $non_stop $displaced > + > + if {![prepare_test_iter $testpid $non_stop \ > + $attempt $attempts "$::decimal"]} { > + kill_wait_spawned_process $test_spawn_id > + return > + } > + > + gdb_test_multiple "with confirm off -- quit" "" { > + eof { > + pass $gdb_test_name > + } > + } > + } > + } > + > + kill_wait_spawned_process $test_spawn_id > +} > + > # The test program exits after a while, in case GDB crashes. Make it > # wait at least as long as we may wait before declaring a time out > # failure. > @@ -331,6 +381,8 @@ foreach_with_prefix breakpoint-condition-evaluation {"host" "target"} { > foreach_with_prefix displaced {"off" "auto"} { > test_detach_command ${breakpoint-condition-evaluation} \ > ${target-non-stop} ${non-stop} ${displaced} > + test_detach_quit ${breakpoint-condition-evaluation} \ > + ${target-non-stop} ${non-stop} ${displaced} > } > } > } > -- > 2.38.1