[PATCH] Update btrace data in maintenance btrace commands

[PATCH] Update btrace data in maintenance btrace commands

[Stephen Röttger]

From: Stephen Roettger <sroettger@google.com>

When calling `maintenance btrace packet-history` or `maintenance info btrace`,
the commands wouldn't show any data unless it was updated previously by some
other means, for example by running `info record`.
To fix this, use the require_btrace function from record-brace.h which
will update the data before returning the btrace_thread_info pointer.

Here's an example of the issue:
```
$ gdb -nx /usr/bin/ls
(gdb) break malloc
Breakpoint 1 at 0x46c8
(gdb) run
Starting program: /usr/bin/ls 

Breakpoint 1, 0x00007ffff7fec240 in malloc () from /lib64/ld-linux-x86-64.so.2
(gdb) record btrace bts
(gdb) c
Continuing.

Breakpoint 1, 0x00007ffff7fec240 in malloc () from /lib64/ld-linux-x86-64.so.2
(gdb) maintenance btrace packet-history 
No trace.
(gdb) maintenance info btrace 
Format: Branch Trace Store.
Aborted
```

gdb/ChangeLog:

        * btrace.c (maint_btrace_packet_history_cmd): use require_btrace
        (maint_info_btrace_cmd): use require_btrace
        * record-btrace.c (struct btrace_thread_info): mark non-static
        * record-btrace.h (require_btrace): export function

---
 gdb/btrace.c        | 13 ++-----------
 gdb/record-btrace.c |  2 +-
 gdb/record-btrace.h |  7 +++++++
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index c697f37f46c..177e818f0cf 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -3231,12 +3231,8 @@ maint_btrace_packet_history_cmd (const char *arg, int from_tty)
   struct btrace_thread_info *btinfo;
   unsigned int size, begin, end, from, to;
 
-  thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
-  if (tp == NULL)
-    error (_("No thread."));
-
+  btinfo = require_btrace ();
   size = 10;
-  btinfo = &tp->btrace;
 
   btrace_maint_update_packets (btinfo, &begin, &end, &from, &to);
   if (begin == end)
@@ -3372,12 +3368,7 @@ maint_info_btrace_cmd (const char *args, int from_tty)
   if (args != NULL && *args != 0)
     error (_("Invalid argument."));
 
-  if (inferior_ptid == null_ptid)
-    error (_("No thread."));
-
-  thread_info *tp = inferior_thread ();
-
-  btinfo = &tp->btrace;
+  btinfo = require_btrace ();
 
   conf = btrace_conf (btinfo);
   if (conf == NULL)
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index b7b3c91f85d..79a6e47a4a6 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -269,7 +269,7 @@ require_btrace_thread (void)
    Throws an error if there is no thread or no trace.  This function never
    returns NULL.  */
 
