public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] [testsuite] Fix pretty printers regexps for GDB output
@ 2024-01-24 10:47 Christophe Lyon
  2024-01-24 11:01 ` Jonathan Wakely
  0 siblings, 1 reply; 6+ messages in thread
From: Christophe Lyon @ 2024-01-24 10:47 UTC (permalink / raw)
  To: gcc-patches, libstdc++, tom, jwakely; +Cc: Christophe Lyon

GDB emits end of lines as \r\n, we currently match the reverse \n\r,
possibly leading to mismatches under racy conditions.

I noticed this while running the GCC testsuite using the equivalent of
GDB's READ1 feature [1] which helps detecting bufferization issues.

Adjusting the first regexp to match the right order implied fixing the
second one, to skip the empty lines.

Tested on aarch64-linux-gnu.

[1] https//github.com/bminor/binutils-gdb/blob/master/gdb/testsuite/README#L269

2024-01-24  Christophe Lyon  <christophe.lyon@linaro.org>

	libstdc++-v3/
	* testsuite/lib/gdb-test.exp (gdb-test): Fix regexps.
---
 libstdc++-v3/testsuite/lib/gdb-test.exp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
index 31206f2fc32..0de8d9ee153 100644
--- a/libstdc++-v3/testsuite/lib/gdb-test.exp
+++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
@@ -194,7 +194,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
 
     set test_counter 0
     remote_expect target [timeout_value] {
-	-re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
+	-re {^(type|\$([0-9]+)) = ([^\n\r]*)\r\n} {
 	    send_log "got: $expect_out(buffer)"
 
 	    incr test_counter
@@ -250,7 +250,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
 	    return
 	}
 
-	-re {^[^$][^\n\r]*[\n\r]+} {
+	-re {^[\r\n]*[^$][^\n\r]*\r\n} {
 	    send_log "skipping: $expect_out(buffer)"
 	    exp_continue
 	}
-- 
2.34.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] [testsuite] Fix pretty printers regexps for GDB output
  2024-01-24 10:47 [PATCH] [testsuite] Fix pretty printers regexps for GDB output Christophe Lyon
@ 2024-01-24 11:01 ` Jonathan Wakely
  2024-01-25 15:54   ` Christophe Lyon
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Wakely @ 2024-01-24 11:01 UTC (permalink / raw)
  To: Christophe Lyon; +Cc: gcc-patches, libstdc++, tom

On Wed, 24 Jan 2024 at 10:48, Christophe Lyon wrote:
>
> GDB emits end of lines as \r\n, we currently match the reverse \n\r,

We currently match [\n\r]+ which should match any of \n, \r, \n\r or \r\n


> possibly leading to mismatches under racy conditions.

What do we incorrectly match? Is the problem that a \r\n sequence
might be incompletely printed, due to buffering, and so the regex only
sees (and matches) the \r which then leaves an unwanted \n in the
stream, which then interferes with the next match? I don't understand
why that problem wouldn't just result in a failed match with your new
regex though.


>
> I noticed this while running the GCC testsuite using the equivalent of
> GDB's READ1 feature [1] which helps detecting bufferization issues.
>
> Adjusting the first regexp to match the right order implied fixing the
> second one, to skip the empty lines.

At the very least, this part of the description is misleading. The
existing regex matches "the right order" already. The change is to
match *exactly* \r\n instead of any mix of CR and LF characters.
That's not about matching "the right order", it's being more precise
in what we match.

But I'm still confused about what the failure scenario is and how the
change fixes it.

>
> Tested on aarch64-linux-gnu.
>
> [1] https//github.com/bminor/binutils-gdb/blob/master/gdb/testsuite/README#L269
>
> 2024-01-24  Christophe Lyon  <christophe.lyon@linaro.org>
>
>         libstdc++-v3/
>         * testsuite/lib/gdb-test.exp (gdb-test): Fix regexps.
> ---
>  libstdc++-v3/testsuite/lib/gdb-test.exp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
> index 31206f2fc32..0de8d9ee153 100644
> --- a/libstdc++-v3/testsuite/lib/gdb-test.exp
> +++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
> @@ -194,7 +194,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
>
>      set test_counter 0
>      remote_expect target [timeout_value] {
> -       -re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> +       -re {^(type|\$([0-9]+)) = ([^\n\r]*)\r\n} {
>             send_log "got: $expect_out(buffer)"
>
>             incr test_counter
> @@ -250,7 +250,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
>             return
>         }
>
> -       -re {^[^$][^\n\r]*[\n\r]+} {
> +       -re {^[\r\n]*[^$][^\n\r]*\r\n} {
>             send_log "skipping: $expect_out(buffer)"
>             exp_continue
>         }
> --
> 2.34.1
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] [testsuite] Fix pretty printers regexps for GDB output
  2024-01-24 11:01 ` Jonathan Wakely
