[RFC][gdb/testsuite] Add check-readmore

[RFC][gdb/testsuite] Add check-readmore

[Tom de Vries]

Hi,

Consider the gdb output:
...
27        return SYSCALL_CANCEL (nanosleep, requested_time, remaining);^M
(gdb) ^M
Thread 2 "run-attach-whil" stopped.^M
...

When trying to match the gdb prompt using gdb_test which uses '$gdb_prompt $',
it may pass or fail.

This sort of thing needs to be fixed (see commit b0e2f96b56b), but there's
currently no way to reliably find this type of FAILs.

We have check-read1, but that one actually make the test pass reliably.

We need something like the opposite of check-read1: something that makes
expect read a bit slower, or more exhaustively.

Add a new test target check-readmore that implements this.

Atm there are still two methods of implementing this in read1.c:
- the first method waits a bit before doing a read
- the second method does a read and then decides whether to
  return or to wait a bit and do another read.

Atm the first method is enabled by default, given that it is more foolproof.
The second method tries to be smart about waiting less than the first method,
but consequently needs to make decisions about error codes, which is more
fragile.

Tested on x86_64-linux, both with method 1 and 2.

Any comments?

Thanks,
- Tom

[gdb/testsuite] Add check-readmore

gdb/ChangeLog:

2021-06-07  Tom de Vries  <tdevries@suse.de>

	PR testsuite/27957
	* Makefile.in (check-readmore): New target.

gdb/testsuite/ChangeLog:

2021-06-07  Tom de Vries  <tdevries@suse.de>

	PR testsuite/27957
	* Makefile.in (EXPECT): Update.
	(check-readmore, expect-readmore, readmore.so): New target.
	(readmore): New phony target.
	(clean mostlyclean): Update.
	* lib/read1.c (read): Add READMORE case.
	* README: Mention check-readmore.

---
 gdb/Makefile.in           |  4 ++--
 gdb/testsuite/Makefile.in | 43 ++++++++++++++++++++++++++++++-------------
 gdb/testsuite/README      |  2 +-
 gdb/testsuite/lib/read1.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+), 16 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index f664d964536..ea33146f9ee 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1672,12 +1672,12 @@ check-perf: force
 	  $(MAKE) $(TARGET_FLAGS_TO_PASS) check-perf; \
 	else true; fi
 
-check-read1: force
+check-read1 check-readmore: 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; \
+	  $(MAKE) $(TARGET_FLAGS_TO_PASS) $@; \
 	else true; fi
 
 check-parallel: force
diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 2c0482cef1b..1762d631c70 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -39,6 +39,8 @@ CC=@CC@
 
 EXPECT = `if [ "$${READ1}" != "" ] ; then \
             echo $${rootme}/expect-read1; \
+          elif [ "$${READMORE}" != "" ] ; then \
+            echo $${rootme}/expect-readmore; \
           elif [ -f $${rootme}/../../expect/expect ] ; then \
             echo $${rootme}/../../expect/expect ; \
           else \
@@ -161,6 +163,9 @@ check: all $(abs_builddir)/site.exp
 check-read1: read1.so expect-read1
 	$(MAKE) READ1="1" check
 
+check-readmore: readmore.so expect-readmore
+	$(MAKE) READMORE="1" check
+
 # Check whether we need to print the timestamp for each line of
 # status.
 TIMESTAMP = $(if $(TS),| $(srcdir)/print-ts.py $(if $(TS_FORMAT),$(TS_FORMAT),),)
@@ -344,7 +349,7 @@ clean mostlyclean:
 	-rm -f *.dwo *.dwp
 	-rm -rf outputs temp cache
 	-rm -rf gdb.perf/workers gdb.perf/outputs gdb.perf/temp gdb.perf/cache
-	-rm -f read1.so expect-read1
+	-rm -f read1.so expect-read1 readmore.so expect-readmore
 
 distclean maintainer-clean realclean: clean
 	-rm -f *~ core
@@ -364,18 +369,22 @@ TAGS: force
 		-
 
 # Build the expect wrapper script that preloads the read1.so library.
-expect-read1:
+expect-read1 expect-readmore:
 	$(ECHO_GEN) \
-	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
+	rm -f $@-tmp; \
+	touch $@-tmp; \
+	echo "# THIS FILE IS GENERATED -*- buffer-read-only: t -*- \n" >>$@-tmp; \
+	echo "# vi:set ro: */\n\n" >>$@-tmp; \
+	echo "# To regenerate this file, run:\n" >>$@-tmp; \
+	echo "#      make clean; make/\n" >>$@-tmp; \
+	if [ $@ = expect-read1 ]; then \
+	  echo "export LD_PRELOAD=`pwd`/read1.so" >>$@-tmp; \
+	else \
+	  echo "export LD_PRELOAD=`pwd`/readmore.so" >>$@-tmp; \
+	fi; \
+	echo 'exec expect "$$@"' >>$@-tmp; \
+	chmod +x $@-tmp; \
+	mv $@-tmp $@
 
 # Build the read1.so preload library.  This overrides the `read'
 # function, making it read one byte at a time.  Running the testsuite
