public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Update the 'g' packet documentation
@ 2023-01-11 18:37 Tom Tromey
  2023-01-11 19:56 ` Eli Zaretskii
  2023-01-13 12:09 ` Pedro Alves
  0 siblings, 2 replies; 9+ messages in thread
From: Tom Tromey @ 2023-01-11 18:37 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

The 'g' packet documentation references a macro that no longer exists,
and it also claims that the 'x' response for an unavailable register
is limited to trace frames.  This patch updates the documentation to
reflect what I think is currently correct.
---
 gdb/doc/gdb.texinfo | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9c0018ea5c1..80584241870 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41318,11 +41318,12 @@ Reply:
 Each byte of register data is described by two hex digits.  The bytes
 with the register are transmitted in target byte order.  The size of
 each register and their position within the @samp{g} packet are
-determined by the @value{GDBN} internal gdbarch functions
-@code{DEPRECATED_REGISTER_RAW_SIZE} and @code{gdbarch_register_name}.
+determined by the target description (@pxref{Target Descriptions}); in
+the absence of a target description, this is done using code internal
+to @value{GDBN}; typically this is some customary register layout for
+the architecture in question.
 
-When reading registers from a trace frame (@pxref{Analyze Collected
-Data,,Using the Collected Data}), the stub may also return a string of
+When reading registers, the stub may also return a string of
 literal @samp{x}'s in place of the register data digits, to indicate
 that the corresponding register has not been collected, thus its value
 is unavailable.  For example, for an architecture with 4 registers of
-- 
2.38.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Update the 'g' packet documentation
  2023-01-11 18:37 [PATCH] Update the 'g' packet documentation Tom Tromey
@ 2023-01-11 19:56 ` Eli Zaretskii
  2023-01-13 12:09 ` Pedro Alves
  1 sibling, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2023-01-11 19:56 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

> Cc: Tom Tromey <tromey@adacore.com>
> Date: Wed, 11 Jan 2023 11:37:25 -0700
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> 
> The 'g' packet documentation references a macro that no longer exists,
> and it also claims that the 'x' response for an unavailable register
> is limited to trace frames.  This patch updates the documentation to
> reflect what I think is currently correct.
> ---
>  gdb/doc/gdb.texinfo | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

OK, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Update the 'g' packet documentation
  2023-01-11 18:37 [PATCH] Update the 'g' packet documentation Tom Tromey
  2023-01-11 19:56 ` Eli Zaretskii
@ 2023-01-13 12:09 ` Pedro Alves
  2023-01-13 18:58   ` Tom Tromey
  1 sibling, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2023-01-13 12:09 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches

Hi Tom,

On 2023-01-11 6:37 p.m., Tom Tromey via Gdb-patches wrote:
> The 'g' packet documentation references a macro that no longer exists,
> and it also claims that the 'x' response for an unavailable register
> is limited to trace frames.  This patch updates the documentation to
> reflect what I think is currently correct.
> ---
>  gdb/doc/gdb.texinfo | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 9c0018ea5c1..80584241870 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -41318,11 +41318,12 @@ Reply:
>  Each byte of register data is described by two hex digits.  The bytes
>  with the register are transmitted in target byte order.  The size of
>  each register and their position within the @samp{g} packet are
> -determined by the @value{GDBN} internal gdbarch functions
> -@code{DEPRECATED_REGISTER_RAW_SIZE} and @code{gdbarch_register_name}.
> +determined by the target description (@pxref{Target Descriptions}); in
> +the absence of a target description, this is done using code internal
> +to @value{GDBN}; typically this is some customary register layout for
> +the architecture in question.
>  

This part seems fine.

> -When reading registers from a trace frame (@pxref{Analyze Collected
> -Data,,Using the Collected Data}), the stub may also return a string of
> +When reading registers, the stub may also return a string of
>  literal @samp{x}'s in place of the register data digits, to indicate
>  that the corresponding register has not been collected, thus its value
>  is unavailable.  For example, for an architecture with 4 registers of
> 

Here, the new text still uses "collected", but lost the reference to trace frames.
It seems to me that that will result in people not knowing what "collected"
means in this context.  I suggest flipping things around a little, like:

 When reading registers, the stub may also return a string of
 literal @samp{x}'s in place of the register data digits, to indicate
 that the corresponding register's value is unavailable.  For example,
 when reading registers from a trace frame (@pxref{Analyze Collected
 Data,,Using the Collected Data}), this means that the register has
 not been collected.

and then the following sentence, where it reads

 For example, for an architecture with 4 registers of
 4 bytes each, the following reply indicates to @value{GDBN} that
 registers 0 and 2 have not been collected, while registers 1 and 3
 have been collected, and both have zero value:

it may be better to tweak it to say something like:

 For example, for an architecture with 4 registers of
 4 bytes each, the following reply indicates to @value{GDBN} that
 registers 0 and 2 are unavailable, while registers 1 and 3
 are available, and both have zero value:

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Update the 'g' packet documentation
  2023-01-13 12:09 ` Pedro Alves