@ 2024-01-25 15:54   ` Christophe Lyon
  2024-02-06  9:26     ` Christophe Lyon
  2024-04-11 11:01     ` Jonathan Wakely
  0 siblings, 2 replies; 6+ messages in thread
From: Christophe Lyon @ 2024-01-25 15:54 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: gcc-patches, libstdc++, tom

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

On Wed, 24 Jan 2024 at 12:02, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Wed, 24 Jan 2024 at 10:48, Christophe Lyon wrote:
> >
> > GDB emits end of lines as \r\n, we currently match the reverse \n\r,
>
> We currently match [\n\r]+ which should match any of \n, \r, \n\r or \r\n
>

Hmm, right, sorry I had this patch in my tree for quite some time, but
wrote the description just now, so I read a bit too quickly.

>
> > possibly leading to mismatches under racy conditions.
>
> What do we incorrectly match? Is the problem that a \r\n sequence
> might be incompletely printed, due to buffering, and so the regex only
> sees (and matches) the \r which then leaves an unwanted \n in the
> stream, which then interferes with the next match? I don't understand
> why that problem wouldn't just result in a failed match with your new
> regex though.
>
Exactly: READ1 forces read() to return 1 byte at a time, so we leave
an unwanted \r in front of a string that should otherwise match the
"got" case.

>
> >
> > I noticed this while running the GCC testsuite using the equivalent of
> > GDB's READ1 feature [1] which helps detecting bufferization issues.
> >
> > Adjusting the first regexp to match the right order implied fixing the
> > second one, to skip the empty lines.
>
> At the very least, this part of the description is misleading. The
> existing regex matches "the right order" already. The change is to
> match *exactly* \r\n instead of any mix of CR and LF characters.
> That's not about matching "the right order", it's being more precise
> in what we match.
>
> But I'm still confused about what the failure scenario is and how the
> change fixes it.
>

I followed what the GDB testsuite does (it matches \r\n at the end of
many regexps), but in fact it seems it's not needed:
it works if I update the "got" regexp like this (ie. accept any number
of leading \r or \n):
-       -re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
+       -re {^[\n\r]*(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
and leave the "skipping" regexp as it is currently.

Is the new attached version OK?

Thanks,

Christophe

> >
> > Tested on aarch64-linux-gnu.
> >
> > [1] https//github.com/bminor/binutils-gdb/blob/master/gdb/testsuite/README#L269
> >
> > 2024-01-24  Christophe Lyon  <christophe.lyon@linaro.org>
> >
> >         libstdc++-v3/
> >         * testsuite/lib/gdb-test.exp (gdb-test): Fix regexps.
> > ---
> >  libstdc++-v3/testsuite/lib/gdb-test.exp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
> > index 31206f2fc32..0de8d9ee153 100644
> > --- a/libstdc++-v3/testsuite/lib/gdb-test.exp
> > +++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
> > @@ -194,7 +194,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
> >
> >      set test_counter 0
> >      remote_expect target [timeout_value] {
> > -       -re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> > +       -re {^(type|\$([0-9]+)) = ([^\n\r]*)\r\n} {
> >             send_log "got: $expect_out(buffer)"
> >
> >             incr test_counter
> > @@ -250,7 +250,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
> >             return
> >         }
> >
> > -       -re {^[^$][^\n\r]*[\n\r]+} {
> > +       -re {^[\r\n]*[^$][^\n\r]*\r\n} {
> >             send_log "skipping: $expect_out(buffer)"
> >             exp_continue
> >         }
> > --
> > 2.34.1
> >
>

[-- Attachment #2: v2-0001-testsuite-Fix-pretty-printers-regexp-for-GDB-outp.patch --]
[-- Type: text/x-patch, Size: 2487 bytes --]

From e7cd0475141921282d5c9044b2450ae8e196efc4 Mon Sep 17 00:00:00 2001
From: Christophe Lyon <christophe.lyon@linaro.org>
Date: Thu, 25 Jan 2024 15:43:56 +0000
Subject: [PATCH v2] [testsuite] Fix pretty printers regexp for GDB output

GDB emits end of lines as \r\n, we currently match any >0 number of
either \n or \r, possibly leading to mismatches under racy conditions.

I noticed this while running the GCC testsuite using the equivalent of
GDB's READ1 feature [1] which helps detecting bufferization issues.

We try to match
\n$1 = empty std::tuple\r

against {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} which fails because
of the leading \n (which was left in the buffer after the previous
"skipping" pattern matched the preceding \r).

This patch accepts any number of leading \n and/or \r in the "got" clause.

Also take this opportunity to quote \r and \r in the logs, to make
debugging such issues easier.

Tested on aarch64-linux-gnu.

[1] https//github.com/bminor/binutils-gdb/blob/master/gdb/testsuite/README#L269

2024-01-24  Christophe Lyon  <christophe.lyon@linaro.org>

	libstdc++-v3/
	* testsuite/lib/gdb-test.exp (gdb-test): Fix regexp.  Quote
	newlines in logs.
---
 libstdc++-v3/testsuite/lib/gdb-test.exp | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
index 31206f2fc32..2ec5596983d 100644
--- a/libstdc++-v3/testsuite/lib/gdb-test.exp
+++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
@@ -194,8 +194,11 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
 
     set test_counter 0
     remote_expect target [timeout_value] {
-	-re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
-	    send_log "got: $expect_out(buffer)"
+	-re {^[\n\r]*(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
+	    # Escape newlines so that we can print them.
+	    set escaped [string map {"\n" "\\n"} $expect_out(buffer)]
+	    set escaped2 [string map {"\r" "\\r"} $escaped]
+	    send_log "got: $escaped2"
 
 	    incr test_counter
 	    set first $expect_out(3,string)
@@ -251,7 +254,10 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
 	}
 
 	-re {^[^$][^\n\r]*[\n\r]+} {
-	    send_log "skipping: $expect_out(buffer)"
+	    # Escape newlines so that we can print them.
+	    set escaped [string map {"\n" "\\n"} $expect_out(buffer)]
+	    set escaped2 [string map {"\r" "\\r"} $escaped]
+	    send_log "skipping: $escaped2"
 	    exp_continue
 	}
 
-- 
2.34.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] [testsuite] Fix pretty printers regexps for GDB output
  2024-01-25 15:54   ` Christophe Lyon