@@ -383,9 +392,17 @@ expect-read1:
 read1.so: lib/read1.c
 	$(ECHO_CC) $(CC) -o $@ ${srcdir}/lib/read1.c -Wall -g -shared -fPIC $(CFLAGS)
 
+# Build the readmore.so preload library.  This overrides the `read'
+# function, making it try harder to read more at a time.  Running the
+# testsuite with this catches racy tests.
+readmore.so: lib/read1.c
+	$(ECHO_CC) $(CC) -o $@ ${srcdir}/lib/read1.c -Wall -g -shared -fPIC \
+	  $(CFLAGS) -DREADMORE
+
 # Build the read1 machinery.
-.PHONY: read1
+.PHONY: read1 readmore
 read1: read1.so expect-read1
+readmore: readmore.so expect-readmore
 
 # Disable implicit make rules.
 include $(srcdir)/../disable-implicit-rules.mk
diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index 0036753eff0..46d39818460 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -357,7 +357,7 @@ fail, it can also have the effect of making otherwise failing tests pass.
 This happens f.i. if the test is trying to match a gdb prompt using an end of
 input marker "${gdb_prompt} $" and there is output after the gdb prompt.  This
 may either pass or fail in normal operation, but using check-read1 will ensure
-that it passes.
+that it passes.  Use check-readmore to detect this type of failure.
 
 Testsuite Configuration
 ***********************
diff --git a/gdb/testsuite/lib/read1.c b/gdb/testsuite/lib/read1.c
index 7dabdd4ca0c..1f3cd4393a1 100644
--- a/gdb/testsuite/lib/read1.c
+++ b/gdb/testsuite/lib/read1.c
@@ -21,6 +21,9 @@
 #include <unistd.h>
 #include <fcntl.h>
 #include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <stdio.h>
 
 /* Wrap 'read', forcing it to return only one byte at a time, if
    reading from the terminal.  */
@@ -38,7 +41,50 @@ read (int fd, void *buf, size_t count)
       setenv ("LD_PRELOAD", "", 1);
       read2 = dlsym (RTLD_NEXT, "read");
     }
+
+#ifdef READMORE
+  /* READMORE.  */
+
+#if 1
+  /* READMORE, method 1.  */
+
+  if (isatty (fd) != 0)
+    usleep (10 * 1000);
+  return read2 (fd, buf, count);
+
+#else
+  /* READMORE, method 2.  */
+  ssize_t res, res2;
+
+  res = read2 (fd, buf, count);
+  if (isatty (fd) == 0)
+    return res;
+
+  if (res == count || res == -1)
+    return res;
+
+  usleep (10 * 1000);
+  res2 = read2 (fd, (char *)buf + res, count - res);
+  if (res2 == -1)
+    {
+      if (errno == EAGAIN || errno == EIO)
+	res2 = 0;
+      else
+	{
+	  if (0)
+	    printf ("errno: %s\n", strerror (errno));
+	  return res2;
+	}
+    }
+
+  return res + res2;
+#endif
+
+#else
+  /* READ1 */
+
   if (count > 1 && isatty (fd) >= 1)
     count = 1;
   return read2 (fd, buf, count);
