[PATCH] gdb, btrace: Throw an error for empty recordings when replaying starts.

[PATCH] gdb, btrace: Throw an error for empty recordings when replaying starts.

[Felix Willgerodt]

This makes record_btrace_start_replaying() more consistent, as it already
errors out e.g. on a recording with only gaps.
---
 gdb/record-btrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 373d82b8b99..3f8a69dd04f 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2017,7 +2017,7 @@ record_btrace_start_replaying (struct thread_info *tp)
 
   /* We can't start replaying without trace.  */
   if (btinfo->functions.empty ())
-    return NULL;
+    error (_("No trace."));
 
   /* GDB stores the current frame_id when stepping in order to detects steps
      into subroutines.
-- 
2.34.3

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] gdb, btrace: Throw an error for empty recordings when replaying starts.

[Andrew Burgess]

Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> writes:

> This makes record_btrace_start_replaying() more consistent, as it already
> errors out e.g. on a recording with only gaps.
> ---
>  gdb/record-btrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 373d82b8b99..3f8a69dd04f 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -2017,7 +2017,7 @@ record_btrace_start_replaying (struct thread_info *tp)
>  
>    /* We can't start replaying without trace.  */
>    if (btinfo->functions.empty ())
> -    return NULL;
> +    error (_("No trace."));
>  
>    /* GDB stores the current frame_id when stepping in order to detects steps
>       into subroutines.

Please can you add a test for this change.

Thanks,
Andrew

RE: [PATCH] gdb, btrace: Throw an error for empty recordings when replaying starts.

[Willgerodt, Felix]

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Mittwoch, 11. Mai 2022 11:34
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; Metzger, Markus T
> <markus.t.metzger@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH] gdb, btrace: Throw an error for empty recordings when
> replaying starts.
> 
> Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > This makes record_btrace_start_replaying() more consistent, as it already
> > errors out e.g. on a recording with only gaps.
> > ---
> >  gdb/record-btrace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> > index 373d82b8b99..3f8a69dd04f 100644
> > --- a/gdb/record-btrace.c
> > +++ b/gdb/record-btrace.c
> > @@ -2017,7 +2017,7 @@ record_btrace_start_replaying (struct
> thread_info *tp)
> >
> >    /* We can't start replaying without trace.  */
> >    if (btinfo->functions.empty ())
> > -    return NULL;
> > +    error (_("No trace."));
> >
> >    /* GDB stores the current frame_id when stepping in order to detects
> steps
> >       into subroutines.
> 
> Please can you add a test for this change.
> 
> Thanks,
> Andrew

Mhm, I don't think there is an actual test I can write to trigger this.
All callers prevent it from happening by checking btinfo->replay.
I am just making it more consistent, as the function already
errors out in the case of just gaps. Another advantage is that this function
doesn't return NULL anymore, which seems better for future callers.

Thanks,
Felix
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] gdb, btrace: Throw an error for empty recordings when replaying starts.

[Tom Tromey]

>>>>> Willgerodt, Felix via Gdb-patches <gdb-patches@sourceware.org> writes:

> Mhm, I don't think there is an actual test I can write to trigger this.
> All callers prevent it from happening by checking btinfo->replay.
> I am just making it more consistent, as the function already
> errors out in the case of just gaps. Another advantage is that this function
> doesn't return NULL anymore, which seems better for future callers.

I think the patch is still fine, but at the same time it seems like an
assert would be just as correct.

Tom

RE: [PATCH] gdb, btrace: Throw an error for empty recordings when replaying starts.

[Metzger, Markus T]

>> Mhm, I don't think there is an actual test I can write to trigger this.
>> All callers prevent it from happening by checking btinfo->replay.
>> I am just making it more consistent, as the function already
>> errors out in the case of just gaps. Another advantage is that this function
>> doesn't return NULL anymore, which seems better for future callers.
>
>I think the patch is still fine, but at the same time it seems like an
>assert would be just as correct.

It would.  But the proposed error matches a similar one a few lines below.

LGTM,
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] gdb, btrace: Throw an error for empty recordings when replaying starts.

[Willgerodt, Felix]

Thanks, I just pushed this.

Felix

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 17. Mai 2022 12:26
> To: Tom Tromey <tom@tromey.com>; Willgerodt, Felix via Gdb-patches
> <gdb-patches@sourceware.org>
> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>
> Subject: RE: [PATCH] gdb, btrace: Throw an error for empty recordings when
> replaying starts.
> 
> 
> >> Mhm, I don't think there is an actual test I can write to trigger this.
> >> All callers prevent it from happening by checking btinfo->replay.
> >> I am just making it more consistent, as the function already
> >> errors out in the case of just gaps. Another advantage is that this function
> >> doesn't return NULL anymore, which seems better for future callers.
> >
> >I think the patch is still fine, but at the same time it seems like an
> >assert would be just as correct.
> 
> It would.  But the proposed error matches a similar one a few lines below.
> 
> LGTM,
> 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] gdb, btrace: Throw an error for empty recordings when replaying starts.

[Andrew Burgess]

"Willgerodt, Felix via Gdb-patches" <gdb-patches@sourceware.org> writes:

>> -----Original Message-----
>> From: Andrew Burgess <aburgess@redhat.com>
>> Sent: Mittwoch, 11. Mai 2022 11:34
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; Metzger, Markus T
>> <markus.t.metzger@intel.com>; gdb-patches@sourceware.org
>> Subject: Re: [PATCH] gdb, btrace: Throw an error for empty recordings when
>> replaying starts.
>> 
>> Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>> > This makes record_btrace_start_replaying() more consistent, as it already
>> > errors out e.g. on a recording with only gaps.
>> > ---
>> >  gdb/record-btrace.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
>> > index 373d82b8b99..3f8a69dd04f 100644
>> > --- a/gdb/record-btrace.c
>> > +++ b/gdb/record-btrace.c
>> > @@ -2017,7 +2017,7 @@ record_btrace_start_replaying (struct
>> thread_info *tp)
>> >
>> >    /* We can't start replaying without trace.  */
>> >    if (btinfo->functions.empty ())
>> > -    return NULL;
>> > +    error (_("No trace."));
>> >
>> >    /* GDB stores the current frame_id when stepping in order to detects
>> steps
>> >       into subroutines.
>> 
>> Please can you add a test for this change.
>> 
>> Thanks,
>> Andrew
>
> Mhm, I don't think there is an actual test I can write to trigger this.
> All callers prevent it from happening by checking btinfo->replay.

I'd think an assert would be a better choice, but if you really prefer
the error message then I'm not going to block the patch.

Thanks,
Andrew


> I am just making it more consistent, as the function already
> errors out in the case of just gaps. Another advantage is that this function
> doesn't return NULL anymore, which seems better for future callers.
>
> Thanks,
> Felix
> 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