This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Include access size in find command error message


Hi Andrew,

Thanks for doing this.  It looks all good to me.  Minor comments 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.

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.

> gdb/ChangeLog
> 
> 2012-09-26  Andrew Burgess  <aburgess@broadcom.com>
> 
> 	* target.c (simple_search_memory): Include access length in error
> 	message.

Ditto.

>  
> 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.

> 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
> @@ -0,0 +1,89 @@
> +/* 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.
> +
> +    .----.----.----.----.----.
> +    |    |    |    |    |    |
> +    '----'----'----'----'----'
> +    |<- 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?

> +
> +  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));

Space after * missing.

> +
> +  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;

Unnecessary parens in several places (around * and /).

> +
> +  /* 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..341538b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/find-unmapped.exp
> @@ -0,0 +1,48 @@
> +# 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.  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.

> +gdb_test "find global_var_1, (global_var_2 + 16), 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."
> +
> +gdb_test "find global_var_0, (global_var_2 + 16), 0xff" \
> +    "warning: Unable to access $decimal bytes of target memory at $hex, halting search\.\r\nPattern not found."

-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]