+#endif
 }

Re: [RFC][gdb/testsuite] Add check-readmore

[Simon Marchi]

On 2021-06-08 3:24 a.m., Tom de Vries wrote:
> Hi,
> 
> Consider the gdb output:
> ...
> 27        return SYSCALL_CANCEL (nanosleep, requested_time, remaining);^M
> (gdb) ^M
> Thread 2 "run-attach-whil" stopped.^M
> ...
> 
> When trying to match the gdb prompt using gdb_test which uses '$gdb_prompt $',
> it may pass or fail.
> 
> This sort of thing needs to be fixed (see commit b0e2f96b56b), but there's
> currently no way to reliably find this type of FAILs.
> 
> We have check-read1, but that one actually make the test pass reliably.
> 
> We need something like the opposite of check-read1: something that makes
> expect read a bit slower, or more exhaustively.
> 
> Add a new test target check-readmore that implements this.
> 
> Atm there are still two methods of implementing this in read1.c:
> - the first method waits a bit before doing a read
> - the second method does a read and then decides whether to
>   return or to wait a bit and do another read.
> 
> Atm the first method is enabled by default, given that it is more foolproof.
> The second method tries to be smart about waiting less than the first method,
> but consequently needs to make decisions about error codes, which is more
> fragile.
> 
> Tested on x86_64-linux, both with method 1 and 2.
> 
> Any comments?

Is there an advantage to method 2 over method 1?  If not, I'd just go
with the simplest.  I could imagine a modified method 2 version where we
read in a loop though, until we reached "count" bytes or the file
descriptor has no more data to offer (still with a 10ms wait between
each read, to give the writer time to produce more data).

Simon

Re: [RFC][gdb/testsuite] Add check-readmore

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 5021 bytes --]

On 8/26/21 5:09 AM, Simon Marchi wrote:
> 
> 
> On 2021-06-08 3:24 a.m., Tom de Vries wrote:
>> Hi,
>>
>> Consider the gdb output:
>> ...
>> 27        return SYSCALL_CANCEL (nanosleep, requested_time, remaining);^M
>> (gdb) ^M
>> Thread 2 "run-attach-whil" stopped.^M
>> ...
>>
>> When trying to match the gdb prompt using gdb_test which uses '$gdb_prompt $',
>> it may pass or fail.
>>
>> This sort of thing needs to be fixed (see commit b0e2f96b56b), but there's
>> currently no way to reliably find this type of FAILs.
>>
>> We have check-read1, but that one actually make the test pass reliably.
>>
>> We need something like the opposite of check-read1: something that makes
>> expect read a bit slower, or more exhaustively.
>>
>> Add a new test target check-readmore that implements this.
>>
>> Atm there are still two methods of implementing this in read1.c:
>> - the first method waits a bit before doing a read
>> - the second method does a read and then decides whether to
>>   return or to wait a bit and do another read.
>>
>> Atm the first method is enabled by default, given that it is more foolproof.
>> The second method tries to be smart about waiting less than the first method,
>> but consequently needs to make decisions about error codes, which is more
>> fragile.
>>
>> Tested on x86_64-linux, both with method 1 and 2.
>>
>> Any comments?
> 
> Is there an advantage to method 2 over method 1?

Yes.

Consider attached debug patch, and then running test-case
gdb.base/info-macros.exp:
...
$ rm -f LOG; ./test.sh -readmore
...
which gives us attached LOG.gz.

We get sequences like this:
...
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 4096
READ: fd: 5, COUNT: 4096, RES: 2659
...

With method 1, we wait some time before for _each_ read.

With method 2, we wait some time after the last read, and only for that
read.

There's an incentive to increase sleep time, because larger sleep time
has the potential of catching more errors.

OTOH there's an incentive to not let sleep time to grow to large because
that will cause unnecessary timeouts.