@ 2024-02-06  9:26     ` Christophe Lyon
  2024-04-10 13:43       ` Christophe Lyon
  2024-04-11 11:01     ` Jonathan Wakely
  1 sibling, 1 reply; 6+ messages in thread
From: Christophe Lyon @ 2024-02-06  9:26 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: gcc-patches, libstdc++, tom

ping?

On Thu, 25 Jan 2024 at 16:54, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> On Wed, 24 Jan 2024 at 12:02, Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> > On Wed, 24 Jan 2024 at 10:48, Christophe Lyon wrote:
> > >
> > > GDB emits end of lines as \r\n, we currently match the reverse \n\r,
> >
> > We currently match [\n\r]+ which should match any of \n, \r, \n\r or \r\n
> >
>
> Hmm, right, sorry I had this patch in my tree for quite some time, but
> wrote the description just now, so I read a bit too quickly.
>
> >
> > > possibly leading to mismatches under racy conditions.
> >
> > What do we incorrectly match? Is the problem that a \r\n sequence
> > might be incompletely printed, due to buffering, and so the regex only
> > sees (and matches) the \r which then leaves an unwanted \n in the
> > stream, which then interferes with the next match? I don't understand
> > why that problem wouldn't just result in a failed match with your new
> > regex though.
> >
> Exactly: READ1 forces read() to return 1 byte at a time, so we leave
> an unwanted \r in front of a string that should otherwise match the
> "got" case.
>
> >
> > >
> > > I noticed this while running the GCC testsuite using the equivalent of
> > > GDB's READ1 feature [1] which helps detecting bufferization issues.
> > >
> > > Adjusting the first regexp to match the right order implied fixing the
> > > second one, to skip the empty lines.
> >
> > At the very least, this part of the description is misleading. The
> > existing regex matches "the right order" already. The change is to
> > match *exactly* \r\n instead of any mix of CR and LF characters.
> > That's not about matching "the right order", it's being more precise
> > in what we match.
> >
> > But I'm still confused about what the failure scenario is and how the
> > change fixes it.
> >
>
> I followed what the GDB testsuite does (it matches \r\n at the end of
> many regexps), but in fact it seems it's not needed:
> it works if I update the "got" regexp like this (ie. accept any number
> of leading \r or \n):
> -       -re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> +       -re {^[\n\r]*(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> and leave the "skipping" regexp as it is currently.
>
> Is the new attached version OK?
>
> Thanks,
>
> Christophe
>
> > >
> > > Tested on aarch64-linux-gnu.
> > >
> > > [1] https//github.com/bminor/binutils-gdb/blob/master/gdb/testsuite/README#L269
> > >
> > > 2024-01-24  Christophe Lyon  <christophe.lyon@linaro.org>
> > >
> > >         libstdc++-v3/
> > >         * testsuite/lib/gdb-test.exp (gdb-test): Fix regexps.
> > > ---
> > >  libstdc++-v3/testsuite/lib/gdb-test.exp | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
> > > index 31206f2fc32..0de8d9ee153 100644
> > > --- a/libstdc++-v3/testsuite/lib/gdb-test.exp
> > > +++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
> > > @@ -194,7 +194,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
> > >
> > >      set test_counter 0
> > >      remote_expect target [timeout_value] {
> > > -       -re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> > > +       -re {^(type|\$([0-9]+)) = ([^\n\r]*)\r\n} {
> > >             send_log "got: $expect_out(buffer)"
> > >
> > >             incr test_counter
> > > @@ -250,7 +250,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
> > >             return
> > >         }
> > >
> > > -       -re {^[^$][^\n\r]*[\n\r]+} {
> > > +       -re {^[\r\n]*[^$][^\n\r]*\r\n} {
> > >             send_log "skipping: $expect_out(buffer)"
> > >             exp_continue
> > >         }
> > > --
> > > 2.34.1
> > >
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] [testsuite] Fix pretty printers regexps for GDB output
  2024-02-06  9:26     ` Christophe Lyon
