RE: [PATCH 2/2] btrace: allow recording to be started for running threads

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Luis Machado [mailto:lgustavo@codesourcery.com]
> Sent: Wednesday, December 7, 2016 9:21 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 2/2] btrace: allow recording to be started for running threads

Hi Luis,

Thanks for your feedback.


> > +      /* This is not relevant for BTRACE_FORMAT_PT since the trace will already
> > +	 start at the enable location.  */
> 
> enabled location?

I rephrased the sentence to:

      /* This is not relevant for BTRACE_FORMAT_PT since the trace will already
	 start at the PC at which tracing was enabled.  */


> > +if { [skip_btrace_tests] } { return -1 }
> 
> unsupported "btrace not supported on this target"?

Let me do this for all gdb.btrace tests in a separate patch.


> > +    gdb_test "thread $thread" "(running).*"
> 
> Maybe add a more meaningful test name here? "Check if thread $thread is
> running"?

OK.  Also moved it below the with_test_prefix.  It now looks like this:
PASS: gdb.btrace/enable-running.exp: thread 1: is running


> > +    with_test_prefix "thread $thread" {
> > +        # Stop the thread before reading the trace.
> > +        gdb_test_multiple "interrupt" "interrupt" {
> > +            -re "interrupt\r\n$gdb_prompt " {
> > +                pass "interrupt"
> > +            }
> 
> Any reason why we couldn't use gdb_test here? Also, we a more meaningful
> test name and make the name unique (by mentioning the thread number,
> possibly)

The thread number is contained in the test prefix so the test name is unique.
PASS: gdb.btrace/enable-running.exp: thread 1: interrupt


Regarding the use of gdb_test ....

> > +        }
> > +        # Wait until the thread actually stopped.
> > +        gdb_test_multiple "" "stopped" {
> > +            -re "Thread $thread.*stopped\." {
> > +                pass "stopped"
> > +            }
> > +        }
>
> Same here. Couldn't we use gdb_test? Also a more meaningful and unique
> test name.

... I may be able to use it for the first part.  But not here.  This is non-stop mode
so we're getting an asynchronous stopped notification.  gdb_test expects a prompt
at the end but we already got the prompt and now need to consume the stopped
notification to ensure that the thread really stopped before we issue the next
command.

I'm not entirely sure about using gdb_test on the first part, either.  If we got the
stopped notification fast enough, I'm not sure whether the "...$gdb_prompt $"
pattern in gdb_test would still match.  According to my understanding of how
dejagnu works the test may fail sporadically since there is already more input
available after the prompt and the "... $" wouldn't match.

The test prefix makes the test name unique:
PASS: gdb.btrace/enable-running.exp: thread 1: stopped

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928