[binutils-gdb] Fix TID parser bug

[binutils-gdb] Fix TID parser bug

[Pedro Alves]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b9a3f8429b012b753e30a4222bd8e4cbba019fad

commit b9a3f8429b012b753e30a4222bd8e4cbba019fad
Author: Pedro Alves <palves@redhat.com>
Date:   Thu Jun 13 00:06:52 2019 +0100

    Fix TID parser bug
    
    I noticed this inconsistency in the error messages below:
    
     (gdb) print --1
     Left operand of assignment is not an lvalue.
    
     (gdb) thread apply 1 print --1
    
     Thread 1 (Thread 0x7ffff7fb6740 (LWP 17805)):
     inverted range
    
    The "inverted range" error happens because get_number_trailer returns
    0 to indicate error, but number_or_range_parser::get_number is not
    checking for that.  I tried detected the error there, but that doesn't
    work because number_of_range_parser is used in places that _do_ want
    to legitimately handle 0.  IMO we should fix get_number_trailer's
    interface or use something else when we want to parse 0 too.
    
    I've decided to fix it in a different way, similarly to how
    number_or_range_parser::finished was changed in commit 529c08b25ec7
    ("Add helper functions parse_flags and parse_flags_qcs").
    
    Seems like a good change, even if we tweaked
    number_or_range_parser::get_number, as it simplifies
    thread_apply_command and makes them consistent with
    number_or_range_parser::finished().
    
    We now get the same error message in both cases:
    
     (gdb) print --1
     Left operand of assignment is not an lvalue.
    
     (gdb) thread apply 1 print --1
    
     Thread 1 (Thread 0x7ffff7fb6740 (LWP 17805)):
     Left operand of assignment is not an lvalue.
    
    gdb/ChangeLog:
    2019-06-13  Pedro Alves  <palves@redhat.com>
    
    	* thread.c (thread_apply_command): Adjust TID parsing.
    	* tid-parse.c (tid_range_parser::finished): Ensure parsing end is
    	detected before end of string.
    	(tid_is_in_list): Error out if LIST is invalid.
    
    gdb/testsuite/ChangeLog:
    2019-06-13  Pedro Alves  <palves@redhat.com>
    
    	* gdb.multi/tids.exp: Adjust expected output.  Add "thread apply 1
    	foo --1" test.

Diff:
---
 gdb/ChangeLog                    |  7 +++++++
 gdb/testsuite/ChangeLog          |  5 +++++
 gdb/testsuite/gdb.multi/tids.exp | 16 ++++++++++++++--
 gdb/thread.c                     | 15 ++++++---------
 gdb/tid-parse.c                  | 10 +++++++++-
 5 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index cd81cfe..3843e12 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2019-06-13  Pedro Alves  <palves@redhat.com>
 
+	* thread.c (thread_apply_command): Adjust TID parsing.
+	* tid-parse.c (tid_range_parser::finished): Ensure parsing end is
+	detected before end of string.
+	(tid_is_in_list): Error out if LIST is invalid.
+
+2019-06-13  Pedro Alves  <palves@redhat.com>
+
 	* completer.c (complete_line_internal_1): Rewind completion word
 	point.
 	(completion_tracker::advance_custom_word_point_by): Change
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 1a07266..5590bf9 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2019-06-13  Pedro Alves  <palves@redhat.com>
+
+	* gdb.multi/tids.exp: Adjust expected output.  Add "thread apply 1
+	foo --1" test.
+
 2019-06-11  Bernhard Heckel  <bernhard.heckel@intel.com>
 
 	* gdb.fortran/block-data.f: New.
