[review,gdb/testsuite] Make inferior_exited_re match a single line

Message ID gerrit.1580742892000.Id7b1dcecd8c7fda3d1ab34b4fa1364d301748333@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Feb. 3, 2020, 3:14 p.m. UTC
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/764
......................................................................

[gdb/testsuite] Make inferior_exited_re match a single line

The current inferior_exited_re regexp contains a '.*':
...
set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(.*\\) exited)"
...

This means that while matching a single line:
...
$ tclsh
% set re "(?:\\\[Inferior \[0-9\]+ \\(.*\\) exited)"
(?:\[Inferior [0-9]+ \(.*\) exited)
% set line "\[Inferior 1 (process 33) exited\]\n"
[Inferior 1 (process 33) exited]

% regexp $re $line
1
...
it also matches more than one line:
...
$ tclsh
% set re "(?:\\\[Inferior \[0-9\]+ \\(.*\\) exited)"
(?:\[Inferior [0-9]+ \(.*\) exited)
% set line "\[Inferior 1 (process 33) exited\]\n\[Inferior 2 (process 44) exited\]\n"
[Inferior 1 (process 33) exited]
[Inferior 2 (process 44) exited]

% regexp $re $line
1
...

Fix this by using "\[^\n\r]*" instead of ".*".

Build and reg-tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2020-02-01  Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (inferior_exited_re): Use "\[^\n\r]*" instead of ".*".

Change-Id: Id7b1dcecd8c7fda3d1ab34b4fa1364d301748333
---
M gdb/testsuite/lib/gdb.exp
1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Simon Marchi (Code Review) Feb. 4, 2020, 4:38 a.m. UTC | #1
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/764
......................................................................


Patch Set 1:

(1 comment)

| --- gdb/testsuite/lib/gdb.exp
| +++ gdb/testsuite/lib/gdb.exp
| @@ -112,18 +112,18 @@ global env
|  if ![info exists env(EXEEXT)] {
|      set EXEEXT ""
|  } else {
|      set EXEEXT $env(EXEEXT)
|  }
|  
|  set octal "\[0-7\]+"
|  
| -set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(.*\\) exited)"
| +set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(\[^\n\r]*\\) exited)"

PS1, Line 120:

Usually I would escape the closing square bracket, like what is done
in `\[0-9\]` just before on the same line.  But I presume it's not
strictly necessary, and that the regexp engine sees that this closing
bracket doesn't match any opening bracket, and therefore treats it
like a normal character?

For consistency, I would suggest escaping it, but the patch LGTM
either way.

|  
|  # A regular expression that matches a value history number.
|  # E.g., $1, $2, etc.
|  set valnum_re "\\\$$decimal"
|  
|  ### Only procedures should come after this point.
|  
|  #
|  # gdb_version -- extract and print the version number of GDB
  
Simon Marchi (Code Review) Feb. 4, 2020, 9:31 a.m. UTC | #2
Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/764
......................................................................


Patch Set 1:

(1 comment)

| --- gdb/testsuite/lib/gdb.exp
| +++ gdb/testsuite/lib/gdb.exp
| @@ -112,18 +112,18 @@ global env
|  if ![info exists env(EXEEXT)] {
|      set EXEEXT ""
|  } else {
|      set EXEEXT $env(EXEEXT)
|  }
|  
|  set octal "\[0-7\]+"
|  
| -set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(.*\\) exited)"
| +set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(\[^\n\r]*\\) exited)"

PS1, Line 120:

It was a typo, thanks for noticing that. I've retested and will submit
the updated patch.

I've tried to understand why not escaping the closing square bracket
still works, and I think it happens before a string is interpreted as
regexp, in the substitutions.

The escape of the opening square bracket prevents command
substitution, but after substitution is done we're left with the same
string, whether we escaped the closing square bracket or not:
...
$ tclsh
% set line1 "\[bla]"
[bla]
% set line2 "\[bla\]"
[bla]
% string equal $line1 $line2
1
...

|  
|  # A regular expression that matches a value history number.
|  # E.g., $1, $2, etc.
|  set valnum_re "\\\$$decimal"
|  
|  ### Only procedures should come after this point.
|  
|  #
|  # gdb_version -- extract and print the version number of GDB
  
Simon Marchi (Code Review) Feb. 4, 2020, 2:23 p.m. UTC | #3
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/764
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

| --- gdb/testsuite/lib/gdb.exp
| +++ gdb/testsuite/lib/gdb.exp
| @@ -112,18 +112,18 @@ global env
|  if ![info exists env(EXEEXT)] {
|      set EXEEXT ""
|  } else {
|      set EXEEXT $env(EXEEXT)
|  }
|  
|  set octal "\[0-7\]+"
|  
| -set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(.*\\) exited)"
| +set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(\[^\n\r]*\\) exited)"

PS1, Line 120:

Eh, you're right, what I said didn't make sense.  We escape the
opening bracket so it's not interpreted as a procedure call, so it can
be interpreted as a character group.  But still, it's the same: if
there's an unescaped closing bracket that doesn't match any unescaped
opening bracket, tcl will treat it as a regular character.

|  
|  # A regular expression that matches a value history number.
|  # E.g., $1, $2, etc.
|  set valnum_re "\\\$$decimal"
|  
|  ### Only procedures should come after this point.
|  
|  #
|  # gdb_version -- extract and print the version number of GDB
  

Patch

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 25bed76..14d752b 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -117,7 +117,7 @@ 
 
 set octal "\[0-7\]+"
 
-set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(.*\\) exited)"
+set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(\[^\n\r]*\\) exited)"
 
 # A regular expression that matches a value history number.
 # E.g., $1, $2, etc.