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

Pedro Alves
2016-09-14T19:31:40Z

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