[binutils-gdb] Merge forward-search/reverse-search, use gdb::def_vector, remove limit

[Pedro Alves <palves at sourceware dot org>]

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

commit 73e8dc90a8a94bc52e29596b1cc176b882fbbc8e
Author: Pedro Alves <palves@redhat.com>
Date:   Sat Dec 8 15:03:29 2018 +0000

    Merge forward-search/reverse-search, use gdb::def_vector, remove limit
    
    Back in:
    
     commit 85ae1317add94adef4817927e89cff80b92813dd
     Author:     Stan Shebs <shebs@codesourcery.com>
     AuthorDate: Thu Dec 8 02:27:47 1994 +0000
    
    	     * source.c: Various cosmetic changes.
    	     (forward_search_command): Handle very long source lines correctly.
    
    a buffer with a hard limit was converted to a heap buffer:
    
      @@ -1228,15 +1284,26 @@ forward_search_command (regex, from_tty)
         stream = fdopen (desc, FOPEN_RT);
         clearerr (stream);
         while (1) {
      -/* FIXME!!!  We walk right off the end of buf if we get a long line!!! */
      -    char buf[4096];            /* Should be reasonable??? */
      -    register char *p = buf;
      +    static char *buf = NULL;
      +    register char *p;
      +    int cursize, newsize;
      +
      +    cursize = 256;
      +    buf = xmalloc (cursize);
      +    p = buf;
    
    However, reverse_search_command has the exact same problem, and that
    wasn't fixed.  We still have that "we walk right off" comment...
    
    Recently, the xmalloc above was replaced with a xrealloc, because as
    can be seen above, that 'buf' variable above was a static local,
    otherwise we'd be leaking.  This commit replaces that and the
    associated manual buffer growing with a gdb::def_vector<char>.  I
    don't think there's much point in reusing the buffer across command
    invocations.
    
    While doing this, I realized that reverse_search_command is almost
    identical to forward_search_command.  So this commit factors out a
    common helper function instead of duplicating a lot of code.
    
    There are some tests for "forward-search" in gdb.base/list.exp, but
    since they use the "search" alias, they were a bit harder to find than
    expected.  That's now fixed, both by testing both variants, and by
    adding some commentary.  Also, there are no tests for the
    "reverse-search" command, so this commit adds some for that too.
    
    gdb/ChangeLog:
    2018-12-08  Pedro Alves  <palves@redhat.com>
    
    	* source.c (forward_search_command): Rename to ...
    	(search_command_helper): ... this.  Add 'forward' parameter.
    	Tweak to use a gdb::def_vector<char> instead of a xrealloc'ed
    	buffer.  Handle backward searches too.
    	(forward_search_command, reverse_search_command): Reimplement by
    	calling search_command_helper.
    
    gdb/testsuite/ChangeLog:
    2018-12-08  Pedro Alves  <palves@redhat.com>
    
    	* gdb.base/list.exp (test_forward_search): Rename to ...
    	(test_forward_reverse_search): ... this.  Also test reverse-search
    	and the forward-search alias.

Diff:
---
 gdb/ChangeLog                   |   9 +++
 gdb/source.c                    | 149 ++++++++++++----------------------------
 gdb/testsuite/ChangeLog         |   6 ++
 gdb/testsuite/gdb.base/list.exp |  17 ++++-
 4 files changed, 74 insertions(+), 107 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2e105ae..df49408 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2018-12-08  Pedro Alves  <palves@redhat.com>
+
+	* source.c (forward_search_command): Rename to ...
+	(search_command_helper): ... this.  Add 'forward' parameter.
+	Tweak to use a gdb::def_vector<char> instead of a xrealloc'ed
+	buffer.  Handle backward searches too.
+	(forward_search_command, reverse_search_command): Reimplement by
+	calling search_command_helper.
+
 2018-12-07  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* .dir-locals.el: Copy most of the settings from c-mode over to
diff --git a/gdb/source.c b/gdb/source.c
index c75351e..952fc3f 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1521,16 +1521,14 @@ info_line_command (const char *arg, int from_tty)
 
 /* Commands to search the source file for a regexp.  */
 
+/* Helper for forward_search_command/reverse_search_command.  FORWARD
+   indicates direction: true for forward, false for
+   backward/reverse.  */
+
 static void
-forward_search_command (const char *regex, int from_tty)
+search_command_helper (const char *regex, int from_tty, bool forward)
 {
-  int c;
-  int line;
-  char *msg;
-
-  line = last_line_listed + 1;
-
-  msg = (char *) re_comp (regex);
+  const char *msg = re_comp (regex);
   if (msg)
     error (("%s"), msg);
 
@@ -1544,6 +1542,10 @@ forward_search_command (const char *regex, int from_tty)
   if (current_source_symtab->line_charpos == 0)
     find_source_lines (current_source_symtab, desc.get ());
 
+  int line = (forward
+	      ? last_line_listed + 1
+	      : last_line_listed - 1);
+
   if (line < 1 || line > current_source_symtab->nlines)
     error (_("Expression not found"));
 
@@ -1553,43 +1555,35 @@ forward_search_command (const char *regex, int from_tty)
 
   gdb_file_up stream = desc.to_file (FDOPEN_MODE);
   clearerr (stream.get ());
+
+  gdb::def_vector<char> buf;
+  buf.reserve (256);
+
   while (1)
     {
-      static char *buf = NULL;
-      char *p;
-      int cursize, newsize;
-
-      cursize = 256;
-      buf = (char *) xrealloc (buf, cursize);
-      p = buf;
+      buf.resize (0);
 
-      c = fgetc (stream.get ());
+      int c = fgetc (stream.get ());
       if (c == EOF)
 	break;
       do
 	{
-	  *p++ = c;
-	  if (p - buf == cursize)
-	    {
-	      newsize = cursize + cursize / 2;
-	      buf = (char *) xrealloc (buf, newsize);
-	      p = buf + cursize;
-	      cursize = newsize;
-	    }
+	  buf.push_back (c);
 	}
       while (c != '\n' && (c = fgetc (stream.get ())) >= 0);
 
       /* Remove the \r, if any, at the end of the line, otherwise
          regular expressions that end with $ or \n won't work.  */
-      if (p - buf > 1 && p[-2] == '\r')
+      size_t sz = buf.size ();
+      if (sz >= 2 && buf[sz - 2] == '\r')
 	{
-	  p--;
-	  p[-1] = '\n';
+	  buf[sz - 2] = '\n';
+	  buf.resize (sz - 1);
 	}
 
       /* We now have a source line in buf, null terminate and match.  */
-      *p = 0;
-      if (re_exec (buf) > 0)
+      buf.push_back ('\0');
+      if (re_exec (buf.data ()) > 0)
 	{
 	  /* Match!  */
 	  print_source_lines (current_source_symtab, line, line + 1, 0);
@@ -1597,90 +1591,35 @@ forward_search_command (const char *regex, int from_tty)
 	  current_source_line = std::max (line - lines_to_list / 2, 1);
 	  return;
 	}
-      line++;
+
+      if (forward)
+	line++;
+      else
+	{
+	  line--;
+	  if (fseek (stream.get (),
+		     current_source_symtab->line_charpos[line - 1], 0) < 0)
+	    {
+	      const char *filename
+		= symtab_to_filename_for_display (current_source_symtab);
+	      perror_with_name (filename);
+	    }
+	}
     }
 
   printf_filtered (_("Expression not found\n"));
 }
 
 static void
-reverse_search_command (const char *regex, int from_tty)
+forward_search_command (const char *regex, int from_tty)
 {
-  int c;
-  int line;
-  char *msg;
-
-  line = last_line_listed - 1;
-
-  msg = (char *) re_comp (regex);
-  if (msg)
-    error (("%s"), msg);
-
-  if (current_source_symtab == 0)
-    select_source_symtab (0);
-
-  scoped_fd desc = open_source_file (current_source_symtab);
-  if (desc.get () < 0)
-    perror_with_name (symtab_to_filename_for_display (current_source_symtab));
-
-  if (current_source_symtab->line_charpos == 0)
-    find_source_lines (current_source_symtab, desc.get ());
-
-  if (line < 1 || line > current_source_symtab->nlines)
-    error (_("Expression not found"));
-
-  if (lseek (desc.get (), current_source_symtab->line_charpos[line - 1], 0)
-      < 0)
-    perror_with_name (symtab_to_filename_for_display (current_source_symtab));
-
-  gdb_file_up stream = desc.to_file (FDOPEN_MODE);
-  clearerr (stream.get ());
-  while (line > 1)
-    {
-/* FIXME!!!  We walk right off the end of buf if we get a long line!!!  */
-      char buf[4096];		/* Should be reasonable???  */
-      char *p = buf;
-
-      c = fgetc (stream.get ());
-      if (c == EOF)
-	break;
-      do
-	{
-	  *p++ = c;
-	}
-      while (c != '\n' && (c = fgetc (stream.get ())) >= 0);
-
-      /* Remove the \r, if any, at the end of the line, otherwise
-         regular expressions that end with $ or \n won't work.  */
-      if (p - buf > 1 && p[-2] == '\r')
-	{
-	  p--;
-	  p[-1] = '\n';
-	}
-
-      /* We now have a source line in buf; null terminate and match.  */
-      *p = 0;
-      if (re_exec (buf) > 0)
-	{
-	  /* Match!  */
-	  print_source_lines (current_source_symtab, line, line + 1, 0);
-	  set_internalvar_integer (lookup_internalvar ("_"), line);
-	  current_source_line = std::max (line - lines_to_list / 2, 1);
-	  return;
-	}
-      line--;
-      if (fseek (stream.get (),
-		 current_source_symtab->line_charpos[line - 1], 0) < 0)
-	{
-	  const char *filename;
-
-	  filename = symtab_to_filename_for_display (current_source_symtab);
-	  perror_with_name (filename);
-	}
-    }
+  search_command_helper (regex, from_tty, true);
+}
 
-  printf_filtered (_("Expression not found\n"));
-  return;
+static void
+reverse_search_command (const char *regex, int from_tty)
+{
+  search_command_helper (regex, from_tty, false);
 }
 
 /* If the last character of PATH is a directory separator, then strip it.  */
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 66310d7..b6c6c4a 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-12-08  Pedro Alves  <palves@redhat.com>
+
+	* gdb.base/list.exp (test_forward_search): Rename to ...
+	(test_forward_reverse_search): ... this.  Also test reverse-search
+	and the forward-search alias.
+
 2018-12-05  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* config/sim.exp (gdb_target_sim): Remove redundant adjustment of
diff --git a/gdb/testsuite/gdb.base/list.exp b/gdb/testsuite/gdb.base/list.exp
index abfb0e1..9b172e9 100644
--- a/gdb/testsuite/gdb.base/list.exp
+++ b/gdb/testsuite/gdb.base/list.exp
@@ -485,7 +485,9 @@ proc test_list_filename_and_function {} {
 
 }
 
-proc test_forward_search {} {
+# Test the forward-search (aka search) and the reverse-search commands.
+
+proc test_forward_reverse_search {} {
 	global timeout
 
 	gdb_test_no_output "set listsize 4"
@@ -499,6 +501,17 @@ proc test_forward_search {} {
 
 	gdb_test "search 6789" "28\[ \t\]+oof .6789.;"
 
+	# Try again, we shouldn't re-find the same source line.  Also,
+	# while at it, test using the "forward-search" alias.
+	gdb_test "forward-search 6789" " not found"
+
+	# Now test backwards.  First make sure we start searching from
+	# the previous line, not the current line.
+	gdb_test "reverse-search 6789" " not found"
+
+	# Now find something in a previous line.
+	gdb_test "reverse-search 67" "26\[ \t\]+oof .67.;"
+
 	# Test that GDB won't crash if the line being searched is extremely long.
 
 	set oldtimeout $timeout
@@ -553,7 +566,7 @@ if [ set_listsize 10 ] then {
     test_repeat_list_command
     test_list_range
     test_list_filename_and_function
-    test_forward_search
+    test_forward_reverse_search
     test_only_end
     test_list_invalid_args
 }