From patchwork Fri Nov 30 19:43:36 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 30482 Received: (qmail 64925 invoked by alias); 30 Nov 2018 19:43:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 64907 invoked by uid 89); 30 Nov 2018 19:43:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Dec, searches, sk:search_, def_vector X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 30 Nov 2018 19:43:39 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DD71981F12; Fri, 30 Nov 2018 19:43:37 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2665A60FAB; Fri, 30 Nov 2018 19:43:36 +0000 (UTC) Subject: [PATCH] Merge forward-search/reverse-search, use gdb::def_vector, remove limit (Re: [RFA] Fix leak in forward-search) To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20181127233328.5164-1-philippe.waroquiers@skynet.be> <3cf960b8-ff82-0670-fa90-c94d78573bfe@redhat.com> <1543532723.4149.7.camel@skynet.be> From: Pedro Alves Message-ID: <6e8a2a00-bcbd-cdc6-f332-f59988bb8c40@redhat.com> Date: Fri, 30 Nov 2018 19:43:36 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1543532723.4149.7.camel@skynet.be> 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 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 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 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. 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 * source.c (forward_search_command): Rename to ... (search_command_helper): ... this. Add 'forward' parameter. Tweak to use a gdb::def_vector 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 * 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) /* 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 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 }