* [PATCH] Merge forward-search/reverse-search, use gdb::def_vector, remove limit (Re: [RFA] Fix leak in forward-search)
2018-11-29 23:05 ` Philippe Waroquiers
@ 2018-11-30 19:43 ` Pedro Alves
2018-11-30 20:42 ` Tom Tromey
2018-12-02 16:57 ` Philippe Waroquiers
0 siblings, 2 replies; 7+ messages in thread
From: Pedro Alves @ 2018-11-30 19:43 UTC (permalink / raw)
To: Philippe Waroquiers, gdb-patches
On 11/29/2018 11:05 PM, Philippe Waroquiers wrote:
> On Thu, 2018-11-29 at 15:42 +0000, Pedro Alves wrote:
>> On 11/27/2018 11:33 PM, Philippe Waroquiers wrote:
>> The patch is OK, but I think that replacing 'buf' and all that
>> manual buffer growing with a non-static gdb::def_vector<char> defined
>> outside the outer loop would be even better.
> Thanks for the review, I have pushed this version, but I have added in
> my todo list the better fix + add a test : I found no explicit
> functional test for this command + my limited time on GDB development is also
> shared with analysing the remaining several hundreds tests having a definite
> leak :).
>
I looked a little and found that we do indeed have tests, however
they test using the "search" alias, instead of "forward-search",
which made them invisible to greps for the latter.
I also noticed a couple other things... See the patch below. WDYT?
From 7323b0b52c3c6cbd667904b7bb4653396ea10fac Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 30 Nov 2018 18:50:23 +0000
Subject: [PATCH] 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-11-30 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-11-30 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.
---
gdb/source.c | 149 ++++++++++++----------------------------
gdb/testsuite/gdb.base/list.exp | 17 ++++-
2 files changed, 59 insertions(+), 107 deletions(-)
diff --git a/gdb/source.c b/gdb/source.c
index c75351e65f4..f0e98fa5d32 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1521,16 +1521,14 @@ info_line_command (const char *arg, int from_tty)
\f
/* 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);
+ char *msg = (char *) 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/gdb.base/list.exp b/gdb/testsuite/gdb.base/list.exp
index abfb0e165d1..9e39e51a118 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
}
--
2.14.4
^ permalink raw reply [flat|nested] 7+ messages in thread