Re: [PATCH] gcc parallel make check

[Christophe Lyon <christophe dot lyon at linaro dot org>]

Hi Jakub,


On 15 September 2014 18:05, Jakub Jelinek <jakub@redhat.com> wrote:
[...]
>  # For parallelized check-% targets, this decides whether parallelization
>  # is desirable (if -jN is used and RUNTESTFLAGS doesn't contain anything
>  # but optional --target_board or --extra_opts arguments).  If desirable,
>  # recursive make is run with check-parallel-$lang{,1,2,3,4,5} etc. goals,
>  # which can be executed in parallel, as they are run in separate directories.
> -# check-parallel-$lang{1,2,3,4,5} etc. goals invoke runtest with the longest
> -# running *.exp files from the testsuite, as determined by check_$lang_parallelize
> -# variable.  The check-parallel-$lang goal in that case invokes runtest with
> -# all the remaining *.exp files not handled by the separate goals.
> +# check-parallel-$lang{,1,2,3,4,5} etc. goals invoke runtest with
> +# GCC_RUNTEST_PARALLELIZE_DIR var in the environment and runtest_file_p
> +# dejaGNU procedure is overridden to additionally synchronize through
> +# a $lang-parallel directory which tests will be run by which runtest instance.
>  # Afterwards contrib/dg-extract-results.sh is used to merge the sum and log
>  # files.  If parallelization isn't desirable, only one recursive make
>  # is run with check-parallel-$lang goal and check_$lang_parallelize variable
> @@ -3662,76 +3645,60 @@ check_p_subdirs=$(wordlist 1,$(words $(c
>  # to lang_checks_parallelized variable and define check_$lang_parallelize
>  # variable (see above check_gcc_parallelize description).
>  $(lang_checks_parallelized): check-% : site.exp
> -       @if [ -z "$(filter-out --target_board=%,$(filter-out --extra_opts%,$(RUNTESTFLAGS)))" ] \

Since you removed this test, the comment above is not longer accurate:
setting RUNTESTFLAGS to whatever value no longer disables
parallelization.

Which leads me to discuss a bug I faced after you committed this
change: I am testing a patch which bring a series of new tests.
$ RUNTESTFLAGS=my.exp make -jN check (in fact the 'make -j' is
embedded in a larger build script)

my.exp contains the following construct which is often used in the testsuite:
==========
foreach src [lsort [glob -nocomplain $srcdir/$subdir/*.c]] {
    # If we're only testing specific files and this isn't one of them,
skip it.
    if ![runtest_file_p $runtests $src] then {
        continue
    }
    c-torture-execute $src $additional_flags
    gcc-dg-runtest $src "" $additional_flags
}
==========
Note that gcc-dg-runtest calls runtest_file_p too.

What I observed is that if I use -j1, all my .c files get tested,
while with N>2 some of them are silently skipped.

It took me a while to figure out that it's because gcc-dg-runtest
calls runtest_file_p, which means that runtest_file_p is called twice
when the 1st invocation returns 1, and only once when the 1st
invocation returns 0.

For example, if we have pid0, pid1 the concurrent runtest processes,
and file0.c, file1.c, .... the testcases, then:
* pid0 decides to keep file0.c file1.c file2.c file3.c file4.c. Since
the above loop calls runtest_file_p twice for each, we reach the
"minor" counter of 10.
* in the mean time, pid1 decides to skip file0.c, file1.c ... file9.c
since it calls runtest_file_p only once for each
* pid1 increments its parallel counter to 1, and create the new testing subdir
* pid1 decides to keep file10, file11, file12, file13 and file14
(again, 2 calls to runtest_file_p per testcase)
* pid0 increments its parallel counter to 1, and decides it has to skip it
* pid0 thus decides to skip file5, file6, file7, ... file14, calling
runtest_file_p once for each
* etc...

In the end, we have ignored file5...file9

I'm not sure why you have made special cases for some of the existing
*.exp when you forced them to disable parallelization.
Was it to handle such cases?

I'm not sure about the next step:
- should I modify my .exp file?
- should you modify gcc_parallel_test_run_p?

Even if I have to modify my .exp file, I think this is error prone,
and others could introduce a similar construct in the future.

Thanks,

Christophe.