@ 2023-01-13 18:58   ` Tom Tromey
  2023-01-27 15:25     ` [PATCH v2] " Pedro Alves
  0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2023-01-13 18:58 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Tom Tromey, gdb-patches

>> -When reading registers from a trace frame (@pxref{Analyze Collected
>> -Data,,Using the Collected Data}), the stub may also return a string of
>> +When reading registers, the stub may also return a string of
>> literal @samp{x}'s in place of the register data digits, to indicate
>> that the corresponding register has not been collected, thus its value
>> is unavailable.  For example, for an architecture with 4 registers of
>> 

Pedro> Here, the new text still uses "collected", but lost the reference to trace frames.
Pedro> It seems to me that that will result in people not knowing what "collected"
Pedro> means in this context.

Yeah, I wanted to get rid of the trace frame note, because it's
confusing -- 'x' can be sent any time, not just a trace frame.

Tom

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] Update the 'g' packet documentation
  2023-01-13 18:58   ` Tom Tromey
@ 2023-01-27 15:25     ` Pedro Alves
  2023-01-27 16:15       ` Eli Zaretskii
  2023-01-30 21:15       ` Tom Tromey
  0 siblings, 2 replies; 9+ messages in thread
From: Pedro Alves @ 2023-01-27 15:25 UTC (permalink / raw)
  To: Tom Tromey, Eli Zaretskii; +Cc: gdb-patches

On 2023-01-13 6:58 p.m., Tom Tromey wrote:
>>> -When reading registers from a trace frame (@pxref{Analyze Collected
>>> -Data,,Using the Collected Data}), the stub may also return a string of
>>> +When reading registers, the stub may also return a string of
>>> literal @samp{x}'s in place of the register data digits, to indicate
>>> that the corresponding register has not been collected, thus its value
>>> is unavailable.  For example, for an architecture with 4 registers of
>>>
> 
> Pedro> Here, the new text still uses "collected", but lost the reference to trace frames.
> Pedro> It seems to me that that will result in people not knowing what "collected"
> Pedro> means in this context.
> 
> Yeah, I wanted to get rid of the trace frame note, because it's
> confusing -- 'x' can be sent any time, not just a trace frame.

Yeah, but then people won't know what "collected" here means.  Also, in the normal
case you shouldn't really end up with unavailable registers -- if some register
really doesn't exist, then the target description should ideally not describe it.

