[2/2] gdb: testsuite: Test whether PC register is expedited in gdb.server/server-run.exp

One thing GDB always does when the inferior stops is finding out where
it's stopped at, by way of querying the value of the program counter
register.

To save a packet round trip, the remote target can send the PC
value (often alongside other frequently consulted registers such as the
stack pointer) in the stop reply packet as an "expedited register".

Test that this is actually done for the targets where gdbserver is
supposed to.

Extend the "maintenance print remote-registers" command output with an
"Expedited" column which says "yes" if the register was seen by GDB in
the last stop reply packet it received, and is left blank otherwise.

Tested for regressions on aarch64-linux-gnu native-extended-remote.

The testcase was tested on aarch64-linux-gnu, i686-linux-gnu and
x86_64-linux-gnu native-remote and native-extended-remote targets.
---
 gdb/regcache-dump.c                     |  9 +++++--
 gdb/remote.c                            | 27 ++++++++++++++++++++-
 gdb/remote.h                            |  5 ++++
 gdb/testsuite/gdb.server/server-run.exp | 31 +++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 3 deletions(-)

Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

> One thing GDB always does when the inferior stops is finding out where
> it's stopped at, by way of querying the value of the program counter
> register.
>
> To save a packet round trip, the remote target can send the PC
> value (often alongside other frequently consulted registers such as the
> stack pointer) in the stop reply packet as an "expedited register".
>
> Test that this is actually done for the targets where gdbserver is
> supposed to.
>
> Extend the "maintenance print remote-registers" command output with an
> "Expedited" column which says "yes" if the register was seen by GDB in
> the last stop reply packet it received, and is left blank otherwise.

I think this is a good idea.

>
> Tested for regressions on aarch64-linux-gnu native-extended-remote.
>
> The testcase was tested on aarch64-linux-gnu, i686-linux-gnu and
> x86_64-linux-gnu native-remote and native-extended-remote targets.
> ---
>  gdb/regcache-dump.c                     |  9 +++++--
>  gdb/remote.c                            | 27 ++++++++++++++++++++-
>  gdb/remote.h                            |  5 ++++
>  gdb/testsuite/gdb.server/server-run.exp | 31 +++++++++++++++++++++++++

I think this should have a doc/ entry -- we already document the 'maint
print remote-registers' command, so that should be expanded.

I think it's important to document that this column only represents the
last stop packet, and could potentially change from one stop to the
next.

I also think this should get a NEWS entry.  We add NEWS entries when new
maintenance commands are added, so I don't see why we shouldn't document
changes to the same.