diff --git a/gdb/testsuite/gdb.multi/tids.exp b/gdb/testsuite/gdb.multi/tids.exp
index 617a1b0..3b0e1c1 100644
--- a/gdb/testsuite/gdb.multi/tids.exp
+++ b/gdb/testsuite/gdb.multi/tids.exp
@@ -350,8 +350,13 @@ with_test_prefix "two inferiors" {
 	thr_apply_info_thr_error "${prefix}1-" "inverted range"
 	thr_apply_info_thr_error "${prefix}2-1" "inverted range"
 	thr_apply_info_thr_error "${prefix}2-\$one" "inverted range"
-	thr_apply_info_thr_error "${prefix}-1" "negative value"
-	thr_apply_info_thr_error "${prefix}-\$one" "negative value"
+	if {$prefix == ""} {
+	    thr_apply_info_thr_error "${prefix}-1" "Invalid thread ID: -1"
+	    thr_apply_info_thr_error "${prefix}-\$one" "Invalid thread ID: -\\\$one"
+	} else {
+	    thr_apply_info_thr_error "${prefix}-1" "negative value"
+	    thr_apply_info_thr_error "${prefix}-\$one" "negative value"
+	}
 	thr_apply_info_thr_error "${prefix}\$minus_one" \
 	    "negative value: ${prefix_re}\\\$minus_one"
 
@@ -374,6 +379,13 @@ with_test_prefix "two inferiors" {
 	gdb_test "thread apply 1.*" $output
     }
 
+    # Check that thread ID list parsing stops at the non-number token
+    # "foo" in a corner case where the "foo" is followed by hyphens.
+    # In this corner case, GDB used to skip past "foo", and then parse
+    # "--1" as a tid range for the current inferior.
+    gdb_test "thread apply 1 foo --1" \
+	"Undefined command: \"foo\".  Try \"help\"\\."
+
     # Check that we do parse the inferior number and don't confuse it.
     gdb_test "info threads 3.1" \
 	"No threads match '3.1'\."
diff --git a/gdb/thread.c b/gdb/thread.c
index 9a6a773..a84dbf9 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1560,7 +1560,6 @@ thread_apply_command (const char *tidlist, int from_tty)
 {
   qcs_flags flags;
   const char *cmd = NULL;
-  const char *cmd_or_flags;
   tid_range_parser parser;
 
   if (tidlist == NULL || *tidlist == '\000')
@@ -1572,17 +1571,15 @@ thread_apply_command (const char *tidlist, int from_tty)
       int inf_num, thr_start, thr_end;
 
       if (!parser.get_tid_range (&inf_num, &thr_start, &thr_end))
-	{
-	  cmd = parser.cur_tok ();
-	  break;
-	}
+	break;
     }
 
-  cmd_or_flags = cmd;
-  while (cmd != NULL && parse_flags_qcs ("thread apply", &cmd, &flags))
+  cmd = parser.cur_tok ();
+
+  while (parse_flags_qcs ("thread apply", &cmd, &flags))
     ;
 
-  if (cmd == NULL)
+  if (*cmd == '\0')
     error (_("Please specify a command following the thread ID list"));
 
   if (tidlist == cmd || !isalpha (cmd[0]))
@@ -1591,7 +1588,7 @@ thread_apply_command (const char *tidlist, int from_tty)
   scoped_restore_current_thread restore_thread;
 
   parser.init (tidlist, current_inferior ()->num);
-  while (!parser.finished () && parser.cur_tok () < cmd_or_flags)
+  while (!parser.finished ())
     {
       struct thread_info *tp = NULL;
       struct inferior *inf;
diff --git a/gdb/tid-parse.c b/gdb/tid-parse.c
index 828362e..07d7d2c 100644
--- a/gdb/tid-parse.c
+++ b/gdb/tid-parse.c
@@ -139,7 +139,13 @@ tid_range_parser::finished () const
   switch (m_state)
     {
     case STATE_INFERIOR:
-      return *m_cur_tok == '\0';
+      /* Parsing is finished when at end of string or null string,
+	 or we are not in a range and not in front of an integer, negative
+	 integer, convenience var or negative convenience var.  */
+      return (*m_cur_tok == '\0'
+	      || !(isdigit (*m_cur_tok)
+		   || *m_cur_tok == '$'
+		   || *m_cur_tok == '*'));
     case STATE_THREAD_RANGE:
     case STATE_STAR_RANGE:
       return m_range_parser.finished ();
@@ -311,6 +317,8 @@ tid_is_in_list (const char *list, int default_inferior,
     return 1;
 
   tid_range_parser parser (list, default_inferior);
+  if (parser.finished ())
+    invalid_thread_id_error (parser.cur_tok ());
   while (!parser.finished ())
     {
       int tmp_inf, tmp_thr_start, tmp_thr_end;