Re: [PATCH] Integrate PR 12649's race detector directly in the testsuite machinery

[Pedro Alves <palves at redhat dot com>]

On 08/15/2014 09:48 PM, Jan Kratochvil wrote:
> On Fri, 25 Jul 2014 20:47:08 +0200, Pedro Alves wrote:
>> --- /dev/null
>> +++ b/gdb/testsuite/read1.c
> 
> I would find it more suitable as: gdb/testsuite/lib/read1.c

Done.

>> +ssize_t
>> +read (int fd, void *buf, size_t count)
>> +{
>> +  static ssize_t (*read2) (int fd, void *buf, size_t count) = NULL;
>> +  if (read2 == NULL)
>> +    {
>> +      unsetenv ("LD_PRELOAD");
>> +      read2 = dlsym (RTLD_NEXT, "read");
>> +    }
>> +  if (count > 0 && isatty (fd) >= 1)
> 
> Here could be 'count > 1'.

Indeed.  Done.

> Otherwise the Makefiles work for me.

I've pushed this now, as below.

Thanks,
Pedro Alves

--------
Integrate PR 12649's race detector directly in the testsuite machinery

This integrates Jan Kratochvil's nice race reproducer from PR
testsuite/12649 into the testsuite infrustructure directly.

With this, one only has to do either 'make check-read1' or 'make check
READ1="1"' to preload the read1.so library into expect.

Currently only enabled for glibc/GNU systems, and if
build==host==target.

gdb/testsuite/ChangeLog:

	* Makefile.in (EXTRA_RULES, CC): New variables, get from
	configure.
	(EXPECT): Handle READ1 being set.
	(all): Depend on EXTRA_RULES.
	(check-read1, expect-read1, read1.so, read1): New rules.
	* README (Testsuite Parameters): Document the READ1 make variable.
	(Race detection): New section.
	* configure: Regenerate.
	* configure.ac: If build==host==target, and running under a
	GNU/glibc system, add read1 to the extra Makefile rules.
	(EXTRA_RULES): AC_SUBST it.
	* lib/read1.c: New file.

gdb/ChangeLog:

	* Makefile.in (check-read1): New rule.
---
 gdb/ChangeLog              |  4 ++++
 gdb/testsuite/ChangeLog    | 16 ++++++++++++++++
 gdb/Makefile.in            |  8 ++++++++
 gdb/testsuite/Makefile.in  | 44 ++++++++++++++++++++++++++++++++++++++++----
 gdb/testsuite/README       | 41 +++++++++++++++++++++++++++++++++++++++++
 gdb/testsuite/configure    | 10 ++++++++++
 gdb/testsuite/configure.ac |  9 +++++++++
 gdb/testsuite/lib/read1.c  | 40 ++++++++++++++++++++++++++++++++++++++++
 8 files changed, 168 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/lib/read1.c

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8c6b204..8bae7d3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2014-08-20  Pedro Alves  <palves@redhat.com>
+
+	* Makefile.in (check-read1): New rule.
+
 2014-08-20  Joel Brobecker  <brobecker@adacore.com>
 
 	* value.c (value_from_contents_and_address): Strip resolved_type's
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index ef024b9..a9205d1 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,19 @@
+2014-08-20  Pedro Alves  <palves@redhat.com>
+	    Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* Makefile.in (EXTRA_RULES, CC): New variables, get from
+	configure.
+	(EXPECT): Handle READ1 being set.
+	(all): Depend on EXTRA_RULES.
+	(check-read1, expect-read1, read1.so, read1): New rules.
+	* README (Testsuite Parameters): Document the READ1 make variable.
+	(Race detection): New section.
+	* configure: Regenerate.
+	* configure.ac: If build==host==target, and running under a
+	GNU/glibc system, add read1 to the extra Makefile rules.
+	(EXTRA_RULES): AC_SUBST it.
+	* lib/read1.c: New file.
+
 2014-08-20  Joel Brobecker  <brobecker@adacore.com>
 
 	* gdb.dwarf2/data-loc.exp: Add additional tests exercising
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index b33defe..f251627 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1098,6 +1098,14 @@ check-perf: force
 	  $(MAKE) $(TARGET_FLAGS_TO_PASS) check-perf; \
 	else true; fi
 
+check-read1: force
+	@if [ -f testsuite/Makefile ]; then \
+	  rootme=`pwd`; export rootme; \
+	  rootsrc=`cd $(srcdir); pwd`; export rootsrc; \
+	  cd testsuite; \
+	  $(MAKE) $(TARGET_FLAGS_TO_PASS) check-read1; \
+	else true; fi
+
 # The idea is to parallelize testing of multilibs, for example:
 #   make -j3 check//sh-hms-sim/{-m1,-m2,-m3,-m3e,-m4}/{,-nofpu}
 # will run 3 concurrent sessions of check, eventually testing all 10
diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 0020a0f..1c923cd 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -39,9 +39,17 @@ ALL_SUBDIRS = gdb.ada gdb.arch gdb.asm gdb.base gdb.btrace gdb.cell gdb.cp \
 	gdb.stabs gdb.reverse gdb.threads gdb.trace gdb.xml \
 	$(SUBDIRS)
 
-EXPECT = `if [ -f $${rootme}/../../expect/expect ] ; then \
-          echo $${rootme}/../../expect/expect ; \
-          else echo expect ; fi`
+EXTRA_RULES = @EXTRA_RULES@
+
+CC=@CC@
+
+EXPECT = `if [ "$${READ1}" != "" ] ; then \
+            echo $${rootme}/expect-read1; \
+          elif [ -f $${rootme}/../../expect/expect ] ; then \
+            echo $${rootme}/../../expect/expect ; \
+          else \
+            echo expect ; \
+          fi`
 
 RUNTEST = $(RUNTEST_FOR_TARGET)
 
@@ -84,7 +92,7 @@ TARGET_FLAGS_TO_PASS = \
         "RUNTEST=$(RUNTEST)" \
         "RUNTESTFLAGS=$(RUNTESTFLAGS)"
 
-all:
+all: $(EXTRA_RULES)
 	@echo "Nothing to be done for all..."
 
 .NOEXPORT:
@@ -146,6 +154,9 @@ installcheck:
 check: all $(abs_builddir)/site.exp
 	$(MAKE) $(CHECK_TARGET)
 
+check-read1:
+	$(MAKE) READ1="1" check
+
 # All the hair to invoke dejagnu.  A given invocation can just append
 # $(RUNTESTFLAGS)
 DO_RUNTEST = \
@@ -230,6 +241,7 @@ clean mostlyclean:
 	-rm -f core.* *.tf *.cl *.py tracecommandsscript copy1.txt zzz-gdbscript
 	-rm -f *.dwo *.dwp
 	-rm -rf outputs temp cache
+	-rm -f read1.so expect-read1
 	if [ x"${ALL_SUBDIRS}" != x ] ; then \
 	    for dir in ${ALL_SUBDIRS}; \
 	    do \
@@ -263,3 +275,27 @@ config.status: configure
 TAGS: force
 	find $(srcdir) -name '*.exp' -print | \
 	    etags --regex='/proc[ \t]+\([^ \t]+\)/\1/' -
+
+# Build the expect wrapper script that preloads the read1.so library.
+expect-read1:
+	@echo Making expect-read1
+	@rm -f expect-read1-tmp
+	@touch expect-read1-tmp
+	@echo "# THIS FILE IS GENERATED -*- buffer-read-only: t -*- \n" >>expect-read1-tmp
+	@echo "# vi:set ro: */\n\n" >>expect-read1-tmp
+	@echo "# To regenerate this file, run:\n" >>expect-read1-tmp
+	@echo "#      make clean; make/\n" >>expect-read1-tmp
+	@echo "export LD_PRELOAD=`pwd`/read1.so" >>expect-read1-tmp
+	@echo 'exec expect "$$@"' >>expect-read1-tmp
+	@chmod +x expect-read1-tmp
+	@mv expect-read1-tmp expect-read1
+
+# Build the read1.so preload library.  This overrides the `read'
+# function, making it read one byte at a time.  Running the testsuite
+# with this catches racy tests.
+read1.so: lib/read1.c
+	$(CC) -o $@ ${srcdir}/lib/read1.c -Wall -g -shared -fPIC $(CFLAGS)
+
+# Build the read1 machinery.
+.PHONY: read1
+read1: read1.so expect-read1
diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index 466993d..9a5059a 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -176,6 +176,47 @@ Example:
 If not using GNU make then the value is passed directly to runtest.
 If not specified, all tests are run.
 
+READ1
+
+This make (not runtest) variable is used to specify whether the
+testsuite preloads the read1.so library into expect.  Any non-empty
+value means true.  See "Race detection" below.
+
+Race detection
+**************
+
+The testsuite includes a mechanism that helps detect test races.
+
+For example, say the program running under expect outputs "abcd", and
+a test does something like this:
+
+  expect {
+    "a.*c" {
+    }
+    "b" {
+    }
+    "a" {
+    }
+  }
+
+Which case happens to match depends on what expect manages to read
+into its internal buffer in one go.  If it manages to read three bytes
+or more, then the first case matches.  If it manages to read two
+bytes, then the second case matches.  If it manages to read only one
+byte, then the third case matches.
+
+To help detect these cases, the race detection mechanism preloads a
+library into expect that forces the `read' system call to always
+return at most 1 byte.
+
+To enable this, either pass a non-empty value in the READ1 make
+variable, or use the check-read1 make target instead of check.
+
+Examples:
+
+	make -j10 check-read1 TESTS="*/paginate-*.exp"
+	make -j10 check READ1="1"
+
 Testsuite Configuration
 ***********************
 
diff --git a/gdb/testsuite/configure b/gdb/testsuite/configure
index 4bf6121..15944ed 100755
--- a/gdb/testsuite/configure
+++ b/gdb/testsuite/configure
@@ -591,6 +591,7 @@ ac_includes_default="\
 
 ac_subst_vars='LTLIBOBJS
 LIBOBJS
+EXTRA_RULES
 EGREP
 GREP
 CPP
@@ -3448,6 +3449,15 @@ done
 
 
 
+if test "${build}" = "${host}" -a "${host}" = "${target}"; then
+   case "${host}" in
+     *gnu*)
+       EXTRA_RULES=read1
+       ;;
+   esac
+fi
+
+
 ac_config_files="$ac_config_files Makefile gdb.ada/Makefile gdb.arch/Makefile gdb.asm/Makefile gdb.base/Makefile gdb.btrace/Makefile gdb.cell/Makefile gdb.cp/Makefile gdb.disasm/Makefile gdb.dwarf2/Makefile gdb.dlang/Makefile gdb.fortran/Makefile gdb.gdb/Makefile gdb.go/Makefile gdb.server/Makefile gdb.java/Makefile gdb.hp/Makefile gdb.hp/gdb.objdbg/Makefile gdb.hp/gdb.base-hp/Makefile gdb.hp/gdb.aCC/Makefile gdb.hp/gdb.compat/Makefile gdb.hp/gdb.defects/Makefile gdb.guile/Makefile gdb.linespec/Makefile gdb.mi/Makefile gdb.modula2/Makefile gdb.multi/Makefile gdb.objc/Makefile gdb.opencl/Makefile gdb.opt/Makefile gdb.pascal/Makefile gdb.perf/Makefile gdb.python/Makefile gdb.reverse/Makefile gdb.stabs/Makefile gdb.threads/Makefile gdb.trace/Makefile gdb.xml/Makefile"
 
 cat >confcache <<\_ACEOF
diff --git a/gdb/testsuite/configure.ac b/gdb/testsuite/configure.ac
index fb084d4..9dccc9f 100644
--- a/gdb/testsuite/configure.ac
+++ b/gdb/testsuite/configure.ac
@@ -87,6 +87,15 @@ AC_CHECK_HEADERS(pthread.h)
 
 AC_EXEEXT
 
+if test "${build}" = "${host}" -a "${host}" = "${target}"; then
+   case "${host}" in
+     *gnu*)
+       EXTRA_RULES=read1
+       ;;
+   esac
+fi
+AC_SUBST(EXTRA_RULES)
+
 AC_OUTPUT([Makefile \
   gdb.ada/Makefile \
   gdb.arch/Makefile gdb.asm/Makefile gdb.base/Makefile gdb.btrace/Makefile \
diff --git a/gdb/testsuite/lib/read1.c b/gdb/testsuite/lib/read1.c
new file mode 100644
index 0000000..5873ba2
--- /dev/null
+++ b/gdb/testsuite/lib/read1.c
@@ -0,0 +1,40 @@
+/* This is part of GDB, the GNU debugger.
+
+   Copyright 2011-2014 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/>.
+*/
+
+#define _GNU_SOURCE 1
+#include <dlfcn.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <stdlib.h>
+
+/* Wrap 'read', forcing it to return only one byte at a time, if
+   reading from the terminal.  */
+
+ssize_t
+read (int fd, void *buf, size_t count)
+{
+  static ssize_t (*read2) (int fd, void *buf, size_t count) = NULL;
+  if (read2 == NULL)
+    {
+      unsetenv ("LD_PRELOAD");
+      read2 = dlsym (RTLD_NEXT, "read");
+    }
+  if (count > 1 && isatty (fd) >= 1)
+    count = 1;
+  return read2 (fd, buf, count);
+}
-- 
1.9.3