From: "Paunovic, Aleksandar via Gdb-patches" <gdb-patches@sourceware.org>
To: Andrew Burgess <andrew.burgess@embecosm.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [PATCH 2/2] gdb: Improve the resuming of the stepped thread
Date: Mon, 14 Jun 2021 10:47:49 +0000 [thread overview]
Message-ID: <DM6PR11MB25889C3D6F45819B7D86EAE2F0319@DM6PR11MB2588.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210607134818.GT2672@embecosm.com>
>> From: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
>>
>> Stepped thread should not be resumed (again) if the next stopping
>> point (next stopping PC) cannot be determined. Do not resume the
>> stepped thread in this case. Resuming would lead to an inconsistent
>> state where the stepped thread would be running forever and GDB would
>> never get to breakpoints of the other threads. */
>>
>> gdb/ChangeLog:
>> 2021-04-30 Aleksandar Paunovic <aleksandar.paunovic@intel.com>
>>
>> * infrun.c (keep_going_stepped_thread): Do not resume an
>> already executing thread.
>>
>> gdb/testsuite/ChangeLog:
>> 2021-04-30 Aleksandar Paunovic <aleksandar.paunovic@intel.com>
>>
>> * gdb.base/breakpoint-running-inferior.exp: Add a start_step
>> function check.
>>
>> 2021-06-07 Aleksandar Paunovic <aleksandar.paunovic@intel.com>
>> ---
>> gdb/infrun.c | 15 ++++++++++++++-
>> .../gdb.base/breakpoint-running-inferior.exp | 5 +++++
>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c index 78da1f0e72..459a679ee6
>> 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -7529,7 +7529,7 @@ restart_after_all_stop_detach
>> (process_stratum_target *proc_target)
>>
>> /* Set a previously stepped thread back to stepping. Returns true on
>> success, false if the resume is not possible (e.g., the thread
>> - vanished). */
>> + vanished, or the thread breakpoint position cannot be determined).
>> + */
>>
>> static bool
>> keep_going_stepped_thread (struct thread_info *tp) @@ -7566,6
>> +7566,19 @@ keep_going_stepped_thread (struct thread_info *tp)
>> delete_thread (tp);
>> return false;
>> }
>> + /* Step start function determines the location of the next stopping
>> + point (next PC where the stepped thread should stop). In case that
>> + this position cannot be determined the step_start_function will not
>> + be set. Do not resume the stepped thread in this case. Resuming
>> + would lead to an inconsistent state where the stepped thread would be
>> + running forever and GDB would never get to breakpoints of the other
>> + threads. */
>> + if (tp->control.step_start_function == nullptr)
>> + {
>> + infrun_debug_printf ("not resuming previously stepped thread, it is "
>> + "already executing");
>> + return 0;
>
>I don't understand why the condition you're checking means that the thread is already executing.
>
>Also, your comment says step_start_function is the pc where we should next stop, but it is set with find_pc_function, which just gives the symbol for the function containing the $pc when the step/next was started, so I don't understand why this tells GDB where to stop.
>
>Also 'return false'.
>
>Thanks,
>Andrew
Actually now when I tested this further I saw that this check (the whole patch) is unnecessary because GDB recovers successfully.
Originally the regcache_read_pc function call (10 lines below my change) was throwing "PC register is not available" but it seems in the meantime this is fixed.
I will put only first patch for review.
>> + }
>>
>> infrun_debug_printf ("resuming previously stepped thread");
>>
>> diff --git a/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp
>> b/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp
>> index bb0406bc65..b95f2ec012 100644
>> --- a/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp
>> +++ b/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp
>> @@ -74,10 +74,15 @@ gdb_test "next" ".*Thread 2.*hit Breakpoint.*" "next while in inferior 1"
>>
>> # We should be able to normally switch to thread 1.1.
>> # In case of a bad GDB flow the GDB was losing the thread.
>> +# The thread should also not be in a "running" state because it is #
>> +stopped.
>> gdb_test_multiple "thread 1.1" "Switching to thread 1.1" {
>> -re "\\\[Switching to thread 1.1 \\(Thread .*\\)\\\]" {
>> pass $gdb_test_name
>> }
>> + -re "\\\[Switching to thread 1.1.*\\(running\\)" {
>> + fail $gdb_test_name
>> + }
>> -re ".*Thread ID 1.1 has terminated.*" {
>> fail $gdb_test_name
>> }
>> --
>> 2.17.1
>>
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
next prev parent reply other threads:[~2021-06-14 10:48 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-07 8:42 [PATCH 0/2] Fix next command in the running inferior aleksandar.paunovic via Gdb-patches
2021-06-07 8:42 ` [PATCH 1/2] gdb: Fix deleted thread when issuing next command aleksandar.paunovic via Gdb-patches
2021-06-07 8:42 ` [PATCH 2/2] gdb: Improve the resuming of the stepped thread aleksandar.paunovic via Gdb-patches
2021-06-07 13:48 ` Andrew Burgess
2021-06-14 10:47 ` Paunovic, Aleksandar via Gdb-patches [this message]
2022-05-23 18:00 [PATCH 0/2] Some patches for multi inferior case Eduard Sargsyan via Gdb-patches
2022-05-23 18:00 ` [PATCH 2/2] gdb: Improve the resuming of the stepped thread Eduard Sargsyan via Gdb-patches
2022-08-25 23:20 ` Thiago Jung Bauermann via Gdb-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM6PR11MB25889C3D6F45819B7D86EAE2F0319@DM6PR11MB2588.namprd11.prod.outlook.com \
--to=gdb-patches@sourceware.org \
--cc=aleksandar.paunovic@intel.com \
--cc=andrew.burgess@embecosm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox