[PATCH 0/2] OpenMP parallel region scope tests

[PATCH 0/2] OpenMP parallel region scope tests

[Kevin Buettner]

This two part series adds parallel region scope tests for OpenMP
programs.

Kevin Buettner (2):
  Add gdb_compile_openmp to lib/gdb.exp
  OpenMP parallel region scope tests

 gdb/testsuite/gdb.threads/omp-par-scope.c   | 162 +++++++++++
 gdb/testsuite/gdb.threads/omp-par-scope.exp | 286 ++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                   |  20 ++
 3 files changed, 468 insertions(+)
 create mode 100644 gdb/testsuite/gdb.threads/omp-par-scope.c
 create mode 100644 gdb/testsuite/gdb.threads/omp-par-scope.exp

-- 
2.21.0

[PATCH 1/2] Add gdb_compile_openmp to lib/gdb.exp

[Kevin Buettner]

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_compile_openmp): New proc.
---
 gdb/testsuite/lib/gdb.exp | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index af56e8aa12..c8bca2f277 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4053,6 +4053,26 @@ proc gdb_compile_objc {source dest type options} {
     }
 }
 
+# Build an OpenMP program from SOURCE.  See prefatory comment for
+# gdb_compile, above, for discussion of the parameters to this proc.
+
+proc gdb_compile_openmp {source dest type options} {
+    switch -glob [test_compiler_info] {
+        "gcc-*" {
+            if { !([istarget "*-*-cygwin*"]
+                   || [istarget "*-*-mingw*"]
+                   || [istarget "*-*-pe*"]) } {
+                lappend options "additional_flags=-fopenmp"
+            }
+        }
+        default {
+	    # Don't know what flags to pass for this compiler
+        }
+    }
+
+    return [gdb_compile $source $dest $type $options]
+}
+
 # Send a command to GDB.
 # For options for TYPE see gdb_stdin_log_write
 
-- 
2.21.0

Re: [PATCH 1/2] Add gdb_compile_openmp to lib/gdb.exp

[Christian Biesinger via gdb-patches]

On Thu, Aug 22, 2019 at 12:14 PM Kevin Buettner <kevinb@redhat.com> wrote:
>
> gdb/testsuite/ChangeLog:
>
>         * lib/gdb.exp (gdb_compile_openmp): New proc.
> ---
>  gdb/testsuite/lib/gdb.exp | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index af56e8aa12..c8bca2f277 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4053,6 +4053,26 @@ proc gdb_compile_objc {source dest type options} {
>      }
>  }
>
> +# Build an OpenMP program from SOURCE.  See prefatory comment for
> +# gdb_compile, above, for discussion of the parameters to this proc.
> +
> +proc gdb_compile_openmp {source dest type options} {
> +    switch -glob [test_compiler_info] {
> +        "gcc-*" {
> +            if { !([istarget "*-*-cygwin*"]
> +                   || [istarget "*-*-mingw*"]
> +                   || [istarget "*-*-pe*"]) } {
> +                lappend options "additional_flags=-fopenmp"

Why exclude cygwin/mingw here?

> +            }
> +        }
> +        default {
> +           # Don't know what flags to pass for this compiler
> +        }
> +    }
> +
> +    return [gdb_compile $source $dest $type $options]
> +}
> +
>  # Send a command to GDB.
>  # For options for TYPE see gdb_stdin_log_write
>
> --
> 2.21.0
>

Re: [PATCH 1/2] Add gdb_compile_openmp to lib/gdb.exp

[Kevin Buettner]

On Thu, 22 Aug 2019 12:18:13 -0500
Christian Biesinger <cbiesinger@google.com> wrote:

> > +# Build an OpenMP program from SOURCE.  See prefatory comment for
> > +# gdb_compile, above, for discussion of the parameters to this proc.
> > +
> > +proc gdb_compile_openmp {source dest type options} {
> > +    switch -glob [test_compiler_info] {
> > +        "gcc-*" {
> > +            if { !([istarget "*-*-cygwin*"]
> > +                   || [istarget "*-*-mingw*"]
> > +                   || [istarget "*-*-pe*"]) } {
> > +                lappend options "additional_flags=-fopenmp"  
> 
> Why exclude cygwin/mingw here?

No good reason.  I've removed those lines from my local sources.

Thanks for noticing this.

Kevin

[PATCH 2/2] OpenMP parallel region scope tests

[Kevin Buettner]

Add tests which check for accessibility of variables from within
various OpenMP parallel regions.

Tested on Fedora 27, 28, 29, and 30.  I also tested with my OpenMP
work on Fedora 30.  The test has been annotated with setup_xfail and
setup_kfail statements so that there are no unexpected failures on any
of these platforms.  Better still, for my own testing anyway, is that
there are also no XPASSes or KPASSes either.  So, regardless of platform,
and regardless of whether my (not yet public) OpenMP work is used,
seeing a FAIL indicates a real problem.

Fedora 27 results (with just this test):

        # of expected passes            85
        # of expected failures          65

Fedora 28, 29, 30 results:

        # of expected passes            130
        # of expected failures          4
        # of known failures             16

Fedora 30 results w/ my OpenMP work:

        # of expected passes            151

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

I think it's also possible, when using my current OpenMP work, that
some failures may occur for some of the nested_parallel:inner_threads
tests.  While I have not observed these failures recently, these can
occur because I do not yet have a solution for accurately finding the
thread parent when stopping within nested parallel regions.

gdb/testsuite/ChangeLog:

	* gdb.threads/omp-par-scope.c: New file.
	* gdb/threads/omp-par-scope.exp: New file.
---
 gdb/testsuite/gdb.threads/omp-par-scope.c   | 162 +++++++++++
 gdb/testsuite/gdb.threads/omp-par-scope.exp | 286 ++++++++++++++++++++
 2 files changed, 448 insertions(+)
 create mode 100644 gdb/testsuite/gdb.threads/omp-par-scope.c
 create mode 100644 gdb/testsuite/gdb.threads/omp-par-scope.exp

diff --git a/gdb/testsuite/gdb.threads/omp-par-scope.c b/gdb/testsuite/gdb.threads/omp-par-scope.c
new file mode 100644
index 0000000000..f9faff1ea5
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/omp-par-scope.c
@@ -0,0 +1,162 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+#include <stdio.h>
+#include <omp.h>
+
+/* Testcase for checking access to variables in a single / outer scope.
+   Make sure that variables not referred to in the parallel section are
+   accessible from the debugger.  */
+
+void
+single_scope (void)
+{
+  static int s1 = -41, s2 = -42, s3 = -43;
+  int i1 = 11, i2 = 12, i3 = 13;
+
+#pragma omp parallel num_threads (2) shared (s1, i1) private (s2, i2)
+  {
+    int thread_num = omp_get_thread_num ();
+
+    s2 = 100 * (thread_num + 1) + 2;
+    i2 = s2 + 10;
+
+    #pragma omp critical
+    printf ("single_scope: thread_num=%d, s1=%d, i1=%d, s2=%d, i2=%d\n",
+            thread_num, s1, i1, s2, i2);
+  }
+
+  printf ("single_scope: s1=%d, s2=%d, s3=%d, i1=%d, i2=%d, i3=%d\n",
+          s1, s2, s3, i1, i2, i3);
+}
+
+static int file_scope_var = 9876;
+
+/* Testcase for checking access to variables from parallel region
+   nested within more than one lexical scope.  Of particular interest
+   are variables which are not referenced in the parallel section.  */
+
+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)
+	{
+	  int thread_num = omp_get_thread_num ();
+	  i21 = 100 * (thread_num + 1) + 21;
+
+	  #pragma omp critical
+	  printf ("multi_scope: thread_num=%d, i01=%d, i11=%d, i21\n",
+		  thread_num, i01, i11, i21);
+	}
+
+	printf ("multi_scope: i01=%d, i02=%d, i11=%d, "
+	        "i12=%d, i21=%d, i22=%d\n",
+		i01, i02, i11, i12, i21, i22);
+    }
+  }
+}
+
+/* Nested functions in C is a GNU extension.  */
+#ifdef __GNUC__
+
+/* Testcase for checking access of variables from within parallel
+   region in a lexically nested function.  */
+
+void
+nested_func (void)
+{
+  static int s1 = -42;
+  int i = 1, j = 2, k = 3;
+
+  void
+  foo (int p, int q, int r)
+  {
+    int x = 4;
+
+    {
+      int y = 5, z = 6;
+#pragma omp parallel num_threads (2) shared (i, p, x) private (j, q, y)
+      {
+	int tn = omp_get_thread_num ();
+
+	j = 1000 * (tn + 1);
+	q = j + 1;
+	y = q + 1;
+	#pragma omp critical
+	printf ("nested_func: tn=%d: i=%d, p=%d, x=%d, j=%d, q=%d, y=%d\n",
+		 tn, i, p, x, j, q, y);
+      }
+    }
+  }
+
+  foo (10, 11, 12);
+
+  i = 101; j = 102; k = 103;
+  foo (20, 21, 22);
+}
+#endif
+
+/* Testcase for checking access to variables from within a nested parallel
+   region. */
+
+void
+nested_parallel (void)
+{
+  int i = 1, j = 2;
+  int l = -1;
+
+  omp_set_nested (1);
+  omp_set_dynamic (0);
+#pragma omp parallel num_threads (2) private (l)
+  {
+    int num = omp_get_thread_num ();
+    int nthr = omp_get_num_threads ();
+    int off = num * nthr;
+    int k = off + 101;
+    l = off + 102;
+#pragma omp parallel num_threads (2) shared (num)
+    {
+      int inner_num = omp_get_thread_num ();
+      #pragma omp critical
+      printf ("nested_parallel (inner threads): outer thread num = %d, thread num = %d\n", num, inner_num);
+    }
+    #pragma omp critical
+    printf ("nested_parallel (outer threads) %d: k = %d, l = %d\n", num, k, l);
+  }
+}
+
+int
+main (int argc, char **argv)
+{
+  single_scope ();
+  multi_scope ();
+  nested_func ();
+  nested_parallel ();
+  return 0;
+}
+
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.
+
+standard_testfile
+
+if {[gdb_compile_openmp "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != ""} {
+    untested "failed to compile OpenMP program"
+    return -1
+}
+
+clean_restart ${binfile}
+
+# 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.
+
+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"
+    }
+    -re "No parent found.*" {
+        pass "maint print thread-parent"
+	set allow_kfail 0
+    }
+    -re ".*$gdb_prompt" {
+        fail "maint print thread-parent"
+    }
+}
+
+# Determine whether to xfail some of the tests based on GCC version.
+#
+# This may need to be tweaked somewhat.  Testing shows that GCC 7.3.1
+# needs the xfails.  GCC 8.3.1 and 9.1.1 do not.  The assumption made
+# below is that all versions of gcc 8 and above won't require the
+# XFAIL setup and that all versions of gcc 7 and below will, but it's
+# possible that there are versions in between 7.3.1 and 8.3.1 for
+# which this assumption is invalid.
+
+set have_older_gcc 0
+if {[test_compiler_info {gcc-[0-7]-*}]} {
+    set have_older_gcc 1
+}
+
+# maybe_setup_kfail will set up a kfail for gdb/22214 when COND holds in
+# addition to considering the values of $have_older_gcc and $allow_kfail.
+#
+# When $have_older_gcc evaluates to true, setup_xfail will invoked
+# instead.
+
+proc maybe_setup_kfail {cond} {
+    global have_older_gcc allow_kfail
+    if {$have_older_gcc} {
+	setup_xfail *-*-*
+    } elseif {[uplevel 1 [list expr $cond]] && $allow_kfail} {
+	setup_kfail "gdb/22214" *-*-*
+    }
+}
+
+with_test_prefix "single_scope" {
+
+    gdb_breakpoint [gdb_get_line_number "single_scope: thread_num="]
+    gdb_breakpoint [gdb_get_line_number "single_scope: s1="]
+
+    foreach pref {"first thread" "second thread"} {
+	with_test_prefix $pref {
+	    gdb_continue_to_breakpoint "at printf"
+
+	    if {$have_older_gcc} { setup_xfail "*-*-*" }
+	    set thread_num [get_valueof "" thread_num "unknown"]
+	    if {$have_older_gcc} { setup_xfail "*-*-*" }
+	    gdb_test "print s1" "= -41"
+	    gdb_test "print s2" "= \[12\]02"
+	    if {$have_older_gcc} { setup_xfail "*-*-*" }
+	    gdb_test "print s3" "= -43"
+	    gdb_test "print i1" "= 11"
+	    gdb_test "print i2" "= \[12]12"
+	    maybe_setup_kfail {$thread_num != 0}
+	    gdb_test "print i3" "= 13"
+	}
+    }
+
+    with_test_prefix "after parallel region" {
+	gdb_continue_to_breakpoint "at printf"
+
+	gdb_test "print s1" "= -41"
+	gdb_test "print s2" "= -42"
+	gdb_test "print s3" "= -43"
+	gdb_test "print i1" "= 11"
+	gdb_test "print i2" "= 12"
+	gdb_test "print i3" "= 13"
+    }
+
+}
+
+with_test_prefix "multi_scope" {
+    gdb_breakpoint [gdb_get_line_number "multi_scope: thread_num="]
+    gdb_breakpoint [gdb_get_line_number "multi_scope: i01="]
+
+    foreach pref {"first thread" "second thread"} {
+	with_test_prefix $pref {
+	    gdb_continue_to_breakpoint "at printf"
+
+	    if {$have_older_gcc} { setup_xfail "*-*-*" }
+	    set thread_num [get_valueof "" thread_num "unknown"]
+
+	    gdb_test "print i01" "= 1"
+	    maybe_setup_kfail {$thread_num != 0}
+	    gdb_test "print i02" "= 2"
+	    gdb_test "print i11" "= 11"
+	    maybe_setup_kfail {$thread_num != 0}
+	    gdb_test "print i12" "= 12"
+	    gdb_test "print i21" "= \[12\]21"
+	    maybe_setup_kfail {$thread_num != 0}
+	    gdb_test "print i22" "= 22"
+	    gdb_test "print file_scope_var" "= 9876"
+	}
+    }
+
+    with_test_prefix "after parallel" {
+	gdb_continue_to_breakpoint "at printf"
+
+	gdb_test "print i01" "= 1"
+	gdb_test "print i02" "= 2"
+	gdb_test "print i11" "= 11"
+	gdb_test "print i12" "= 12"
+	gdb_test "print i21" "= -21"
+	gdb_test "print i22" "= 22"
+	gdb_test "print file_scope_var" "= 9876"
+    }
+}
+
+# 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*] {
+    with_test_prefix "nested_func" {
+	gdb_breakpoint [gdb_get_line_number "nested_func: tn="]
+
+	foreach call_prefix {"1st call" "2nd call"} {
+	    with_test_prefix $call_prefix {
+	        foreach thread_prefix {"1st thread" "2nd thread"} {
+	            with_test_prefix $thread_prefix {
+			gdb_continue_to_breakpoint "at printf"
+
+			if {$have_older_gcc} { setup_xfail "*-*-*" }
+			set thread_num [get_valueof "" "tn" "unknown"]
+
+			gdb_test "print file_scope_var" "= 9876"
+			if {$have_older_gcc} { setup_xfail *-*-* }
+			gdb_test "print s1" "= -42"
+			if {$call_prefix eq "1st call"} {
+			    gdb_test "print i" "= 1"
+			} else {
+			    gdb_test "print i" "= 101"
+			}
+			gdb_test "print j" "= \[12\]000"
+			maybe_setup_kfail {$thread_num != 0}
+			if {$call_prefix eq "1st call"} {
+			    gdb_test "print k" "= 3"
+			} else {
+			    gdb_test "print k" "= 103"
+			}
+			if {$call_prefix eq "1st call"} {
+			    gdb_test "print p" "= 10"
+			} else {
+			    gdb_test "print p" "= 20"
+			}
+			gdb_test "print q" "= \[12\]001"
+			maybe_setup_kfail {$thread_num != 0}
+			if {$call_prefix eq "1st call"} {
+			    gdb_test "print r" "= 12"
+			} else {
+			    gdb_test "print r" "= 22"
+			}
+			gdb_test "print x" "= 4"
+			gdb_test "print y" "= \[12\]002"
+			maybe_setup_kfail {$thread_num != 0}
+			gdb_test "print z" "= 6"
+			if {$have_older_gcc} { setup_xfail "*-*-*" }
+			gdb_test "print tn" "= \[01\]"
+		    }
+		}
+	    }
+	}
+    }
+}
+
+with_test_prefix "nested_parallel" {
+    gdb_breakpoint [gdb_get_line_number "nested_parallel (inner threads)"]
+
+    with_test_prefix "inner_threads" {
+	foreach pref {"1st stop" "2nd stop" "3rd stop" "4th stop"} {
+	    with_test_prefix $pref {
+		gdb_continue_to_breakpoint "at printf"
+
+		# Don't need setup_xfail here due to fact that num is made
+		# made known to the inner parallel region.
+		set thread_num [get_valueof "" "num" "unknown"]
+
+		if {$have_older_gcc} { setup_xfail "*-*-*" }
+		set inner_thread_num [get_valueof "" "inner_num" "unknown"]
+
+		gdb_test "print file_scope_var" "= 9876"
+		gdb_test "print num" "= \[01\]"
+		maybe_setup_kfail {$thread_num != 0 || $inner_thread_num != 0}
+		gdb_test "print i" "= 1"
+		maybe_setup_kfail {$thread_num != 0 || $inner_thread_num != 0}
+		gdb_test "print j" "= 2"
+		if {$have_older_gcc || ($inner_thread_num != 0 && $allow_kfail)} { setup_xfail *-*-* }
+		gdb_test "print l" "= 10\[24\]"
+		if {$have_older_gcc ||( $inner_thread_num != 0 && $allow_kfail)} { setup_xfail *-*-* }
+		gdb_test "print k" "= 10\[13\]"
+	    }
+	}
+    }
+
+    with_test_prefix "outer_threads" {
+	gdb_breakpoint [gdb_get_line_number "nested_parallel (outer threads)"]
+
+	with_test_prefix "outer stop" {
+	    gdb_continue_to_breakpoint "at printf"
+
+	    if {$have_older_gcc} { setup_xfail "*-*-*" }
+	    set thread_num [get_valueof "" "num" "unknown"]
+
+	    gdb_test "print file_scope_var" "= 9876"
+	    if {$have_older_gcc} { setup_xfail "*-*-*" }
+	    gdb_test "print num" "= \[01\]"
+	    maybe_setup_kfail {$thread_num != 0}
+	    gdb_test "print i" "= 1"
+	    maybe_setup_kfail {$thread_num != 0}
+	    gdb_test "print j" "= 2"
+	    gdb_test "print l" "= 10\[24\]"
+	    if {$have_older_gcc} { setup_xfail "*-*-*" }
+	    gdb_test "print k" "= 10\[13\]"
+	}
+    }
+}
-- 
2.21.0

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

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