@ 2024-04-10 13:43       ` Christophe Lyon
  0 siblings, 0 replies; 6+ messages in thread
From: Christophe Lyon @ 2024-04-10 13:43 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: gcc-patches, libstdc++, tom

ping?

On Tue, 6 Feb 2024 at 10:26, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>
> ping?
>
> On Thu, 25 Jan 2024 at 16:54, Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
> >
> > On Wed, 24 Jan 2024 at 12:02, Jonathan Wakely <jwakely@redhat.com> wrote:
> > >
> > > On Wed, 24 Jan 2024 at 10:48, Christophe Lyon wrote:
> > > >
> > > > GDB emits end of lines as \r\n, we currently match the reverse \n\r,
> > >
> > > We currently match [\n\r]+ which should match any of \n, \r, \n\r or \r\n
> > >
> >
> > Hmm, right, sorry I had this patch in my tree for quite some time, but
> > wrote the description just now, so I read a bit too quickly.
> >
> > >
> > > > possibly leading to mismatches under racy conditions.
> > >
> > > What do we incorrectly match? Is the problem that a \r\n sequence
> > > might be incompletely printed, due to buffering, and so the regex only
> > > sees (and matches) the \r which then leaves an unwanted \n in the
> > > stream, which then interferes with the next match? I don't understand
> > > why that problem wouldn't just result in a failed match with your new
> > > regex though.
> > >
> > Exactly: READ1 forces read() to return 1 byte at a time, so we leave
> > an unwanted \r in front of a string that should otherwise match the
> > "got" case.
> >
> > >
> > > >
> > > > I noticed this while running the GCC testsuite using the equivalent of
> > > > GDB's READ1 feature [1] which helps detecting bufferization issues.
> > > >
> > > > Adjusting the first regexp to match the right order implied fixing the
> > > > second one, to skip the empty lines.
> > >
> > > At the very least, this part of the description is misleading. The
> > > existing regex matches "the right order" already. The change is to
> > > match *exactly* \r\n instead of any mix of CR and LF characters.
> > > That's not about matching "the right order", it's being more precise
> > > in what we match.
> > >
> > > But I'm still confused about what the failure scenario is and how the
> > > change fixes it.
> > >
> >
> > I followed what the GDB testsuite does (it matches \r\n at the end of
> > many regexps), but in fact it seems it's not needed:
> > it works if I update the "got" regexp like this (ie. accept any number
> > of leading \r or \n):
> > -       -re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> > +       -re {^[\n\r]*(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> > and leave the "skipping" regexp as it is currently.
> >
> > Is the new attached version OK?
> >
> > Thanks,
> >
> > Christophe
> >
> > > >
> > > > Tested on aarch64-linux-gnu.
> > > >
> > > > [1] https//github.com/bminor/binutils-gdb/blob/master/gdb/testsuite/README#L269
> > > >
> > > > 2024-01-24  Christophe Lyon  <christophe.lyon@linaro.org>
> > > >
> > > >         libstdc++-v3/
> > > >         * testsuite/lib/gdb-test.exp (gdb-test): Fix regexps.
> > > > ---
> > > >  libstdc++-v3/testsuite/lib/gdb-test.exp | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
> > > > index 31206f2fc32..0de8d9ee153 100644
> > > > --- a/libstdc++-v3/testsuite/lib/gdb-test.exp
> > > > +++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
> > > > @@ -194,7 +194,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
> > > >
> > > >      set test_counter 0
> > > >      remote_expect target [timeout_value] {
> > > > -       -re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> > > > +       -re {^(type|\$([0-9]+)) = ([^\n\r]*)\r\n} {
> > > >             send_log "got: $expect_out(buffer)"
> > > >
> > > >             incr test_counter
> > > > @@ -250,7 +250,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
> > > >             return
> > > >         }
> > > >
> > > > -       -re {^[^$][^\n\r]*[\n\r]+} {
> > > > +       -re {^[\r\n]*[^$][^\n\r]*\r\n} {
> > > >             send_log "skipping: $expect_out(buffer)"
> > > >             exp_continue
> > > >         }
> > > > --
> > > > 2.34.1
> > > >
> > >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] [testsuite] Fix pretty printers regexps for GDB output
  2024-01-25 15:54   ` Christophe Lyon
  2024-02-06  9:26     ` Christophe Lyon
@ 2024-04-11 11:01     ` Jonathan Wakely
  1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Wakely @ 2024-04-11 11:01 UTC (permalink / raw)
  To: Christophe Lyon; +Cc: Jonathan Wakely, gcc-patches, libstdc++, tom

