This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Include access size in find command error message
On 26/09/2012 4:48 PM, Pedro Alves wrote:
> Thanks for doing this. It looks all good to me. Minor comments below.
Thanks for the review. I believe I've addressed all your comments, I have an updated patch below.
>
> On 09/26/2012 01:28 PM, Andrew Burgess wrote:
>
>> gdb/gdbserver/ChangeLog
>>
>> 2012-09-26 Andrew Burgess <aburgess@broadcom.com>
>>
>> * server.c (handle_search_memory_1): Include access length in error
>> message.
>
> Pedantically, it's a warning message, not an error though.
Corrected in both cases.
>
> This code should be using the proper paddress/pulongest/%u,etc. for the
> types instead of the (long) casts, but it's precedent, so subject for
> a separate fix.
I tried switching to plongest for printing the buffer size, however, in the gdbserver version of plongest (gdb/gdbserver/utils.c) the behaviour is to zero pad the number, so the error message looks a bit rubbish: "... failed to read 000034 bytes ... ".
I couldn't see a better function to use, I didn't really want to start adding/changing utils.c in this patch, so I've left thing as they were for now.
>> diff --git a/gdb/target.c b/gdb/target.c
>> index 1fc8802..cea3d10 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -2874,8 +2874,9 @@ simple_search_memory (struct target_ops *ops,
>> if (target_read (ops, TARGET_OBJECT_MEMORY, NULL,
>> search_buf, start_addr, search_buf_size) != search_buf_size)
>> {
>> - warning (_("Unable to access target memory at %s, halting search."),
>> - hex_string (start_addr));
>> + warning (_("Unable to access %s bytes of target "
>> + "memory at %s, halting search."),
>> + plongest (search_buf_size), hex_string (start_addr));
>> do_cleanups (old_cleanups);
>> return -1;
>> }
>> @@ -2928,8 +2929,9 @@ simple_search_memory (struct target_ops *ops,
>> search_buf + keep_len, read_addr,
>> nr_to_read) != nr_to_read)
>> {
>> - warning (_("Unable to access target "
>> + warning (_("Unable to access %s bytes of target "
>> "memory at %s, halting search."),
>> + plongest (nr_to_read),
>> hex_string (read_addr));
>> do_cleanups (old_cleanups);
>> return -1;
>>
>
> Pedantically, neither search_buf_size nor nr_to_read are
> of type LONGEST, but it doesn't really matter. Just mentioning
> to be thorough.
I did change the first call on search_buf_size to pulongest as I spotted that it was unsigned. But other than that I've left the code as is. There seems to be massive precedent for this throughout gdb, so in the absence of better suggestions, this seems acceptable.
>
>> gdb/testsuite/ChangeLog
>>
>> 2012-09-26 Andrew Burgess <aburgess@broadcom.com>
>>
>> Test find command on unmapped memory.
>> * gdb.base/find-unmapped.c: New file.
>> * gdb.base/find-unmapped.exp: New file.
>>
>> diff --git a/gdb/testsuite/gdb.base/find-unmapped.c b/gdb/testsuite/gdb.base/find-unmapped.c
>> new file mode 100644
>> index 0000000..70059b1
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/find-unmapped.c
>> +
>> + /*
>> + Map enough pages to cover at least CHUNK_SIZE, and one extra page. We
>> + then unmap the last page.
>> +
>> + From gdb we can then perform find commands into unmapped region, gdb
>> + should given an error.
>> +
>> + .----.----.----.----.----.
>> + | | | | | |
>> + '----'----'----'----'----'
>> + |<- CHUNK_SIZE ->|
>> + */
>> +
>> + pg_size = getpagesize ();
>> + /* The +2 gives the extra page. */
>> + pg_count = (CHUNK_SIZE / pg_size) + 2;
>
> Will this all still work if pg_size is > than CHUNK_SIZE?
I believe it will (if I've missed something please feel free to say), I've updated the comment to mention that this case should work, and also explain a little more what the test plan is.
>> +
>> + memset (p, 0, (pg_count *pg_size));
>
> Space after * missing.
Fixed.
>> + last_mapped_page = p + ((pg_count - 2) * pg_size);
>> + unmapped_page = last_mapped_page + pg_size;
>
> Unnecessary parens in several places (around * and /).
I believe I got all of these.
>> diff --git a/gdb/testsuite/gdb.base/find-unmapped.exp b/gdb/testsuite/gdb.base/find-unmapped.exp
>> new file mode 100644
>> index 0000000..341538b
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/find-unmapped.exp
>> +
>> +# Now try a find starting from each global. The warning message is made
>> +# optional here as for gdbserver targets the warning is printed by
>> +# gdbserver, but expect is only collecting gdb output.
>
> It looks like the second sentence is stale, given the test is skipped
> against remote targets.
Ooops. Fixed.
Cheers,
Andrew
gdb/gdbserver/ChangeLog
2012-09-26 Andrew Burgess <aburgess@broadcom.com>
* server.c (handle_search_memory_1): Include access length in
warning message.
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 547552f..61a7313 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -735,8 +735,9 @@ handle_search_memory_1 (CORE_ADDR start_addr, CORE_ADDR search_space_len,
if (gdb_read_memory (start_addr, search_buf, search_buf_size)
!= search_buf_size)
{
- warning ("Unable to access target memory at 0x%lx, halting search.",
- (long) start_addr);
+ warning ("Unable to access %ld bytes of target "
+ "memory at 0x%lx, halting search.",
+ (long) search_buf_size, (long) start_addr);
return -1;
}
@@ -787,9 +788,9 @@ handle_search_memory_1 (CORE_ADDR start_addr, CORE_ADDR search_space_len,
if (gdb_read_memory (read_addr, search_buf + keep_len,
nr_to_read) != search_buf_size)
{
- warning ("Unable to access target memory "
+ warning ("Unable to access %ld bytes of target memory "
"at 0x%lx, halting search.",
- (long) read_addr);
+ (long) nr_to_read, (long) read_addr);
return -1;
}
gdb/ChangeLog
2012-09-26 Andrew Burgess <aburgess@broadcom.com>
* target.c (simple_search_memory): Include access length in
warning message.
diff --git a/gdb/target.c b/gdb/target.c
index 1fc8802..861e6a6 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2874,8 +2874,9 @@ simple_search_memory (struct target_ops *ops,
if (target_read (ops, TARGET_OBJECT_MEMORY, NULL,
search_buf, start_addr, search_buf_size) != search_buf_size)
{
- warning (_("Unable to access target memory at %s, halting search."),
- hex_string (start_addr));
+ warning (_("Unable to access %s bytes of target "
+ "memory at %s, halting search."),
+ pulongest (search_buf_size), hex_string (start_addr));
do_cleanups (old_cleanups);
return -1;
}
@@ -2928,8 +2929,9 @@ simple_search_memory (struct target_ops *ops,
search_buf + keep_len, read_addr,
nr_to_read) != nr_to_read)
{
- warning (_("Unable to access target "
+ warning (_("Unable to access %s bytes of target "
"memory at %s, halting search."),
+ plongest (nr_to_read),
hex_string (read_addr));
do_cleanups (old_cleanups);
return -1;
gdb/testsuite/ChangeLog
2012-09-26 Andrew Burgess <aburgess@broadcom.com>
Test find command on unmapped memory.
* gdb.base/find-unmapped.c: New file.
* gdb.base/find-unmapped.exp: New file.
diff --git a/gdb/testsuite/gdb.base/find-unmapped.c b/gdb/testsuite/gdb.base/find-unmapped.c
new file mode 100644
index 0000000..d16f95f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/find-unmapped.c
@@ -0,0 +1,106 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2012 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#include <string.h>
+
+#define CHUNK_SIZE 16000 /* same as findcmd.c's */
+
+void *global_var_0;
+void *global_var_1;
+void *global_var_2;
+
+void
+breakpt ()
+{
+ /* Nothing. */
+}
+
+int
+main (void)
+{
+ void *p;
+ size_t pg_size;
+ int pg_count;
+ void *unmapped_page, *last_mapped_page, *first_mapped_page;
+
+ /*
+ Map enough pages to cover at least CHUNK_SIZE, and one extra page. We
+ then unmap the last page.
+
+ From gdb we can then perform find commands into unmapped region, gdb
+ should given an error.
+
+ .-- global_var_0 .-- global_var_1
+ | | .-- global_var_2
+ | | |
+ .----.----.----.----.----.
+ | | | | | |
+ '----'----'----'----'----'
+ |<- CHUNK_SIZE ->|
+
+ If CHUNK_SIZE equals page size then we'll get 3 pages, and if
+ CHUNK_SIZE is less than page size we'll get 2 pages. The test will
+ still work in these cases.
+
+ (1) We do a find from global_var_0 to global_var_2, this will fail when
+ loading the second chunk, as we know at least CHUNK_SIZE bytes are in
+ mapped space.
+
+ (2) We do a find from global_var_1 to global_var_2, this will fail when
+ loading the first chunk, assuming the CHUNK_SIZE is at least 16 bytes.
+
+ (3) We do a find from global_var_2 to (global_var_2 + 16), this too
+ will fail when loading the first chunk regardless of the chunk size.
+ */
+
+ pg_size = getpagesize ();
+ /* The +2 ensures the extra page. */
+ pg_count = CHUNK_SIZE / pg_size + 2;
+
+ p = mmap (0, pg_count * pg_size, PROT_READ|PROT_WRITE,
+ MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
+ if (p == MAP_FAILED)
+ {
+ perror ("mmap");
+ return EXIT_FAILURE;
+ }
+
+ memset (p, 0, pg_count * pg_size);
+
+ if (munmap (p + (pg_count - 1) * pg_size, pg_size) == -1)
+ {
+ perror ("munmap");
+ return EXIT_FAILURE;
+ }
+
+ first_mapped_page = p;
+ last_mapped_page = p + (pg_count - 2) * pg_size;
+ unmapped_page = last_mapped_page + pg_size;
+
+ /* Setup global variables we reference from gdb. */
+ global_var_0 = first_mapped_page;
+ global_var_1 = unmapped_page - 16;
+ global_var_2 = unmapped_page + 16;
+
+ breakpt ();
+
+ return EXIT_SUCCESS;
+}
diff --git a/gdb/testsuite/gdb.base/find-unmapped.exp b/gdb/testsuite/gdb.base/find-unmapped.exp
new file mode 100644
index 0000000..66907e2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/find-unmapped.exp
@@ -0,0 +1,46 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+if {[is_remote target]} {
+ # gdbserver prints the warning message but expect is parsing only the
+ # GDB output, not the gdbserver output.
+ return 0
+}
+
+standard_testfile
+
+if { [prepare_for_testing ${testfile}.exp ${testfile}] } {
+ return -1
+}
+
+if ![runto breakpt] {
+ return -1
+}
+
+# Basic attempt to read memory from globals.
+gdb_test "x/5w global_var_1" \
+ "$hex:\[ \t\]+0\[ \t\]+0\[ \t\]+0\[ \t\]+0\r\n$hex:\[ \t\]+Cannot access memory at address $hex"
+gdb_test "x/5w global_var_2" \
+ "$hex:\[ \t\]+Cannot access memory at address $hex"
+
+# Now try a find starting from each global.
+gdb_test "find global_var_0, global_var_2, 0xff" \
+ "warning: Unable to access $decimal bytes of target memory at $hex, halting search\.\r\nPattern not found."
+
+gdb_test "find global_var_1, global_var_2, 0xff" \
+ "warning: Unable to access $decimal bytes of target memory at $hex, halting search\.\r\nPattern not found."
+
+gdb_test "find global_var_2, (global_var_2 + 16), 0xff" \
+ "warning: Unable to access $decimal bytes of target memory at $hex, halting search\.\r\nPattern not found."