>  4 files changed, 69 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/regcache-dump.c b/gdb/regcache-dump.c
> index bc665dc08a67..1dfc881969eb 100644
> --- a/gdb/regcache-dump.c
> +++ b/gdb/regcache-dump.c
> @@ -162,7 +162,7 @@ class register_dump_remote : public register_dump
>    {
>      if (regnum < 0)
>        {
> -	gdb_printf (file, "Rmt Nr  g/G Offset");
> +	gdb_printf (file, "Rmt Nr  g/G Offset  Expedited");
>        }
>      else if (regnum < gdbarch_num_regs (m_gdbarch))
>        {
> @@ -170,7 +170,12 @@ class register_dump_remote : public register_dump
>  
>  	if (remote_register_number_and_offset (m_gdbarch, regnum,
>  					       &pnum, &poffset))
> -	  gdb_printf (file, "%7d %11d", pnum, poffset);
> +	  {
> +	    if (remote_register_is_expedited (regnum))
> +	      gdb_printf (file, "%7d %11d  yes", pnum, poffset);
> +	    else
> +	      gdb_printf (file, "%7d %11d", pnum, poffset);
> +	  }
>        }
>    }
>  };
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 2c3988cb5075..c17572d51c8d 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -693,6 +693,10 @@ class remote_state
>       qSupported.  */
>    gdb_thread_options supported_thread_options = 0;
>  
> +  /* Contains the regnums of the expedited registers in the last stop
> +     reply packet.  */
> +  std::set<int> last_seen_expedited_registers;
> +
>  private:
>    /* Asynchronous signal handle registered as event loop source for
>       when we have pending events ready to be passed to the core.  */
> @@ -1490,6 +1494,20 @@ is_remote_target (process_stratum_target *target)
>    return as_remote_target (target) != nullptr;
>  }
>  
> +/* See remote.h.  */
> +
> +bool
> +remote_register_is_expedited (int regnum)
> +{
> +  remote_target *rt = as_remote_target (current_inferior ()->process_target ());
> +
> +  if (rt == nullptr)
> +    return false;
> +
> +  remote_state *rs = rt->get_remote_state ();
> +  return rs->last_seen_expedited_registers.count (regnum) > 0;
> +}
> +
>  /* Per-program-space data key.  */
>  static const registry<program_space>::key<char, gdb::xfree_deleter<char>>
>    remote_pspace_data;
> @@ -8518,6 +8536,10 @@ remote_target::process_stop_reply (stop_reply_up stop_reply,
>  {
>    *status = stop_reply->ws;
>    ptid_t ptid = stop_reply->ptid;
> +  struct remote_state *rs = get_remote_state ();
> +
> +  /* Forget about last reply's expedited registers.  */
> +  rs->last_seen_expedited_registers.clear ();
>  
>    /* If no thread/process was reported by the stub then select a suitable
>       thread/process.  */
> @@ -8544,7 +8566,10 @@ remote_target::process_stop_reply (stop_reply_up stop_reply,
>  					stop_reply->arch);
>  
>  	  for (cached_reg_t &reg : stop_reply->regcache)
> -	    regcache->raw_supply (reg.num, reg.data.get ());
> +	    {
> +	      regcache->raw_supply (reg.num, reg.data.get ());
> +	      rs->last_seen_expedited_registers.insert (reg.num);
> +	    }
>  	}
>  
>        remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);
> diff --git a/gdb/remote.h b/gdb/remote.h
> index cb0a66da66e5..bfe3c65b4637 100644
> --- a/gdb/remote.h
> +++ b/gdb/remote.h
> @@ -121,4 +121,9 @@ extern void send_remote_packet (gdb::array_view<const char> &buf,
>  
>  extern bool is_remote_target (process_stratum_target *target);
>  
> +/* Return true if REGNUM was returned as an expedited register in the last
> +   stop reply we received.  */
> +
> +extern bool remote_register_is_expedited (int regnum);
> +
>  #endif
> diff --git a/gdb/testsuite/gdb.server/server-run.exp b/gdb/testsuite/gdb.server/server-run.exp
> index 92eb38bd9db0..e22cee1e3c74 100644
> --- a/gdb/testsuite/gdb.server/server-run.exp
> +++ b/gdb/testsuite/gdb.server/server-run.exp
> @@ -52,3 +52,34 @@ if { [istarget *-*-linux*] } {
>  
>  gdb_breakpoint main
>  gdb_test "continue" "Breakpoint.* main .*" "continue to main"
> +
> +if { [istarget "aarch64*-*-*"]
> +     || [istarget "arm*-*-*"]
> +     || [istarget "csky*-*-*"]
> +     || [istarget "loongarch*-*-*"]
> +     || [istarget "riscv*-*-*"]} {
> +    set pc_regname "pc"
> +} elseif { [is_amd64_regs_target] } {
> +    set pc_regname "rip"
> +} elseif { [is_x86_like_target] } {
> +    set pc_regname "eip"
> +} elseif { [istarget "tic6x-*-*"] } {
> +    set pc_regname "PC"
> +}
> +
> +# Sending the PC register in advance is good practice.  Test that this is
> +# actually done for the targets where gdbserver is supposed to.
> +set expedited_pc_test_name "send PC as expedited register in stop reply"
> +if { [info exists pc_regname] } {
> +    gdb_test_multiple "maintenance print remote-registers" \
> +		      $expedited_pc_test_name -lbl {
> +	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}  yes" {
> +	    pass $gdb_test_name
> +	}
> +	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}" {
> +	    fail $gdb_test_name
> +	}

