Re: [PATCH 2/2] OpenMP parallel region scope tests

[Pedro Alves <palves@redhat.com>]

Hi,

On 8/22/19 6:14 PM, Kevin Buettner wrote:

> 
> Note that the various counts for F27-F30 w/o my work only add up to 150
> instead of 151.  This is due to an extra test (which results in a PASS)
> from running openmp_setup.
> 
> Also, I have observed runs which show two fewer passes when not
> testing against a GDB (plus libgomp w/ plugin) which does not include

I guess you meant "when testing" instead of "when not testing"?


> my OpenMP work.  This is due to the fact that only one stop is made
> in the nested_parallel:outer_threads tests.  When it happens that the
> first (and only) stop occurs for a non-master thread, two KFAILs
> result instead of a PASS.

> +void
> +multi_scope (void)
> +{
> +  int i01 = 1, i02 = 2;
> +
> +  {
> +    int i11 = 11, i12 = 12;
> +
> +    {
> +      int i21 = -21, i22 = 22;
> +
> +#pragma omp parallel num_threads (2) \
> +                     firstprivate (i01) \
> +                     shared (i11) \
> +		     private (i21)

Something odd with indentation here.  Tabs vs spaces?

> diff --git a/gdb/testsuite/gdb.threads/omp-par-scope.exp b/gdb/testsuite/gdb.threads/omp-par-scope.exp
> new file mode 100644
> index 0000000000..719a6123a9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/omp-par-scope.exp
> @@ -0,0 +1,286 @@
> +# Copyright 2017-2019 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License 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 file is part of the gdb testsuite.
> +

Missing short intro here.

> +standard_testfile
> +
> +if {[gdb_compile_openmp "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != ""} {
> +    untested "failed to compile OpenMP program"
> +    return -1
> +}
> +
> +clean_restart ${binfile}


How about adding an "openmp" option to build_executable_from_specs
so that you can write:

  if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} {openmp debug}] } {

instead of all the above?

> +
> +# openmp_setup may be defined to set auto-load safe-path and possibly
> +# sysroot.  These settings are required for gdb to be able to find
> +# the libgomp python plugin.  (sysroot only needs to be defined for
> +# remote debugging.)
> +#
> +# This approach has both pros and cons.  On the plus side, it's easy
> +# to automatically set a precise auto-load safe-path.  (It's easy because
> +# the output of ldd on the binary may be examined to learn the location
> +# of libgomp.so.)
> +# 
> +# However, making these settings is also a drawback due to potentially
> +# overriding settings made by a board file.  So, for the moment
> +# anyway, this proc is optional and will only be called if it's
> +# defined.

I wonder, should we document this hook somewhere?  Maybe
gdb/testsuite/README?  Or would you rather keep it more internal
for the moment?  I'm fine with that, but I thought I'd ask.

Also, maybe rename it to gdb_openmp_setup?  As is seems just a little
bit too generic.

> +
> +if {[info procs openmp_setup] != ""} {
> +    if {[openmp_setup $binfile] != ""} {
> +	return -1
> +    }
> +}
> +


> +if {![runto_main]} {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# We want to invoke setup_kfail (and in some cases setup_xfail) when
> +# GDB does not yet have support for finding the values of variables in
> +# (non-master) threads.  We'll check this by looking at the output of
> +# "maint print thread-parent".  If this command is undefined, then GDB
> +# does not yet have thread parent support, and it makes sense to kfail
> +# tests which won't work.  It's possible for GDB to have this support,
> +# but not work.  E.g. it may be the case that the plugin doesn't
> +# exist or is not found.  We may eventually need to add additional
> +# constraints related to setting allow_kfail to 0.  But, for the moment,
> +# this simple test should be sufficient.
> +
> +set allow_kfail 1
> +gdb_test_multiple "maint print thread-parent" "maint print thread-parent" {
> +    -re "Undefined maintenance print command.*$gdb_prompt" {
> +        pass "maint print thread-parent does not exist"

All pass/fail calls should have the same message/name.
You can add extra info in parens if you want, though, like:

        pass "maint print thread-parent (does not exist)"

> +    }
> +    -re "No parent found.*" {
> +        pass "maint print thread-parent"
> +	set allow_kfail 0

Odd indentation.  Tabs vs spaces?

> +    }
> +    -re ".*$gdb_prompt" {
> +        fail "maint print thread-parent"
> +    }

This last case is unnecessary, because it's implicit, right?

> +
> +# Nested functions in C are a GNU extension, so don't do the nest_func
> +# tests unless the compiler is GCC.
> +
> +if [test_compiler_info gcc*] {

Other compilers may implement the extension as well.  The Intel compiler
does, I think, for example.  I think a better approach is to check some
symbol or value in the program.  

Also, I think clang does not support nested functions, but it defines
__GNUC__.  Could you run the test against clang too, please?

Thanks,
Pedro Alves