Re: [PATCH master+7.12 v2 3/3] Add test for user context selection sync

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2016-09-14 15:31, Pedro Alves wrote:
> Hi Simon,
> 
> I didn't try to understand everything in detail, but overall
> it looks very nice now.  Thank you very much.  A few comments below.

Oops, while trying to rerun the full testsuite, I noticed that it leaves 
"-ex \"set non-stop 1\"" in the GDBFLAGS... that's not good.  You can 
see it easily if you swap the order of the "all-stop" and "non-stop" 
variations.  Can you spot why the save_vars isn't working as intended?  
When I use a temporary variable instead, it works as intended...

> On 09/14/2016 06:45 PM, Simon Marchi wrote:
>> From: Antoine Tremblay <antoine.tremblay@ericsson.com>
>> 
>> This patch adds a test to verify that events are sent properly to all
>> UIs when the user selection context (inferior, thread, frame) changes.
>> 
>> The goal of the C test file is to provide two threads that are
>> interrupted with the same predictable backtrace (so that we can test
>> frame switching).  This is achieved by having them loop on a single
>> line, such that when the main thread hits a breakpoint, they are both
>> stopped that line.  It would not be practical to have the threads 
>> sleep,
>> since their backtraces would not be predictable (the functions that
>> implement sleep may vary between systems).
>> 
>> There is a 1 second sleep in the main thread to make sure the threads
>> have time to spawn and reach the loop.  If you can find a way that is
>> sleep-free and race-free to achieve the same result, it would be 
>> really
>> nice, as most of the time taken by the test is spent sleeping.
> 
> Did you try using barriers and breakpoints?  Several tests use that
> to make sure threads are past a point.

I tried, but the issue is that depending on the scheduling, the threads 
might still be in the pthread_barrier_wait function when you stop.

Consider this pseudo-code:

thread_function:
   barrier_wait
   infinite_loop # This is where I want the threads to be stopped.

main_thread ():
   initialize_barrier with n = 3
   spawn_thread (thread_function)
   spawn_thread (thread_function)
   barrier_wait
   breakpoint

Once the main thread hits the breakpoint, we have the assurance that the 
threads have started, but we don't know where they are.  They might not 
have exited the barrier_wait function, or they might be in the infinite 
loop.  Adding a sleep before the breakpoint in the main thread is the 
only way to be reasonnably sure (assuming 1 second is enough...) that 
both threads will have reached the infinite loop.

Actually, it might work by putting thread-specific breakpoints on the 
single-line infinite loop, then doing two "continue".  This way I think 
we would be guaranteed that the two threads stop exactly at that line.  
With a regular breakpoint it might not work, since a thread could hit 
the breakpoint twice while the other still hasn't reached it.