The first method multiplies sleep time quite rapidly and will hit the
timeouts sooner than method 2.

And besides the timeouts, method 2 is faster than method 1.

The drawback of method2 is that it's technically more complicated: it
inspects the return status of read, and potentially does a second read.
 It needs to know whether to do the second read, and it needs to know
which errors from the second read to ignore.  There's simply more scope
for errors and corner-cases.

> If not, I'd just go
> with the simplest.  I could imagine a modified method 2 version where we
> read in a loop though, until we reached "count" bytes or the file
> descriptor has no more data to offer (still with a 10ms wait between
> each read, to give the writer time to produce more data).

Agreed, there could be some benefit in doing this in a loop.  It would
add a benefit similar to increasing the timeout, without the drawback of
waiting unnecessary while the buffer is already full.

Thanks,
- Tom

[-- Attachment #2: 0002-debug.patch --]
[-- Type: text/x-patch, Size: 1195 bytes --]

debug

---
 gdb/testsuite/lib/read1.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/read1.c b/gdb/testsuite/lib/read1.c
index 1f3cd4393a1..eded36d6c1f 100644
--- a/gdb/testsuite/lib/read1.c
+++ b/gdb/testsuite/lib/read1.c
@@ -32,6 +32,7 @@ ssize_t
 read (int fd, void *buf, size_t count)
 {
   static ssize_t (*read2) (int fd, void *buf, size_t count) = NULL;
+  static FILE *log = NULL;
   if (read2 == NULL)
     {
       /* Use setenv (v, "", 1) rather than unsetenv (v) to work around
@@ -40,6 +41,7 @@ read (int fd, void *buf, size_t count)
 	 for existence from another interp".  */
       setenv ("LD_PRELOAD", "", 1);
       read2 = dlsym (RTLD_NEXT, "read");
+      log = fopen ("/home/vries/gdb_versions/devel/LOG", "a");
     }
 
 #ifdef READMORE
@@ -48,10 +50,13 @@ read (int fd, void *buf, size_t count)
 #if 1
   /* READMORE, method 1.  */
 
+#if 0
   if (isatty (fd) != 0)
     usleep (10 * 1000);
-  return read2 (fd, buf, count);
-
+#endif
+  ssize_t res = read2 (fd, buf, count);
+  fprintf (log, "READ: fd: %d, COUNT: %zd, RES: %zd\n", fd, count, res);
+  return res;
 #else
   /* READMORE, method 2.  */
   ssize_t res, res2;

[-- Attachment #3: LOG.gz --]
[-- Type: application/gzip, Size: 678 bytes --]

[PATCH][gdb/testsuite] Add check-readmore

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

[ was: Re: [RFC][gdb/testsuite] Add check-readmore ]

On 8/30/21 5:30 PM, Tom de Vries wrote:

>> If not, I'd just go
>> with the simplest.  I could imagine a modified method 2 version where we
>> read in a loop though, until we reached "count" bytes or the file
>> descriptor has no more data to offer (still with a 10ms wait between
>> each read, to give the writer time to produce more data).
> 
> Agreed, there could be some benefit in doing this in a loop.  It would
> add a benefit similar to increasing the timeout, without the drawback of
> waiting unnecessary while the buffer is already full.

Implemented the loop approach.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Add-check-readmore.patch --]
[-- Type: text/x-patch, Size: 11108 bytes --]

[gdb/testsuite] Add check-readmore

Consider the gdb output:
...
27        return SYSCALL_CANCEL (nanosleep, requested_time, remaining);^M
(gdb) ^M
Thread 2 "run-attach-whil" stopped.^M
...

When trying to match the gdb prompt using gdb_test which uses '$gdb_prompt $',
it may pass or fail.

This sort of thing needs to be fixed (see commit b0e2f96b56b), but there's
currently no way to reliably find this type of FAILs.

We have check-read1, but that one actually make the test pass reliably.

We need something like the opposite of check-read1: something that makes
expect read a bit slower, or more exhaustively.

Add a new test target check-readmore that implements this.

There are two methods of implementing this in read1.c:
- the first method waits a bit before doing a read
- the second method does a read and then decides whether to
  return or to wait a bit and do another read, and so on.

The second method is potentially faster, has less risc of timeout and could
potentially detect more problems.  The first method has a simpler
implementation.

The second method is enabled by default.  The default waiting period is 10
miliseconds.

The first method can be enabled using:
...
$ export READMORE_METHOD=1
...
and the waiting period can be specified in miliseconds using:
...
$ export READMORE_SLEEP=9
...

Also a log file can be specified using:
...
$ export READMORE_LOG=$(pwd -P)/LOG
...

Tested on x86_64-linux.

Testing with check-readmore showed these regressions:
...
FAIL: gdb.base/bp-cmds-continue-ctrl-c.exp: run: stop with control-c (continue)
FAIL: gdb.base/bp-cmds-continue-ctrl-c.exp: attach: stop with control-c (continue)
...

I have not been able to find a problem in the test-case, and I think it's the
nature of both the test-case and readmore that makes it run longer.  Make
these pass by increasing the alarm timeout from 60 to 120 seconds.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27957

---
 gdb/Makefile.in                                  |   4 +-
 gdb/testsuite/Makefile.in                        |  43 ++++---
 gdb/testsuite/README                             |   2 +-
 gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c |   2 +-
 gdb/testsuite/lib/read1.c                        | 136 ++++++++++++++++++++++-
 5 files changed, 166 insertions(+), 21 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 320d3326a81..6594d3ae444 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1675,12 +1675,12 @@ check-perf: force
 	  $(MAKE) $(TARGET_FLAGS_TO_PASS) check-perf; \
 	else true; fi
 
-check-read1: force
+check-read1 check-readmore: 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; \
+	  $(MAKE) $(TARGET_FLAGS_TO_PASS) $@; \
 	else true; fi
 
 check-parallel: force
diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 20cb3dae2ed..6b4c7588cfa 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -39,6 +39,8 @@ CC=@CC@
 
 EXPECT = `if [ "$${READ1}" != "" ] ; then \
             echo $${rootme}/expect-read1; \
+          elif [ "$${READMORE}" != "" ] ; then \
+            echo $${rootme}/expect-readmore; \
           elif [ -f $${rootme}/../../expect/expect ] ; then \
             echo $${rootme}/../../expect/expect ; \
           else \
@@ -161,6 +163,9 @@ check: all $(abs_builddir)/site.exp
 check-read1: read1.so expect-read1
 	$(MAKE) READ1="1" check
 
+check-readmore: readmore.so expect-readmore
+	$(MAKE) READMORE="1" check
+
 # Check whether we need to print the timestamp for each line of
 # status.
 TIMESTAMP = $(if $(TS),| $(srcdir)/print-ts.py $(if $(TS_FORMAT),$(TS_FORMAT),),)
@@ -344,7 +349,7 @@ clean mostlyclean:
 	-rm -f *.dwo *.dwp
 	-rm -rf outputs temp cache
 	-rm -rf gdb.perf/workers gdb.perf/outputs gdb.perf/temp gdb.perf/cache
-	-rm -f read1.so expect-read1
+	-rm -f read1.so expect-read1 readmore.so expect-readmore
 
 distclean maintainer-clean realclean: clean
 	-rm -f *~ core
@@ -367,18 +372,22 @@ TAGS: force
 		-
 
 # Build the expect wrapper script that preloads the read1.so library.
-expect-read1:
+expect-read1 expect-readmore:
 	$(ECHO_GEN) \
-	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
+	rm -f $@-tmp; \
+	touch $@-tmp; \
+	echo "# THIS FILE IS GENERATED -*- buffer-read-only: t -*- \n" >>$@-tmp; \
+	echo "# vi:set ro: */\n\n" >>$@-tmp; \
+	echo "# To regenerate this file, run:\n" >>$@-tmp; \
+	echo "#      make clean; make/\n" >>$@-tmp; \
+	if [ $@ = expect-read1 ]; then \
+	  echo "export LD_PRELOAD=`pwd`/read1.so" >>$@-tmp; \
+	else \
+	  echo "export LD_PRELOAD=`pwd`/readmore.so" >>$@-tmp; \
+	fi; \
+	echo 'exec expect "$$@"' >>$@-tmp; \
+	chmod +x $@-tmp; \
+	mv $@-tmp $@
 
 # Build the read1.so preload library.  This overrides the `read'
 # function, making it read one byte at a time.  Running the testsuite
@@ -386,9 +395,17 @@ expect-read1:
 read1.so: lib/read1.c
 	$(ECHO_CC) $(CC) -o $@ ${srcdir}/lib/read1.c -Wall -g -shared -fPIC $(CFLAGS)
 
+# Build the readmore.so preload library.  This overrides the `read'
+# function, making it try harder to read more at a time.  Running the
+# testsuite with this catches racy tests.
+readmore.so: lib/read1.c
+	$(ECHO_CC) $(CC) -o $@ ${srcdir}/lib/read1.c -Wall -g -shared -fPIC \
+	  $(CFLAGS) -DREADMORE
+
 # Build the read1 machinery.
-.PHONY: read1
+.PHONY: read1 readmore
 read1: read1.so expect-read1
+readmore: readmore.so expect-readmore
 
 # Disable implicit make rules.
 include $(srcdir)/../disable-implicit-rules.mk
diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index 862f423a988..7552774c78b 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -361,7 +361,7 @@ fail, it can also have the effect of making otherwise failing tests pass.
 This happens f.i. if the test is trying to match a gdb prompt using an end of
 input marker "${gdb_prompt} $" and there is output after the gdb prompt.  This
 may either pass or fail in normal operation, but using check-read1 will ensure
-that it passes.
+that it passes.  Use check-readmore to detect this type of failure.
 
 Testsuite Configuration
 ***********************
diff --git a/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c
index 968c879dd09..fb76cfb3973 100644
--- a/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c
+++ b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c
@@ -26,7 +26,7 @@ foo (void)
 int
 main ()
 {
-  alarm (60);
+  alarm (120);
 
   while (1)
     foo ();
diff --git a/gdb/testsuite/lib/read1.c b/gdb/testsuite/lib/read1.c
index 7dabdd4ca0c..4e8aac6566f 100644
--- a/gdb/testsuite/lib/read1.c
+++ b/gdb/testsuite/lib/read1.c
@@ -21,14 +21,62 @@
 #include <unistd.h>
 #include <fcntl.h>
 #include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <stdio.h>
 
-/* Wrap 'read', forcing it to return only one byte at a time, if
-   reading from the terminal.  */
+/* Default READMORE method.  */
+#define READMORE_METHOD_DEFAULT 2
+
+/* Default READMORE sleep time in miliseconds.  */
+#define READMORE_SLEEP_DEFAULT 10
+
+/* Helper function.  Intialize *METHOD according to environment variable
+   READMORE_METHOD, and *SLEEP according to environment variable
+   READMORE_SLEEP.  */
+
+static void
+init_readmore (int *method, unsigned int *sleep, FILE **log)
+{
+  char *env = getenv ("READMORE_METHOD");
+  if (env == NULL)
+    *method = READMORE_METHOD_DEFAULT;
+  else if (strcmp (env, "1") == 0)
+    *method = 1;
+  else if (strcmp (env, "2") == 0)
+    *method = 2;
+  else
+    /* Default.  */
+    *method = READMORE_METHOD_DEFAULT;
+
+  env = getenv ("READMORE_SLEEP");
+  if (env == NULL)
+    *sleep = READMORE_SLEEP_DEFAULT;
+  else
+    *sleep = atoi (env);
+
+  env = getenv ("READMORE_LOG");
+  if (env == NULL)
+    *log = NULL;
+  else
+    *log = fopen (env, "w");
+}
+
+/* Wrap 'read', and modify it's behaviour using READ1 or READMORE style.  */
 
 ssize_t
 read (int fd, void *buf, size_t count)
 {
   static ssize_t (*read2) (int fd, void *buf, size_t count) = NULL;
+  static FILE *log;
+  int readmore;
+#ifdef READMORE
+  readmore = 1;
+#else
+  readmore = 0;
+#endif
+  static int readmore_method;
+  static unsigned int readmore_sleep;
   if (read2 == NULL)
     {
       /* Use setenv (v, "", 1) rather than unsetenv (v) to work around
@@ -37,8 +85,88 @@ read (int fd, void *buf, size_t count)
 	 for existence from another interp".  */
       setenv ("LD_PRELOAD", "", 1);
       read2 = dlsym (RTLD_NEXT, "read");
+      if (readmore)
+	init_readmore (&readmore_method, &readmore_sleep, &log);
     }
-  if (count > 1 && isatty (fd) >= 1)
-    count = 1;
+
+  /* Only modify 'read' behaviour when reading from the terminal.  */
+  if (isatty (fd) == 0)
+    goto fallback;
+
+  if (!readmore)
+    {
+      /* READ1.  Force read to return only one byte at a time.  */
+      return read2 (fd, buf, 1);
+    }
+
+  if (readmore_method == 1)
+    {
+      /* READMORE, method 1.  Wait a little before doing a read.  */
+      usleep (readmore_sleep * 1000);
+      return read2 (fd, buf, count);
+    }
+
+  if (readmore_method == 2)
+    {
+      /* READMORE, method 2.  After doing a read, either return or wait
+	 a little and do another read, and so on.  */
+      ssize_t res, total;
+      int iteration;
+      int max_iterations = -1;
+
+      total = 0;
+      for (iteration = 1; ; iteration++)
+	{
+	  res = read2 (fd, (char *)buf + total, count - total);
+	  if (log != NULL)
+	    fprintf (log,
+		     "READ (%d): fd: %d, COUNT: %zd, RES: %zd, ERRNO: %s\n",
+		     iteration, fd, count - total, res,
+		     res == -1 ? strerror (errno) : "none");
+	  if (res == -1)
+	    {
+	      if (iteration == 1)
+		{
+		  /* Error on first read, report.  */
+		  total = -1;
+		  break;
+		}
+
+	      if (total > 0
+		  && (errno == EAGAIN || errno == EWOULDBLOCK || errno == EIO))
+		{
+		  /* Ignore error, but don't try anymore reading.  */
+		  errno = 0;
+		  break;
+		}
+
+	      /* Other error, report back.  */
+	      total = -1;
+	      break;
+	    }
+
+	  total += res;
+	  if (total == count)
+	    /* Buf full, no need to do any more reading.  */
+	    break;
+
+	  /* Handle end-of-file.  */
+	  if (res == 0)
+	    break;
+
+	  if (iteration == max_iterations)
+	    break;
+
+	  usleep (readmore_sleep * 1000);
+	}
+
+      if (log)
+	fprintf (log, "READ returning: RES: %zd, ERRNO: %s\n",
+		 total, total == -1 ? strerror (errno) : "none");
+      return total;
+    }
+
+ fallback:
+  /* Fallback, regular read.  */
   return read2 (fd, buf, count);
 }

Re: [PATCH][gdb/testsuite] Add check-readmore

[Tom de Vries]

On 8/31/21 3:45 PM, Tom de Vries via Gdb-patches wrote:
> [ was: Re: [RFC][gdb/testsuite] Add check-readmore ]
> 
> On 8/30/21 5:30 PM, Tom de Vries wrote:
> 
>>> If not, I'd just go
>>> with the simplest.  I could imagine a modified method 2 version where we
>>> read in a loop though, until we reached "count" bytes or the file
>>> descriptor has no more data to offer (still with a 10ms wait between
>>> each read, to give the writer time to produce more data).
>>
>> Agreed, there could be some benefit in doing this in a loop.  It would
>> add a benefit similar to increasing the timeout, without the drawback of
>> waiting unnecessary while the buffer is already full.
> 
> Implemented the loop approach.
> 

I've committed this now.

Thanks,
- Tom