This should match up to the final $gdb_prompt.  Right now this isn't an
issue, but you're leaving uncaptured output in the expect buffer.  If a
new test is added after this in the future then that test is going to
suffer as a result.  Something like this should do:

    set seen_line false
    gdb_test_multiple "maintenance print remote-registers" \
             $expedited_pc_test_name -lbl {
	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}  yes" {
	    set seen_line true
	    exp_continue
	}
	-re "\r\n$gdb_prompt $" {
	    gdb_assert { $seen_line } $gdb_test_name
	}
    }

Thanks,
Andrew



> +    }
> +} else {
> +    untested $expedited_pc_test_name
> +}

Andrew Burgess <aburgess@redhat.com> writes:

> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>
>> One thing GDB always does when the inferior stops is finding out where
>> it's stopped at, by way of querying the value of the program counter
>> register.
>>
>> To save a packet round trip, the remote target can send the PC
>> value (often alongside other frequently consulted registers such as the
>> stack pointer) in the stop reply packet as an "expedited register".
>>
>> Test that this is actually done for the targets where gdbserver is
>> supposed to.
>>
>> Extend the "maintenance print remote-registers" command output with an
>> "Expedited" column which says "yes" if the register was seen by GDB in
>> the last stop reply packet it received, and is left blank otherwise.
>
> I think this is a good idea.

Thanks!

>> Tested for regressions on aarch64-linux-gnu native-extended-remote.
>>
>> The testcase was tested on aarch64-linux-gnu, i686-linux-gnu and
>> x86_64-linux-gnu native-remote and native-extended-remote targets.
>> ---
>>  gdb/regcache-dump.c                     |  9 +++++--
>>  gdb/remote.c                            | 27 ++++++++++++++++++++-
>>  gdb/remote.h                            |  5 ++++
>>  gdb/testsuite/gdb.server/server-run.exp | 31 +++++++++++++++++++++++++
>
> I think this should have a doc/ entry -- we already document the 'maint
> print remote-registers' command, so that should be expanded.
>
> I think it's important to document that this column only represents the
> last stop packet, and could potentially change from one stop to the
> next.
>
> I also think this should get a NEWS entry.  We add NEWS entries when new
> maintenance commands are added, so I don't see why we shouldn't document
> changes to the same.

Indeed. I hadn't realised that maintenance commands were documented.
v2 will add documentation and a NEWS entry.

>> diff --git a/gdb/testsuite/gdb.server/server-run.exp b/gdb/testsuite/gdb.server/server-run.exp
>> index 92eb38bd9db0..e22cee1e3c74 100644
>> --- a/gdb/testsuite/gdb.server/server-run.exp
>> +++ b/gdb/testsuite/gdb.server/server-run.exp
>> @@ -52,3 +52,34 @@ if { [istarget *-*-linux*] } {
>>  
>>  gdb_breakpoint main
>>  gdb_test "continue" "Breakpoint.* main .*" "continue to main"
>> +
>> +if { [istarget "aarch64*-*-*"]
>> +     || [istarget "arm*-*-*"]
>> +     || [istarget "csky*-*-*"]
>> +     || [istarget "loongarch*-*-*"]
>> +     || [istarget "riscv*-*-*"]} {
>> +    set pc_regname "pc"
>> +} elseif { [is_amd64_regs_target] } {
>> +    set pc_regname "rip"
>> +} elseif { [is_x86_like_target] } {
>> +    set pc_regname "eip"
>> +} elseif { [istarget "tic6x-*-*"] } {
>> +    set pc_regname "PC"
>> +}
>> +
>> +# Sending the PC register in advance is good practice.  Test that this is
>> +# actually done for the targets where gdbserver is supposed to.
>> +set expedited_pc_test_name "send PC as expedited register in stop reply"
>> +if { [info exists pc_regname] } {
>> +    gdb_test_multiple "maintenance print remote-registers" \
>> +		      $expedited_pc_test_name -lbl {
>> +	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}  yes" {
>> +	    pass $gdb_test_name
>> +	}
>> +	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}" {
>> +	    fail $gdb_test_name
>> +	}
>
> This should match up to the final $gdb_prompt.  Right now this isn't an
> issue, but you're leaving uncaptured output in the expect buffer.  If a
> new test is added after this in the future then that test is going to
> suffer as a result.

