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

[Pedro Alves <palves@redhat.com>]

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.

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.

> +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.


e for more details.
> +#
> +# 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.

> +#
> +# 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.


> +# 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 ?

> +		    -re "Thread.*2.*stopped" {
> +			pass "interrupt thread $inf.2"
> +		    }
> +		}
> +	    }
> +	}
> +    }
> +}


> +
> +# 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?

> +	    -re "$re" {
> +		pass $test
> +	    }
> +
> +	    default {
> +		fail $test
> +	    }
> +	}
> +    } else {
> +	ensure_no_output $test
> +    }
> +}


> +    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?

-- 
Pedro Alves