How about this version of the patch?  It combines your original patch with
my suggestions, and also extends it further to describe the normal live target
scenario and include a remark about just not including the register in the tdesc.
(Note: I did not add an xref for the target description section at the end because
there's already one in the preceding paragraph.)

From 0d6c9a0b451933042e2b0d28c6a13bac0d044433 Mon Sep 17 00:00:00 2001
From: Tom Tromey <tom@tromey.com>
Date: Wed, 11 Jan 2023 11:37:25 -0700
Subject: [PATCH] Update the 'g' packet documentation

The 'g' packet documentation references a macro that no longer exists,
and it also claims that the 'x' response for an unavailable register
is limited to trace frames.  This patch updates the documentation to
reflect what I think is currently correct.

Co-Authored-By: Pedro Alves <pedro@palves.net>
Change-Id: I863baa3b9293059cfd4aa3d534602cbcb693ba87
---
 gdb/doc/gdb.texinfo | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b5fad2cb16e..2bc4b5b4aa8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41352,17 +41352,27 @@ Reply:
 Each byte of register data is described by two hex digits.  The bytes
 with the register are transmitted in target byte order.  The size of
 each register and their position within the @samp{g} packet are
-determined by the @value{GDBN} internal gdbarch functions
-@code{DEPRECATED_REGISTER_RAW_SIZE} and @code{gdbarch_register_name}.
-
-When reading registers from a trace frame (@pxref{Analyze Collected
-Data,,Using the Collected Data}), the stub may also return a string of
-literal @samp{x}'s in place of the register data digits, to indicate
-that the corresponding register has not been collected, thus its value
-is unavailable.  For example, for an architecture with 4 registers of
+determined by the target description (@pxref{Target Descriptions}); in
+the absence of a target description, this is done using code internal
+to @value{GDBN}; typically this is some customary register layout for
+the architecture in question.
+
+When reading registers, the stub may also return a string of literal
+@samp{x}'s in place of the register data digits, to indicate that the
+corresponding register's value is unavailable.  For example, when
+reading registers from a trace frame (@pxref{Analyze Collected
+Data,,Using the Collected Data}), this means that the register has not
+been collected in the trace frame.  When reading registers from a live
+program, this indicates that the stub has no means to access the
+register contents, even though the corresponding register is known to
+exist.  Note that if a register truly does not exist on the target,
+then it is better to not include it in the target description in the
+first place.
+
+For example, for an architecture with 4 registers of
 4 bytes each, the following reply indicates to @value{GDBN} that
-registers 0 and 2 have not been collected, while registers 1 and 3
-have been collected, and both have zero value:
+registers 0 and 2 are unavailable, while registers 1 and 3
+are available, and both have zero value:
 
 @smallexample
 -> @code{g}

base-commit: 1c66b8a03989b963534689ec0a9cce57e419afd5
-- 
2.36.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Update the 'g' packet documentation
  2023-01-27 15:25     ` [PATCH v2] " Pedro Alves
@ 2023-01-27 16:15       ` Eli Zaretskii
  2023-01-30 21:15       ` Tom Tromey
  1 sibling, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2023-01-27 16:15 UTC (permalink / raw)
  To: Pedro Alves; +Cc: tromey, gdb-patches

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 27 Jan 2023 15:25:33 +0000
> 
> >From 0d6c9a0b451933042e2b0d28c6a13bac0d044433 Mon Sep 17 00:00:00 2001
> From: Tom Tromey <tom@tromey.com>
> Date: Wed, 11 Jan 2023 11:37:25 -0700
> Subject: [PATCH] Update the 'g' packet documentation
> 
> The 'g' packet documentation references a macro that no longer exists,
> and it also claims that the 'x' response for an unavailable register
> is limited to trace frames.  This patch updates the documentation to
> reflect what I think is currently correct.
> 
> Co-Authored-By: Pedro Alves <pedro@palves.net>
> Change-Id: I863baa3b9293059cfd4aa3d534602cbcb693ba87
> ---
>  gdb/doc/gdb.texinfo | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)

This is OK, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Update the 'g' packet documentation
  2023-01-27 15:25     ` [PATCH v2] " Pedro Alves
  2023-01-27 16:15       ` Eli Zaretskii
@ 2023-01-30 21:15       ` Tom Tromey
  2023-02-16 17:10         ` Pedro Alves
  1 sibling, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2023-01-30 21:15 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Tom Tromey, Eli Zaretskii, gdb-patches

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> Here, the new text still uses "collected", but lost the reference to trace frames.
Pedro> It seems to me that that will result in people not knowing what "collected"
Pedro> means in this context.

>> Yeah, I wanted to get rid of the trace frame note, because it's
>> confusing -- 'x' can be sent any time, not just a trace frame.

Pedro> Yeah, but then people won't know what "collected" here means.  Also, in the normal
Pedro> case you shouldn't really end up with unavailable registers -- if some register
Pedro> really doesn't exist, then the target description should ideally not describe it.

While I agree with that, it's also the case that tdep code can reject
such a description, and we ran into a case like this in the wild --
where a gdbserver does not report a register, causing i386-tdep to
reject the description.  However, at one point AdaCore had a port for
this target, and that port reported the register but sent back 'x' --
which worked.

Also, I wanted to say, I think I misread your first reply.  If I'd read
it correctly I would have rewritten the text.  I'm sorry about that.

Pedro> How about this version of the patch?  It combines your original patch with
Pedro> my suggestions, and also extends it further to describe the normal live target
Pedro> scenario and include a remark about just not including the register in the tdesc.
Pedro> (Note: I did not add an xref for the target description section at the end because
Pedro> there's already one in the preceding paragraph.)

This sounds great.  Thank you.

Tom

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Update the 'g' packet documentation
  2023-01-30 21:15       ` Tom Tromey
@ 2023-02-16 17:10         ` Pedro Alves
  2023-02-16 18:16           ` Tom Tromey
  0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2023-02-16 17:10 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Eli Zaretskii, gdb-patches

On 2023-01-30 9:15 p.m., Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> Here, the new text still uses "collected", but lost the reference to trace frames.
> Pedro> It seems to me that that will result in people not knowing what "collected"
> Pedro> means in this context.
> 
>>> Yeah, I wanted to get rid of the trace frame note, because it's
>>> confusing -- 'x' can be sent any time, not just a trace frame.
> 
> Pedro> Yeah, but then people won't know what "collected" here means.  Also, in the normal
> Pedro> case you shouldn't really end up with unavailable registers -- if some register
> Pedro> really doesn't exist, then the target description should ideally not describe it.
> 
> While I agree with that, it's also the case that tdep code can reject
> such a description, and we ran into a case like this in the wild --
> where a gdbserver does not report a register, causing i386-tdep to
> reject the description.  However, at one point AdaCore had a port for
> this target, and that port reported the register but sent back 'x' --
> which worked.

Curious.  It sounds like we made some register in a tdesc feature mandatory,
even after the tdesc feature existed in previous releases?  That shouldn't have
happened.

> 
> Also, I wanted to say, I think I misread your first reply.  If I'd read
> it correctly I would have rewritten the text.  I'm sorry about that.

No worries at all.

> Pedro> How about this version of the patch?  It combines your original patch with
> Pedro> my suggestions, and also extends it further to describe the normal live target
> Pedro> scenario and include a remark about just not including the register in the tdesc.
> Pedro> (Note: I did not add an xref for the target description section at the end because
> Pedro> there's already one in the preceding paragraph.)
> 
> This sounds great.  Thank you.

Great, I'm going to merge it.

Thanks,
Pedro Alves


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Update the 'g' packet documentation
  2023-02-16 17:10         ` Pedro Alves
@ 2023-02-16 18:16           ` Tom Tromey
  0 siblings, 0 replies; 9+ messages in thread
From: Tom Tromey @ 2023-02-16 18:16 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Tom Tromey, Eli Zaretskii, gdb-patches

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

>> While I agree with that, it's also the case that tdep code can reject
>> such a description, and we ran into a case like this in the wild --
>> where a gdbserver does not report a register, causing i386-tdep to
>> reject the description.  However, at one point AdaCore had a port for
>> this target, and that port reported the register but sent back 'x' --
>> which worked.

Pedro> Curious.  It sounds like we made some register in a tdesc feature
Pedro> mandatory, even after the tdesc feature existed in previous
Pedro> releases?  That shouldn't have happened.

No, we are in the clear, what happened is that a vendor out there hacked
gdbserver to drop a register (the x86 "fop" register) for reasons we do
not know.  They also hacked their gdb to accept this.

AdaCore also had a patch for this target's gdbserver to not supply "fop"
(the kernel, mysteriously, does not make it available) -- but did not
change the XML or gdb, instead having the register report as
unavailable.

Tom

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-02-16 18:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-11 18:37 [PATCH] Update the 'g' packet documentation Tom Tromey
2023-01-11 19:56 ` Eli Zaretskii
2023-01-13 12:09 ` Pedro Alves
2023-01-13 18:58   ` Tom Tromey
2023-01-27 15:25     ` [PATCH v2] " Pedro Alves
2023-01-27 16:15       ` Eli Zaretskii
2023-01-30 21:15       ` Tom Tromey
2023-02-16 17:10         ` Pedro Alves
2023-02-16 18:16           ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).