-static struct btrace_thread_info *
+struct btrace_thread_info *
 require_btrace (void)
 {
   struct thread_info *tp;
diff --git a/gdb/record-btrace.h b/gdb/record-btrace.h
index 8e7bbd90e60..b56e720e5a5 100644
--- a/gdb/record-btrace.h
+++ b/gdb/record-btrace.h
@@ -29,4 +29,11 @@ extern void record_btrace_push_target (void);
    NULL if the cpu was configured as auto.  */
 extern const struct btrace_cpu *record_btrace_get_cpu (void);
 
+/* Update the branch trace for the current thread and return a pointer to its
+   branch trace information struct.
+
+   Throws an error if there is no thread or no trace.  This function never
+   returns NULL.  */
+extern struct btrace_thread_info * require_btrace (void);
+
 #endif /* RECORD_BTRACE_H */
-- 
2.31.1.295.g9ea45b61b8-goog

Re: [PING] [PATCH] Update btrace data in maintenance btrace commands

[Stephen Röttger]

[-- Attachment #1: Type: text/plain, Size: 3720 bytes --]

On Mon, Apr 12, 2021 at 4:01 PM Stephen Röttger <sroettger@google.com>
wrote:

> From: Stephen Roettger <sroettger@google.com>
>
> When calling `maintenance btrace packet-history` or `maintenance info
> btrace`,
> the commands wouldn't show any data unless it was updated previously by
> some
> other means, for example by running `info record`.
> To fix this, use the require_btrace function from record-brace.h which
> will update the data before returning the btrace_thread_info pointer.
>
> Here's an example of the issue:
> ```
> $ gdb -nx /usr/bin/ls
> (gdb) break malloc
> Breakpoint 1 at 0x46c8
> (gdb) run
> Starting program: /usr/bin/ls
>
> Breakpoint 1, 0x00007ffff7fec240 in malloc () from
> /lib64/ld-linux-x86-64.so.2
> (gdb) record btrace bts
> (gdb) c
> Continuing.
>
> Breakpoint 1, 0x00007ffff7fec240 in malloc () from
> /lib64/ld-linux-x86-64.so.2
> (gdb) maintenance btrace packet-history
> No trace.
> (gdb) maintenance info btrace
> Format: Branch Trace Store.
> Aborted
> ```
>
> gdb/ChangeLog:
>
>         * btrace.c (maint_btrace_packet_history_cmd): use require_btrace
>         (maint_info_btrace_cmd): use require_btrace
>         * record-btrace.c (struct btrace_thread_info): mark non-static
>         * record-btrace.h (require_btrace): export function
>
> ---
>  gdb/btrace.c        | 13 ++-----------
>  gdb/record-btrace.c |  2 +-
>  gdb/record-btrace.h |  7 +++++++
>  3 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index c697f37f46c..177e818f0cf 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -3231,12 +3231,8 @@ maint_btrace_packet_history_cmd (const char *arg,
> int from_tty)
>    struct btrace_thread_info *btinfo;
>    unsigned int size, begin, end, from, to;
>
> -  thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
> -  if (tp == NULL)
> -    error (_("No thread."));
> -
> +  btinfo = require_btrace ();
>    size = 10;
> -  btinfo = &tp->btrace;
>
>    btrace_maint_update_packets (btinfo, &begin, &end, &from, &to);
>    if (begin == end)
> @@ -3372,12 +3368,7 @@ maint_info_btrace_cmd (const char *args, int
> from_tty)
>    if (args != NULL && *args != 0)
>      error (_("Invalid argument."));
>
> -  if (inferior_ptid == null_ptid)
> -    error (_("No thread."));
> -
> -  thread_info *tp = inferior_thread ();
> -
> -  btinfo = &tp->btrace;
> +  btinfo = require_btrace ();
>
>    conf = btrace_conf (btinfo);
>    if (conf == NULL)
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index b7b3c91f85d..79a6e47a4a6 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -269,7 +269,7 @@ require_btrace_thread (void)
>     Throws an error if there is no thread or no trace.  This function never
>     returns NULL.  */
>
> -static struct btrace_thread_info *
> +struct btrace_thread_info *
>  require_btrace (void)
>  {
>    struct thread_info *tp;
> diff --git a/gdb/record-btrace.h b/gdb/record-btrace.h
> index 8e7bbd90e60..b56e720e5a5 100644
> --- a/gdb/record-btrace.h
> +++ b/gdb/record-btrace.h
> @@ -29,4 +29,11 @@ extern void record_btrace_push_target (void);
>     NULL if the cpu was configured as auto.  */
>  extern const struct btrace_cpu *record_btrace_get_cpu (void);
>
> +/* Update the branch trace for the current thread and return a pointer to
> its
> +   branch trace information struct.
> +
> +   Throws an error if there is no thread or no trace.  This function never
> +   returns NULL.  */
> +extern struct btrace_thread_info * require_btrace (void);
> +
>  #endif /* RECORD_BTRACE_H */
> --
> 2.31.1.295.g9ea45b61b8-goog
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4002 bytes --]

Re: [PATCH] Update btrace data in maintenance btrace commands

[Simon Marchi]

On 2021-04-12 10:01 a.m., Stephen Röttger via Gdb-patches wrote:
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index b7b3c91f85d..79a6e47a4a6 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -269,7 +269,7 @@ require_btrace_thread (void)
>     Throws an error if there is no thread or no trace.  This function never
>     returns NULL.  */
>  
> -static struct btrace_thread_info *
> +struct btrace_thread_info *
>  require_btrace (void)

Since the doc is now in the header, write:

/* See record-btrace.h.  */

here.

I'd LGTM this patch, but I'd like Markus (the btrace specialist) to
give it a look (I think he's away, it will be in a few weeks).

Simon

RE: [PATCH] Update btrace data in maintenance btrace commands

[Metzger, Markus T]

Hello Stephen,

>On 2021-04-12 10:01 a.m., Stephen Röttger via Gdb-patches wrote:
>> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
>> index b7b3c91f85d..79a6e47a4a6 100644
>> --- a/gdb/record-btrace.c
>> +++ b/gdb/record-btrace.c
>> @@ -269,7 +269,7 @@ require_btrace_thread (void)
>>     Throws an error if there is no thread or no trace.  This function never
>>     returns NULL.  */
>>
>> -static struct btrace_thread_info *
>> +struct btrace_thread_info *
>>  require_btrace (void)
>
>Since the doc is now in the header, write:
>
>/* See record-btrace.h.  */
>
>here.
>
>I'd LGTM this patch, but I'd like Markus (the btrace specialist) to
>give it a look (I think he's away, it will be in a few weeks).

Those maintenance commands are not intended for users.  You'd really only
need them to debug issues with PT decode or recording.  My main use-case is
to clear the history, then turn on debugging and logging, and re-decode the
trace.  Re-fetching shouldn't hurt but isn't necessary.

Your patch LGTM (with the comment fixed) but I wonder whether there's
anything to fix, at all.  If you end up needing to use those maint commands,
something else may be wrong.

Regards,
Markus.

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

Re: [PATCH] Update btrace data in maintenance btrace commands

[Stephen Röttger]

[-- Attachment #1: Type: text/plain, Size: 2209 bytes --]

Ah that makes sense. Thanks Markus and Simon.

You're right, in the end I didn't rely on these commands. Though when I started
playing with btrace, it did make me think btrace was broken on my machine and
it took me a bit to figure out what's going on. So I think this change can
still be valuable to save others the headache :).
Let me know what you think, if you think it's valuable I'll send a v2 with the
proposed change.

Cheers,
Stephen

On Tue, Jun 8, 2021 at 4:05 PM Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
>
> Hello Stephen,
>
> >On 2021-04-12 10:01 a.m., Stephen Röttger via Gdb-patches wrote:
> >> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> >> index b7b3c91f85d..79a6e47a4a6 100644
> >> --- a/gdb/record-btrace.c
> >> +++ b/gdb/record-btrace.c
> >> @@ -269,7 +269,7 @@ require_btrace_thread (void)
> >>     Throws an error if there is no thread or no trace.  This function never
> >>     returns NULL.  */
> >>
> >> -static struct btrace_thread_info *
> >> +struct btrace_thread_info *
> >>  require_btrace (void)
> >
> >Since the doc is now in the header, write:
> >
> >/* See record-btrace.h.  */
> >
> >here.
> >
> >I'd LGTM this patch, but I'd like Markus (the btrace specialist) to
> >give it a look (I think he's away, it will be in a few weeks).
>
> Those maintenance commands are not intended for users.  You'd really only
> need them to debug issues with PT decode or recording.  My main use-case is
> to clear the history, then turn on debugging and logging, and re-decode the
> trace.  Re-fetching shouldn't hurt but isn't necessary.
>
> Your patch LGTM (with the comment fixed) but I wonder whether there's
> anything to fix, at all.  If you end up needing to use those maint commands,
> something else may be wrong.
>
> Regards,
> Markus.
>
> 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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4002 bytes --]

Re: [PATCH] Update btrace data in maintenance btrace commands

[Stephen Röttger]

[-- Attachment #1: Type: text/plain, Size: 2620 bytes --]

> Let me know what you think, if you think it's valuable I'll send a v2
with the
proposed change.

Hey, just checking if you would like this patch, should I make the proposed
change?

On Wed, Jun 9, 2021 at 11:41 AM Stephen Röttger <sroettger@google.com>
wrote:

> Ah that makes sense. Thanks Markus and Simon.
>
> You're right, in the end I didn't rely on these commands. Though when I
> started
> playing with btrace, it did make me think btrace was broken on my machine
> and
> it took me a bit to figure out what's going on. So I think this change can
> still be valuable to save others the headache :).
> Let me know what you think, if you think it's valuable I'll send a v2 with
> the
> proposed change.
>
> Cheers,
> Stephen
>
> On Tue, Jun 8, 2021 at 4:05 PM Metzger, Markus T
> <markus.t.metzger@intel.com> wrote:
> >
> > Hello Stephen,
> >
> > >On 2021-04-12 10:01 a.m., Stephen Röttger via Gdb-patches wrote:
> > >> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> > >> index b7b3c91f85d..79a6e47a4a6 100644
> > >> --- a/gdb/record-btrace.c
> > >> +++ b/gdb/record-btrace.c
> > >> @@ -269,7 +269,7 @@ require_btrace_thread (void)
> > >>     Throws an error if there is no thread or no trace.  This function
> never
> > >>     returns NULL.  */
> > >>
> > >> -static struct btrace_thread_info *
> > >> +struct btrace_thread_info *
> > >>  require_btrace (void)
> > >
> > >Since the doc is now in the header, write:
> > >
> > >/* See record-btrace.h.  */
> > >
> > >here.
> > >
> > >I'd LGTM this patch, but I'd like Markus (the btrace specialist) to
> > >give it a look (I think he's away, it will be in a few weeks).
> >
> > Those maintenance commands are not intended for users.  You'd really only
> > need them to debug issues with PT decode or recording.  My main use-case
> is
> > to clear the history, then turn on debugging and logging, and re-decode
> the
> > trace.  Re-fetching shouldn't hurt but isn't necessary.
> >
> > Your patch LGTM (with the comment fixed) but I wonder whether there's
> > anything to fix, at all.  If you end up needing to use those maint
> commands,
> > something else may be wrong.
> >
> > Regards,
> > Markus.
> >
> > 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
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4002 bytes --]

RE: [PATCH] Update btrace data in maintenance btrace commands

[Metzger, Markus T]

Hello Stephen,

I’m OK with the patch if you want to send an updated version.

Thanks,
Markus.

From: Stephen Röttger <sroettger@google.com>
Sent: Tuesday, August 3, 2021 10:50 AM
To: Metzger, Markus T <markus.t.metzger@intel.com>
Cc: gdb-patches@sourceware.org; Simon Marchi <simon.marchi@polymtl.ca>
Subject: Re: [PATCH] Update btrace data in maintenance btrace commands

> Let me know what you think, if you think it's valuable I'll send a v2 with the
proposed change.

Hey, just checking if you would like this patch, should I make the proposed change?

On Wed, Jun 9, 2021 at 11:41 AM Stephen Röttger <sroettger@google.com<mailto:sroettger@google.com>> wrote:
Ah that makes sense. Thanks Markus and Simon.

You're right, in the end I didn't rely on these commands. Though when I started
playing with btrace, it did make me think btrace was broken on my machine and
it took me a bit to figure out what's going on. So I think this change can
still be valuable to save others the headache :).
Let me know what you think, if you think it's valuable I'll send a v2 with the
proposed change.

Cheers,
Stephen

On Tue, Jun 8, 2021 at 4:05 PM Metzger, Markus T
<markus.t.metzger@intel.com<mailto:markus.t.metzger@intel.com>> wrote:
>
> Hello Stephen,
>
> >On 2021-04-12 10:01 a.m., Stephen Röttger via Gdb-patches wrote:
> >> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> >> index b7b3c91f85d..79a6e47a4a6 100644
> >> --- a/gdb/record-btrace.c
> >> +++ b/gdb/record-btrace.c
> >> @@ -269,7 +269,7 @@ require_btrace_thread (void)
> >>     Throws an error if there is no thread or no trace.  This function never
> >>     returns NULL.  */
> >>
> >> -static struct btrace_thread_info *
> >> +struct btrace_thread_info *
> >>  require_btrace (void)
> >
> >Since the doc is now in the header, write:
> >
> >/* See record-btrace.h.  */
> >
> >here.
> >
> >I'd LGTM this patch, but I'd like Markus (the btrace specialist) to
> >give it a look (I think he's away, it will be in a few weeks).
>
> Those maintenance commands are not intended for users.  You'd really only
> need them to debug issues with PT decode or recording.  My main use-case is
> to clear the history, then turn on debugging and logging, and re-decode the
> trace.  Re-fetching shouldn't hurt but isn't necessary.
>
> Your patch LGTM (with the comment fixed) but I wonder whether there's
> anything to fix, at all.  If you end up needing to use those maint commands,
> something else may be wrong.
>
> Regards,
> Markus.
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de<http://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
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