>> +int
>> +main (void)
>> +{
>> +  int i = 0;
>> +  pthread_t threads[NUM_THREADS];
>> +
>> +  for (i = 0; i < NUM_THREADS; i++)
>> +    pthread_create (&threads[i], NULL, child_function, NULL);
>> +
>> +  /* Leave enough time for the threads to reach their infinite loop. 
>> */
>> +  sleep (1);
>> +
>> +  i = 0; /* main break line */
>> +
>> +  sleep (2);
>> +
>> +  /* Allow the test to exit cleanly.  */
>> +  quit = 1;
>> +
>> +  for (i = 0; i < NUM_THREADS; i++)
>> +    pthread_join (threads[i], NULL);
> 
> 
> Hmm, looks like this version of the test still runs forever.

I don't think so, the main thread sets the quit flag which unblocks the 
threads.  If you run the executable you'll see it exits in ~2 seconds.

>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see 
>> <http://www.gnu.org/licenses/>.
>> +
>> +# This test checks that thread, select-frame, frame or inferior 
>> selection
>> +# events are properly sent to all uis.
> 
> This comment is talking about "select-frame", which I take is
> the CLI command's name, not a selection event.  It feels like the 
> comment
> could be tweaked to be a bit clearer.  Something like:
> 
> # This test checks that the "thread", "select-frame", "frame" and 
> "inferior"
> # commands send the appropriate user-selection-change events to all 
> UIs.
> 
> Maybe say something about MI commands too, if the test exercises them.

Good idea.  It became this:

# This test checks that the "thread", "select-frame", "frame" and 
"inferior"
# CLI commands, as well as the "-thread-select" and 
"-stack-select-frame" MI
# commands send the appropriate user-selection-change events to all UIs.

>> +#
>> +# This test considers the case where console and mi are two different 
>> uis
>> +# and mi is created with the new-ui command.
>> +#
>> +# It also considers the case where the console commands are sent 
>> directly in
>> +# the mi channel as described in PR 20487.
> 
> Could you do a quick skim over the test and uppercase "mi" and "ui",
> as in "MI", and "UIs".  IMO, that makes the comments easier to grok.

I think that too, I changed them when I saw them, but I must've missed 
some.  I think I got them all now.  I changed those in the test messages 
too.

> 
>> +# Continue inferior INF until the breakpoint indicating the threads 
>> are started.
>> +
>> +proc test_continue_to_start { mode inf } {
>> +    global gdb_prompt gdb_spawn_id gdb_main_spawn_id
>> +
>> +    if { $gdb_spawn_id != $gdb_main_spawn_id } {
>> +	error "This should not happen."
>> +    }
>> +
>> +    with_test_prefix "inferior $inf" {
>> +	with_spawn_id $gdb_main_spawn_id {
>> +	    gdb_continue_to_breakpoint "main breakpoint"
>> +
>> +	    if { $mode == "non-stop" } {
>> +		gdb_test "thread $inf.2" ".*" "switch to thread $inf.2"
>> +
>> +		send_gdb "interrupt\n"
>> +		gdb_expect {
> 
> gdb_test_multiple ?

Like this?

     set test "interrupt thread $inf.2"

     send_gdb "interrupt\n"
     gdb_test_multiple "" $test {
         -re "Thread.*2.*stopped" {
             pass $test
         }
     }

>> +
>> +# Match a regular expression, or ensure that there was no output.
>> +#
>> +# If RE is non-empty, try to match the content of the program output 
>> (using the
>> +# current spawn_id) and pass/fail TEST accordingly.
>> +# If RE is empty, ensure that the program did not output anything.
>> +
>> +proc match_re_or_ensure_not_output { re test } {
>> +    if { $re != "" } {
>> +	gdb_expect {
> 
> gdb_test_multiple?  Or maybe this is used by MI too?

Indeed it it used by both.

>> +    with_test_prefix "thread 1.2 with --thread" {
>> +	# Test selecting a thread from MI with a --thread option.  This test
>> +	# verifies that even if the thread GDB would switch to is the same 
>> has
>> +	# the thread specified with --thread, an event is still sent to CLI.
>> +	# In this case this is thread 1.2
>> +
>> +	set mi_re [make_mi_re $mode 2 0 response]
>> +	set cli_re [make_cli_re $mode -1 1.2 0]
>> +
>> +	with_spawn_id $mi_spawn_id {
>> +	    mi_gdb_test "-thread-select --thread 2 2" $mi_re 
>> "-thread-select"
>> +	}
>> +
>> +	with_spawn_id $gdb_main_spawn_id {
>> +	    # TODO: it doesn't work as of now.
>> +	    # match_re_or_ensure_not_output "$cli_re\r\n" "-thread-select, 
>> event on cli"
>> +	}
> 
> Is there a plan here?

I think that will go in the same basket as the fact that any MI command 
with --thread currently changes the selected thread silently (without 
any =thread-selected event).  Currently, --thread changes the thread tot 
he desired one, then when the mi_cmd_thread_select tries to change the 
thread, it thinks that it was already the current thread, so that an 
event isn't necessary.  This should get fixed in the next iteration, 
when we split the concepts of user-selected-ptid and 
internally-selected-ptid.  Specifying --thread won't mess with the 
user-selected-ptid, but if you do "-thread-select --thread 2 2", then 
mi_cmd_thread_select will change the user-selected-ptid, generating an 
event.

It's not pretty to leave it like this in the test though.  Should I 
create a bug right now and kfail it?  Leave it commented out but put a 
better description?

Thanks,

Simon