Good point! I hadn't realised it.

> Something like this should do:
>
>     set seen_line false
>     gdb_test_multiple "maintenance print remote-registers" \
>              $expedited_pc_test_name -lbl {
> 	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}  yes" {
> 	    set seen_line true
> 	    exp_continue
> 	}
> 	-re "\r\n$gdb_prompt $" {
> 	    gdb_assert { $seen_line } $gdb_test_name
> 	}
>     }

Thank you for providing the amended code. I adopted it for v2.

Hi,

On Wednesday, September 4, 2024 5:13 PM, Andrew Burgess wrote:
> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
...
> > +# Sending the PC register in advance is good practice.  Test that this is
> > +# actually done for the targets where gdbserver is supposed to.
> > +set expedited_pc_test_name "send PC as expedited register in stop reply"
> > +if { [info exists pc_regname] } {
> > +    gdb_test_multiple "maintenance print remote-registers" \
> > +		      $expedited_pc_test_name -lbl {
> > +	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}  yes" {
> > +	    pass $gdb_test_name
> > +	}
> > +	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}" {
> > +	    fail $gdb_test_name
> > +	}
> 
> This should match up to the final $gdb_prompt.  Right now this isn't an
> issue, but you're leaving uncaptured output in the expect buffer.  If a
> new test is added after this in the future then that test is going to
> suffer as a result.  Something like this should do:
> 
>     set seen_line false
>     gdb_test_multiple "maintenance print remote-registers" \
>              $expedited_pc_test_name -lbl {
> 	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}  yes" {
> 	    set seen_line true
> 	    exp_continue
> 	}
> 	-re "\r\n$gdb_prompt $" {

How about just

  -re -wrap "" {

for simplification purposes?

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

"Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:

> Hi,
>
> On Wednesday, September 4, 2024 5:13 PM, Andrew Burgess wrote:
>> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
> ...
>> > +# Sending the PC register in advance is good practice.  Test that this is
>> > +# actually done for the targets where gdbserver is supposed to.
>> > +set expedited_pc_test_name "send PC as expedited register in stop reply"
>> > +if { [info exists pc_regname] } {
>> > +    gdb_test_multiple "maintenance print remote-registers" \
>> > +		      $expedited_pc_test_name -lbl {
>> > +	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}  yes" {
>> > +	    pass $gdb_test_name
>> > +	}
>> > +	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}" {
>> > +	    fail $gdb_test_name
>> > +	}
>> 
>> This should match up to the final $gdb_prompt.  Right now this isn't an
>> issue, but you're leaving uncaptured output in the expect buffer.  If a
>> new test is added after this in the future then that test is going to
>> suffer as a result.  Something like this should do:
>> 
>>     set seen_line false
>>     gdb_test_multiple "maintenance print remote-registers" \
>>              $expedited_pc_test_name -lbl {
>> 	-re " ${pc_regname}\[\[:space:\]\]+${decimal}.*${decimal}  yes" {
>> 	    set seen_line true
>> 	    exp_continue
>> 	}
>> 	-re "\r\n$gdb_prompt $" {
>
> How about just
>
>   -re -wrap "" {
>
> for simplification purposes?

Yeah, that would probably be better.  Too often I forget about this
flag :(

Thanks,
Andrew