On Thu, 25 Jan 2024 at 15:54, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> On Wed, 24 Jan 2024 at 12:02, Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> > On Wed, 24 Jan 2024 at 10:48, Christophe Lyon wrote:
> > >
> > > GDB emits end of lines as \r\n, we currently match the reverse \n\r,
> >
> > We currently match [\n\r]+ which should match any of \n, \r, \n\r or \r\n
> >
>
> Hmm, right, sorry I had this patch in my tree for quite some time, but
> wrote the description just now, so I read a bit too quickly.
>
> >
> > > possibly leading to mismatches under racy conditions.
> >
> > What do we incorrectly match? Is the problem that a \r\n sequence
> > might be incompletely printed, due to buffering, and so the regex only
> > sees (and matches) the \r which then leaves an unwanted \n in the
> > stream, which then interferes with the next match? I don't understand
> > why that problem wouldn't just result in a failed match with your new
> > regex though.
> >
> Exactly: READ1 forces read() to return 1 byte at a time, so we leave
> an unwanted \r in front of a string that should otherwise match the
> "got" case.
>
> >
> > >
> > > I noticed this while running the GCC testsuite using the equivalent of
> > > GDB's READ1 feature [1] which helps detecting bufferization issues.
> > >
> > > Adjusting the first regexp to match the right order implied fixing the
> > > second one, to skip the empty lines.
> >
> > At the very least, this part of the description is misleading. The
> > existing regex matches "the right order" already. The change is to
> > match *exactly* \r\n instead of any mix of CR and LF characters.
> > That's not about matching "the right order", it's being more precise
> > in what we match.
> >
> > But I'm still confused about what the failure scenario is and how the
> > change fixes it.
> >
>
> I followed what the GDB testsuite does (it matches \r\n at the end of
> many regexps), but in fact it seems it's not needed:
> it works if I update the "got" regexp like this (ie. accept any number
> of leading \r or \n):
> -       -re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> +       -re {^[\n\r]*(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> and leave the "skipping" regexp as it is currently.
>
> Is the new attached version OK?

Yeah this makes more sense to me now, thanks.

OK for trunk.


>
> Thanks,
>
> Christophe
>
> > >
> > > Tested on aarch64-linux-gnu.
> > >
> > > [1] https//github.com/bminor/binutils-gdb/blob/master/gdb/testsuite/README#L269
> > >
> > > 2024-01-24  Christophe Lyon  <christophe.lyon@linaro.org>
> > >
> > >         libstdc++-v3/
> > >         * testsuite/lib/gdb-test.exp (gdb-test): Fix regexps.
> > > ---
> > >  libstdc++-v3/testsuite/lib/gdb-test.exp | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
> > > index 31206f2fc32..0de8d9ee153 100644
> > > --- a/libstdc++-v3/testsuite/lib/gdb-test.exp
> > > +++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
> > > @@ -194,7 +194,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
> > >
> > >      set test_counter 0
> > >      remote_expect target [timeout_value] {
> > > -       -re {^(type|\$([0-9]+)) = ([^\n\r]*)[\n\r]+} {
> > > +       -re {^(type|\$([0-9]+)) = ([^\n\r]*)\r\n} {
> > >             send_log "got: $expect_out(buffer)"
> > >
> > >             incr test_counter
> > > @@ -250,7 +250,7 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
> > >             return
> > >         }
> > >
> > > -       -re {^[^$][^\n\r]*[\n\r]+} {
> > > +       -re {^[\r\n]*[^$][^\n\r]*\r\n} {
> > >             send_log "skipping: $expect_out(buffer)"
> > >             exp_continue
> > >         }
> > > --
> > > 2.34.1
> > >
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-04-11 11:01 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-24 10:47 [PATCH] [testsuite] Fix pretty printers regexps for GDB output Christophe Lyon
2024-01-24 11:01 ` Jonathan Wakely
2024-01-25 15:54   ` Christophe Lyon
2024-02-06  9:26     ` Christophe Lyon
2024-04-10 13:43       ` Christophe Lyon
2024-04-11 11:01     ` Jonathan Wakely

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).