Kevin> Add tests which check for accessibility of variables from within
Kevin> various OpenMP parallel regions.

I don't know anything about OpenMP, so I didn't read new .c file.
I'm not especially concerned about it though.

Kevin> +if {[gdb_compile_openmp "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != ""} {

Over-long line.

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

Does this need a call to "untested"?

The rest seemed fine to me.

Tom

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

[Kevin Buettner]

On Mon, 30 Sep 2019 10:22:40 -0600
Tom Tromey <tom@tromey.com> wrote:

> >>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:  
> 
> Kevin> Add tests which check for accessibility of variables from within
> Kevin> various OpenMP parallel regions.  
> 
> I don't know anything about OpenMP, so I didn't read new .c file.
> I'm not especially concerned about it though.
> 
> Kevin> +if {[gdb_compile_openmp "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != ""} {  
> 
> Over-long line.
> 
> Kevin> +if {[info procs openmp_setup] != ""} {
> Kevin> +    if {[openmp_setup $binfile] != ""} {
> Kevin> +	return -1  
> 
> Does this need a call to "untested"?
> 
> The rest seemed fine to me.

Thanks for the review.  I've addressed both of your concerns in
my most recent patch submitted via gerrit:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/504

Kevin

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

[Pedro Alves]

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

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

[Kevin Buettner]

On Wed, 2 Oct 2019 17:52:52 +0100
Pedro Alves <palves@redhat.com> 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"?

I think so, but am no longer certain.

I ended up removing that entire paragraph.  I think it was getting too
far into the weeds.

> > +#pragma omp parallel num_threads (2) \
> > +                     firstprivate (i01) \
> > +                     shared (i11) \
> > +		     private (i21)  
> 
> Something odd with indentation here.  Tabs vs spaces?

Yes, fixed.

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

Fixed.

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

Done.

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

For the moment, I'd like to keep it internal.  We might go in a different
direction with the rest of my work; I don't see much point in documenting
something which might later need to be removed.

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

Done.

> > +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)"

Done.

> > +    -re "No parent found.*" {
> > +        pass "maint print thread-parent"
> > +	set allow_kfail 0  
> 
> Odd indentation.  Tabs vs spaces?

Yes, fixed.

> > +    }
> > +    -re ".*$gdb_prompt" {
> > +        fail "maint print thread-parent"
> > +    }  
> 
> This last case is unnecessary, because it's implicit, right?

Right.  I've removed that case.  (Fixed.)

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

I have now tested against both clang and icc.  Testing against clang
found some problems in the .c file that have now been fixed.  I now
have omp-par-scope.exp doing the following:

I've wrapped nested_func and its call with...

#if HAVE_NESTED_FUNCTION_SUPPORT
...
#endif

I now have the .exp file attempting to compile the file up to two times,
once with -DHAVE_NESTED_FUNCTION_SUPPORT and, if that fails, another
time without that -D switch.  If no nested function call support exists
(as a result of failing that first test), the corresponding tests
in the .exp file will not be run.

Thanks for your review.  I posted an updated patch using gerrit:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/504

Kevin

[PING][PATCH 0/2] OpenMP parallel region scope tests

[Kevin Buettner]

Ping.

On Thu, 22 Aug 2019 10:14:06 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> This two part series adds parallel region scope tests for OpenMP
> programs.
> 
> Kevin Buettner (2):
>   Add gdb_compile_openmp to lib/gdb.exp
>   OpenMP parallel region scope tests
> 
>  gdb/testsuite/gdb.threads/omp-par-scope.c   | 162 +++++++++++
>  gdb/testsuite/gdb.threads/omp-par-scope.exp | 286 ++++++++++++++++++++
>  gdb/testsuite/lib/gdb.exp                   |  20 ++
>  3 files changed, 468 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.threads/omp-par-scope.c
>  create mode 100644 gdb/testsuite/gdb.threads/omp-par-scope.exp
> 
> -- 
> 2.21.0
> 

[PING 2][PATCH 0/2] OpenMP parallel region scope tests

[Kevin Buettner]

Ping.

On Thu, 22 Aug 2019 10:14:06 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> This two part series adds parallel region scope tests for OpenMP
> programs.
> 
> Kevin Buettner (2):
>   Add gdb_compile_openmp to lib/gdb.exp
>   OpenMP parallel region scope tests
> 
>  gdb/testsuite/gdb.threads/omp-par-scope.c   | 162 +++++++++++
>  gdb/testsuite/gdb.threads/omp-par-scope.exp | 286 ++++++++++++++++++++
>  gdb/testsuite/lib/gdb.exp                   |  20 ++
>  3 files changed, 468 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.threads/omp-par-scope.c
>  create mode 100644 gdb/testsuite/gdb.threads/omp-par-scope.exp
> 
> -- 
> 2.21.0
>