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

[Kevin Buettner <kevinb@redhat.com>]

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