[PATCH] btrace, vdso: add vdso target sections

[PATCH] btrace, vdso: add vdso target sections

[Markus Metzger]

When loading symbols for the vdso, also add its sections to target_sections.

This fixes an issue with record btrace where vdso instructions could not be
disassembled during replay.

2014-04-04  Markus Metzger  <markus.t.metzger@intel.com>

	* symfile-mem.c (symbol_file_add_from_memory): Add add_sections
	parameter.  Add BFD sections.  Adjust all callers.
	(struct symbol_file_add_from_memory_args): Add add_sections field.

testsuite/
	* gdb.btrace/vdso.c: New.
	* gdb.btrace/vdso.exp: New.


---
 gdb/symfile-mem.c                 | 33 ++++++++++++++++++++++++----
 gdb/testsuite/gdb.btrace/vdso.c   | 30 +++++++++++++++++++++++++
 gdb/testsuite/gdb.btrace/vdso.exp | 46 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/vdso.c
 create mode 100644 gdb/testsuite/gdb.btrace/vdso.exp

diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index 3f09c4d..6f67cd8 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -80,10 +80,12 @@ target_read_memory_bfd (bfd_vma memaddr, bfd_byte *myaddr, bfd_size_type len)
    non-zero, is the known size of the object.  TEMPL is a bfd
    representing the target's format.  NAME is the name to use for this
    symbol file in messages; it can be NULL or a malloc-allocated string
-   which will be attached to the BFD.  */
+   which will be attached to the BFD.  If ADD_SECTIONS is non-zero,
+   add the sections of the loaded BFD.  */
 static struct objfile *
 symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
-			     size_t size, char *name, int from_tty)
+			     size_t size, char *name, int add_sections,
+			     int from_tty)
 {
   struct objfile *objf;
   struct bfd *nbfd;
@@ -131,6 +133,27 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
 				   from_tty ? SYMFILE_VERBOSE : 0,
                                    sai, OBJF_SHARED, NULL);
 
+  if (add_sections)
+    {
+      struct target_section *sections, *sections_end, *tsec;
+
+      sections = NULL;
+      sections_end = NULL;
+      make_cleanup (xfree, sections);
+
+      if (build_section_table (nbfd, &sections, &sections_end) == 0)
+	{
+	  /* Adjust the target section addresses by the load address.  */
+	  for (tsec = sections; tsec != sections_end; ++tsec)
+	    {
+	      tsec->addr += loadbase;
+	      tsec->endaddr += loadbase;
+	    }
+
+	  add_target_sections (&nbfd, sections, sections_end);
+	}
+    }
+
   /* This might change our ideas about frames already looked at.  */
   reinit_frame_cache ();
 
@@ -159,7 +182,7 @@ add_symbol_file_from_memory_command (char *args, int from_tty)
     error (_("Must use symbol-file or exec-file "
 	     "before add-symbol-file-from-memory."));
 
-  symbol_file_add_from_memory (templ, addr, 0, NULL, from_tty);
+  symbol_file_add_from_memory (templ, addr, 0, NULL, 0, from_tty);
 }
 
 /* Arguments for symbol_file_add_from_memory_wrapper.  */
@@ -171,6 +194,7 @@ struct symbol_file_add_from_memory_args
   size_t size;
   char *name;
   int from_tty;
+  int add_sections;
 };
 
 /* Wrapper function for symbol_file_add_from_memory, for
@@ -182,7 +206,7 @@ symbol_file_add_from_memory_wrapper (struct ui_out *uiout, void *data)
   struct symbol_file_add_from_memory_args *args = data;
 
   symbol_file_add_from_memory (args->bfd, args->sysinfo_ehdr, args->size,
-			       args->name, args->from_tty);
+			       args->name, args->add_sections, args->from_tty);
   return 0;
 }
 
@@ -247,6 +271,7 @@ add_vsyscall_page (struct target_ops *target, int from_tty)
 	 vsyscall DSO was not triggered by the user, even if the user
 	 typed "run" at the TTY.  */
       args.from_tty = 0;
+      args.add_sections = 1;
       catch_exceptions (current_uiout, symbol_file_add_from_memory_wrapper,
 			&args, RETURN_MASK_ALL);
     }
diff --git a/gdb/testsuite/gdb.btrace/vdso.c b/gdb/testsuite/gdb.btrace/vdso.c
new file mode 100644
index 0000000..3e73071
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/vdso.c
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   Contributed by Intel Corp. <markus.t.metzger@intel.com>
+
+   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 <sys/time.h>
+
+int
+main (void)
+{
+  struct timeval tv;
+
+  gettimeofday (&tv, 0);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.btrace/vdso.exp b/gdb/testsuite/gdb.btrace/vdso.exp
new file mode 100644
index 0000000..2f415ff
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/vdso.exp
@@ -0,0 +1,46 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2014 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <markus.t.metzger@intel.com>
+#
+# 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/>.
+#
+#
+# Test that we can access the vdso memory during replay for stepping.
+
+# check for btrace support
+if { [skip_btrace_tests] } { return -1 }
+
+# start inferior
+standard_testfile
+if [prepare_for_testing $testfile.exp $testfile $srcfile] {
+    return -1
+}
+if ![runto_main] {
+    return -1
+}
+
+# trace the test code
+gdb_test_no_output "record btrace"
+gdb_test "next"
+
+# start replaying
+gdb_test "reverse-stepi"
+
+# disassemble the code around the current PC
+gdb_test "disassemble \$pc, +10" [join [list \
+	".*" \
+	"End of assembler dump\." \
+    ] "\r\n"]
-- 
1.8.3.1

RE: [PATCH] btrace, vdso: add vdso target sections

[Metzger, Markus T]

Hello Pedro,

Did you find some time to review this?  This is related to the BFD changes you
and Alan Modra were discussing.

It makes GDB use the BFD sections that Alan's patch added to fix an issue with
disassembling vdso code in record btrace.

Thanks,
Markus.

> -----Original Message-----
> From: Metzger, Markus T
> Sent: Friday, April 04, 2014 10:53 AM
> To: palves@redhat.com
> Cc: gdb-patches@sourceware.org
> Subject: [PATCH] btrace, vdso: add vdso target sections
> 
> When loading symbols for the vdso, also add its sections to target_sections.
> 
> This fixes an issue with record btrace where vdso instructions could not be
> disassembled during replay.
> 
> 2014-04-04  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* symfile-mem.c (symbol_file_add_from_memory): Add
> add_sections
> 	parameter.  Add BFD sections.  Adjust all callers.
> 	(struct symbol_file_add_from_memory_args): Add add_sections
> field.
> 
> testsuite/
> 	* gdb.btrace/vdso.c: New.
> 	* gdb.btrace/vdso.exp: New.
> 
> 
> ---
>  gdb/symfile-mem.c                 | 33 ++++++++++++++++++++++++----
>  gdb/testsuite/gdb.btrace/vdso.c   | 30 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.btrace/vdso.exp | 46
> +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 105 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.btrace/vdso.c
>  create mode 100644 gdb/testsuite/gdb.btrace/vdso.exp
> 
> diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
> index 3f09c4d..6f67cd8 100644
> --- a/gdb/symfile-mem.c
> +++ b/gdb/symfile-mem.c
> @@ -80,10 +80,12 @@ target_read_memory_bfd (bfd_vma memaddr,
> bfd_byte *myaddr, bfd_size_type len)
>     non-zero, is the known size of the object.  TEMPL is a bfd
>     representing the target's format.  NAME is the name to use for this
>     symbol file in messages; it can be NULL or a malloc-allocated string
> -   which will be attached to the BFD.  */
> +   which will be attached to the BFD.  If ADD_SECTIONS is non-zero,
> +   add the sections of the loaded BFD.  */
>  static struct objfile *
>  symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
> -			     size_t size, char *name, int from_tty)
> +			     size_t size, char *name, int add_sections,
> +			     int from_tty)
>  {
>    struct objfile *objf;
>    struct bfd *nbfd;
> @@ -131,6 +133,27 @@ symbol_file_add_from_memory (struct bfd *templ,
> CORE_ADDR addr,
>  				   from_tty ? SYMFILE_VERBOSE : 0,
>                                     sai, OBJF_SHARED, NULL);
> 
> +  if (add_sections)
> +    {
> +      struct target_section *sections, *sections_end, *tsec;
> +
> +      sections = NULL;
> +      sections_end = NULL;
> +      make_cleanup (xfree, sections);
> +
> +      if (build_section_table (nbfd, &sections, &sections_end) == 0)
> +	{
> +	  /* Adjust the target section addresses by the load address.  */
> +	  for (tsec = sections; tsec != sections_end; ++tsec)
> +	    {
> +	      tsec->addr += loadbase;
> +	      tsec->endaddr += loadbase;
> +	    }
> +
> +	  add_target_sections (&nbfd, sections, sections_end);
> +	}
> +    }
> +
>    /* This might change our ideas about frames already looked at.  */
>    reinit_frame_cache ();
> 
> @@ -159,7 +182,7 @@ add_symbol_file_from_memory_command (char
> *args, int from_tty)
>      error (_("Must use symbol-file or exec-file "
>  	     "before add-symbol-file-from-memory."));
> 
> -  symbol_file_add_from_memory (templ, addr, 0, NULL, from_tty);
> +  symbol_file_add_from_memory (templ, addr, 0, NULL, 0, from_tty);
>  }
> 
>  /* Arguments for symbol_file_add_from_memory_wrapper.  */
> @@ -171,6 +194,7 @@ struct symbol_file_add_from_memory_args
>    size_t size;
>    char *name;
>    int from_tty;
> +  int add_sections;
>  };
> 
>  /* Wrapper function for symbol_file_add_from_memory, for
> @@ -182,7 +206,7 @@ symbol_file_add_from_memory_wrapper (struct
> ui_out *uiout, void *data)
>    struct symbol_file_add_from_memory_args *args = data;
> 
>    symbol_file_add_from_memory (args->bfd, args->sysinfo_ehdr, args-
> >size,
> -			       args->name, args->from_tty);
> +			       args->name, args->add_sections, args-
> >from_tty);
>    return 0;
>  }
> 
> @@ -247,6 +271,7 @@ add_vsyscall_page (struct target_ops *target, int
> from_tty)
>  	 vsyscall DSO was not triggered by the user, even if the user
>  	 typed "run" at the TTY.  */
>        args.from_tty = 0;
> +      args.add_sections = 1;
>        catch_exceptions (current_uiout,
> symbol_file_add_from_memory_wrapper,
>  			&args, RETURN_MASK_ALL);
>      }
> diff --git a/gdb/testsuite/gdb.btrace/vdso.c
> b/gdb/testsuite/gdb.btrace/vdso.c
> new file mode 100644
> index 0000000..3e73071
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/vdso.c
> @@ -0,0 +1,30 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2014 Free Software Foundation, Inc.
> +
> +   Contributed by Intel Corp. <markus.t.metzger@intel.com>
> +
> +   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 <sys/time.h>
> +
> +int
> +main (void)
> +{
> +  struct timeval tv;
> +
> +  gettimeofday (&tv, 0);
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.btrace/vdso.exp
> b/gdb/testsuite/gdb.btrace/vdso.exp
> new file mode 100644
> index 0000000..2f415ff
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/vdso.exp
> @@ -0,0 +1,46 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2014 Free Software Foundation, Inc.
> +#
> +# Contributed by Intel Corp. <markus.t.metzger@intel.com>
> +#
> +# 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/>.
> +#
> +#
> +# Test that we can access the vdso memory during replay for stepping.
> +
> +# check for btrace support
> +if { [skip_btrace_tests] } { return -1 }
> +
> +# start inferior
> +standard_testfile
> +if [prepare_for_testing $testfile.exp $testfile $srcfile] {
> +    return -1
> +}
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# trace the test code
> +gdb_test_no_output "record btrace"
> +gdb_test "next"
> +
> +# start replaying
> +gdb_test "reverse-stepi"
> +
> +# disassemble the code around the current PC
> +gdb_test "disassemble \$pc, +10" [join [list \
> +	".*" \
> +	"End of assembler dump\." \
> +    ] "\r\n"]
> --
> 1.8.3.1

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH] btrace, vdso: add vdso target sections

[Pedro Alves]

Not yet, sorry, but I will soon.  I'm busy fixing a few
regressions exposed by an earlier run control series.  I
think I have good fixes already, and should hopefully be
posting them today or tomorrow...

-- 
Pedro Alves

On 04/22/2014 03:31 PM, Metzger, Markus T wrote:
> Hello Pedro,
> 
> Did you find some time to review this?  This is related to the BFD changes you
> and Alan Modra were discussing.
> 
> It makes GDB use the BFD sections that Alan's patch added to fix an issue with
> disassembling vdso code in record btrace.
> 
> Thanks,
> Markus.
> 
>> -----Original Message-----
>> From: Metzger, Markus T
>> Sent: Friday, April 04, 2014 10:53 AM
>> To: palves@redhat.com
>> Cc: gdb-patches@sourceware.org
>> Subject: [PATCH] btrace, vdso: add vdso target sections
>>
>> When loading symbols for the vdso, also add its sections to target_sections.
>>
>> This fixes an issue with record btrace where vdso instructions could not be
>> disassembled during replay.
>>
>> 2014-04-04  Markus Metzger  <markus.t.metzger@intel.com>
>>
>> 	* symfile-mem.c (symbol_file_add_from_memory): Add
>> add_sections
>> 	parameter.  Add BFD sections.  Adjust all callers.
>> 	(struct symbol_file_add_from_memory_args): Add add_sections
>> field.
>>
>> testsuite/
>> 	* gdb.btrace/vdso.c: New.
>> 	* gdb.btrace/vdso.exp: New.
>>
>>
>> ---
>>  gdb/symfile-mem.c                 | 33 ++++++++++++++++++++++++----
>>  gdb/testsuite/gdb.btrace/vdso.c   | 30 +++++++++++++++++++++++++
>>  gdb/testsuite/gdb.btrace/vdso.exp | 46
>> +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 105 insertions(+), 4 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.btrace/vdso.c
>>  create mode 100644 gdb/testsuite/gdb.btrace/vdso.exp
>>
>> diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
>> index 3f09c4d..6f67cd8 100644
>> --- a/gdb/symfile-mem.c
>> +++ b/gdb/symfile-mem.c
>> @@ -80,10 +80,12 @@ target_read_memory_bfd (bfd_vma memaddr,
>> bfd_byte *myaddr, bfd_size_type len)
>>     non-zero, is the known size of the object.  TEMPL is a bfd
>>     representing the target's format.  NAME is the name to use for this
>>     symbol file in messages; it can be NULL or a malloc-allocated string
>> -   which will be attached to the BFD.  */
>> +   which will be attached to the BFD.  If ADD_SECTIONS is non-zero,
>> +   add the sections of the loaded BFD.  */
>>  static struct objfile *
>>  symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
>> -			     size_t size, char *name, int from_tty)
>> +			     size_t size, char *name, int add_sections,
>> +			     int from_tty)
>>  {
>>    struct objfile *objf;
>>    struct bfd *nbfd;
>> @@ -131,6 +133,27 @@ symbol_file_add_from_memory (struct bfd *templ,
>> CORE_ADDR addr,
>>  				   from_tty ? SYMFILE_VERBOSE : 0,
>>                                     sai, OBJF_SHARED, NULL);
>>
>> +  if (add_sections)
>> +    {
>> +      struct target_section *sections, *sections_end, *tsec;
>> +
>> +      sections = NULL;
>> +      sections_end = NULL;
>> +      make_cleanup (xfree, sections);
>> +
>> +      if (build_section_table (nbfd, &sections, &sections_end) == 0)
>> +	{
>> +	  /* Adjust the target section addresses by the load address.  */
>> +	  for (tsec = sections; tsec != sections_end; ++tsec)
>> +	    {
>> +	      tsec->addr += loadbase;
>> +	      tsec->endaddr += loadbase;
>> +	    }
>> +
>> +	  add_target_sections (&nbfd, sections, sections_end);
>> +	}
>> +    }
>> +
>>    /* This might change our ideas about frames already looked at.  */
>>    reinit_frame_cache ();
>>
>> @@ -159,7 +182,7 @@ add_symbol_file_from_memory_command (char
>> *args, int from_tty)
>>      error (_("Must use symbol-file or exec-file "
>>  	     "before add-symbol-file-from-memory."));
>>
>> -  symbol_file_add_from_memory (templ, addr, 0, NULL, from_tty);
>> +  symbol_file_add_from_memory (templ, addr, 0, NULL, 0, from_tty);
>>  }
>>
>>  /* Arguments for symbol_file_add_from_memory_wrapper.  */
>> @@ -171,6 +194,7 @@ struct symbol_file_add_from_memory_args
>>    size_t size;
>>    char *name;
>>    int from_tty;
>> +  int add_sections;
>>  };
>>
>>  /* Wrapper function for symbol_file_add_from_memory, for
>> @@ -182,7 +206,7 @@ symbol_file_add_from_memory_wrapper (struct
>> ui_out *uiout, void *data)
>>    struct symbol_file_add_from_memory_args *args = data;
>>
>>    symbol_file_add_from_memory (args->bfd, args->sysinfo_ehdr, args-
>>> size,
>> -			       args->name, args->from_tty);
>> +			       args->name, args->add_sections, args-
>>> from_tty);
>>    return 0;
>>  }
>>
>> @@ -247,6 +271,7 @@ add_vsyscall_page (struct target_ops *target, int
>> from_tty)
>>  	 vsyscall DSO was not triggered by the user, even if the user
>>  	 typed "run" at the TTY.  */
>>        args.from_tty = 0;
>> +      args.add_sections = 1;
>>        catch_exceptions (current_uiout,
>> symbol_file_add_from_memory_wrapper,
>>  			&args, RETURN_MASK_ALL);
>>      }
>> diff --git a/gdb/testsuite/gdb.btrace/vdso.c
>> b/gdb/testsuite/gdb.btrace/vdso.c
>> new file mode 100644
>> index 0000000..3e73071
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.btrace/vdso.c
>> @@ -0,0 +1,30 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2014 Free Software Foundation, Inc.
>> +
>> +   Contributed by Intel Corp. <markus.t.metzger@intel.com>
>> +
>> +   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 <sys/time.h>
>> +
>> +int
>> +main (void)
>> +{
>> +  struct timeval tv;
>> +
>> +  gettimeofday (&tv, 0);
>> +
>> +  return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.btrace/vdso.exp
>> b/gdb/testsuite/gdb.btrace/vdso.exp
>> new file mode 100644
>> index 0000000..2f415ff
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.btrace/vdso.exp
>> @@ -0,0 +1,46 @@
>> +# This testcase is part of GDB, the GNU debugger.
>> +#
>> +# Copyright 2014 Free Software Foundation, Inc.
>> +#
>> +# Contributed by Intel Corp. <markus.t.metzger@intel.com>
>> +#
>> +# 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/>.
>> +#
>> +#
>> +# Test that we can access the vdso memory during replay for stepping.
>> +
>> +# check for btrace support
>> +if { [skip_btrace_tests] } { return -1 }
>> +
>> +# start inferior
>> +standard_testfile
>> +if [prepare_for_testing $testfile.exp $testfile $srcfile] {
>> +    return -1
>> +}
>> +if ![runto_main] {
>> +    return -1
>> +}
>> +
>> +# trace the test code
>> +gdb_test_no_output "record btrace"
>> +gdb_test "next"
>> +
>> +# start replaying
>> +gdb_test "reverse-stepi"
>> +
>> +# disassemble the code around the current PC
>> +gdb_test "disassemble \$pc, +10" [join [list \
>> +	".*" \
>> +	"End of assembler dump\." \
>> +    ] "\r\n"]
>> --
>> 1.8.3.1
> 
> Intel GmbH
> Dornacher Strasse 1
> 85622 Feldkirchen/Muenchen, Deutschland
> Sitz der Gesellschaft: Feldkirchen bei Muenchen
> Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
> Registergericht: Muenchen HRB 47456
> Ust.-IdNr./VAT Registration No.: DE129385895
> Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
> 

Re: [PATCH] btrace, vdso: add vdso target sections

[Pedro Alves]

Hi Markus,

Sorry for the delay...

Looks largely fine, but ...

On 04/04/2014 09:53 AM, Markus Metzger wrote:

> @@ -131,6 +133,27 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
>  				   from_tty ? SYMFILE_VERBOSE : 0,
>                                     sai, OBJF_SHARED, NULL);
>  
> +  if (add_sections)
> +    {
...

Why is this conditional?  Why not add the sections if the symbols are
added manually with add-symbol-file-from-memory?  The use case for the
command was originally exactly to add the vdso's symbols to GDB.

> +	}
> +    }
> +
>    /* This might change our ideas about frames already looked at.  */
>    reinit_frame_cache ();
>  
> @@ -159,7 +182,7 @@ add_symbol_file_from_memory_command (char *args, int from_tty)
>      error (_("Must use symbol-file or exec-file "
>  	     "before add-symbol-file-from-memory."));
>  
> -  symbol_file_add_from_memory (templ, addr, 0, NULL, from_tty);
> +  symbol_file_add_from_memory (templ, addr, 0, NULL, 0, from_tty);



> +# trace the test code
> +gdb_test_no_output "record btrace"
> +gdb_test "next"

Please add a pattern that makes sure the "next" actually
finished successfully.

> +
> +# start replaying
> +gdb_test "reverse-stepi"

Likewise.

> +
> +# disassemble the code around the current PC
> +gdb_test "disassemble \$pc, +10" [join [list \
> +	".*" \
> +	"End of assembler dump\." \
> +    ] "\r\n"]

What is the error one gets without the fix?  Doesn't
GDB say "End of assembler dump" in that case too?

-- 
Pedro Alves

RE: [PATCH] btrace, vdso: add vdso target sections

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Friday, May 16, 2014 3:00 PM

Thanks for your review.


> Looks largely fine, but ...
> 
> On 04/04/2014 09:53 AM, Markus Metzger wrote:
> 
> > @@ -131,6 +133,27 @@ symbol_file_add_from_memory (struct bfd
> *templ, CORE_ADDR addr,
> >  				   from_tty ? SYMFILE_VERBOSE : 0,
> >                                     sai, OBJF_SHARED, NULL);
> >
> > +  if (add_sections)
> > +    {
> ...
> 
> Why is this conditional?  Why not add the sections if the symbols are
> added manually with add-symbol-file-from-memory?  The use case for the
> command was originally exactly to add the vdso's symbols to GDB.

This function is used by the add-symbol-file-from-memory command, as well.

I'm not sure if we want to add the target sections there, as well.


> > +# trace the test code
> > +gdb_test_no_output "record btrace"
> > +gdb_test "next"
> 
> Please add a pattern that makes sure the "next" actually
> finished successfully.

OK.


> > +# disassemble the code around the current PC
> > +gdb_test "disassemble \$pc, +10" [join [list \
> > +	".*" \
> > +	"End of assembler dump\." \
> > +    ] "\r\n"]
> 
> What is the error one gets without the fix?  Doesn't
> GDB say "End of assembler dump" in that case too?

No.  GDB says "Cannot access memory at address ...".

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

RE: [PATCH] btrace, vdso: add vdso target sections

[Metzger, Markus T]

> -----Original Message-----
> From: Metzger, Markus T
> Sent: Monday, May 19, 2014 10:06 AM


> > > +# trace the test code
> > > +gdb_test_no_output "record btrace"
> > > +gdb_test "next"
> >
> > Please add a pattern that makes sure the "next" actually
> > finished successfully.

There's another problem that showed when I added such a
pattern for the "reverse-stepi" command.

The command prints "Cannot access memory at address 0x4004b0".
The error occurs during frame unwind when we try to
disassemble an instruction in order to get its length.

The problem is that the GDB memory cache may turn reads from
one section into reads from a different section or from memory
regions outside of any section.

The address, 0x4004b0 is the first entry in .plt, a read-only code
section.  The disassembler tries to read 1 byte from this address.
The memory cache turns this into a request for 64 bytes from
0x400480, which lies in a different section, .rela.plt in my case.

The read still succeeds in my example since the other section is
also readonly, but there's no guarantee for this.

The memory read passes through record_btrace_xfer_partial
which reduces the length to fit into a single section, so the target's
read memory function tries to read the remainder of the cache line.

This eventually fails since the cache line contains a memory region
that is not contained in any section and record_btrace_xfer_partial
returns TARGET_XFER_UNAVAILABLE.


I would argue that the memory cache should not extend the original
read request beyond section boundaries.  What do you think?

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH] btrace, vdso: add vdso target sections

[Pedro Alves]

On 05/19/2014 09:05 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
>> owner@sourceware.org] On Behalf Of Pedro Alves
>> Sent: Friday, May 16, 2014 3:00 PM
> 
> Thanks for your review.
> 
> 
>> Looks largely fine, but ...
>>
>> On 04/04/2014 09:53 AM, Markus Metzger wrote:
>>
>>> @@ -131,6 +133,27 @@ symbol_file_add_from_memory (struct bfd
>> *templ, CORE_ADDR addr,
>>>  				   from_tty ? SYMFILE_VERBOSE : 0,
>>>                                     sai, OBJF_SHARED, NULL);
>>>
>>> +  if (add_sections)
>>> +    {
>> ...
>>
>> Why is this conditional?  Why not add the sections if the symbols are
>> added manually with add-symbol-file-from-memory?  The use case for the
>> command was originally exactly to add the vdso's symbols to GDB.
> 
> This function is used by the add-symbol-file-from-memory command, as well.
> 
> I'm not sure if we want to add the target sections there, as well.

If there's no good reason, then I'd rather the command installed
the sections as well.  add-symbol-file-from-memory was invented to
add the vdso manually.  And in that case, we'd want btrace to work
just the same.

>>> +# disassemble the code around the current PC
>>> +gdb_test "disassemble \$pc, +10" [join [list \
>>> +	".*" \
>>> +	"End of assembler dump\." \
>>> +    ] "\r\n"]
>>
>> What is the error one gets without the fix?  Doesn't
>> GDB say "End of assembler dump" in that case too?
> 
> No.  GDB says "Cannot access memory at address ...".

Alright.  I was thinking that that might be a little
brittle: if GDB changes that for some reason, the test will no
longer catch problems.  I think the best test is to make sure the
disassemble output when replaying is the same as when not
replaying at all.

Say, borrow the idea of capture_command_output from gcore.exp,
and do

proc disassemble { what test } {
    global gdb_prompt
    global expect_out

    set output_string ""
    set command "disassemble $what"
    gdb_test_multiple "$command" "$command" {
	-re "${command}\[\r\n\]+(.*)End of assembler dump\.\r\n$gdb_prompt $" {
	    set output_string $expect_out(1,string)
	    pass $command
	}
    }
    return $output_string
}

set pre_btrace_disasm [disassemble "gettimeofday"]

# trace the test code
gdb_test_no_output "record btrace"
gdb_test "next"

# start replaying
gdb_test "reverse-stepi"

with_test_prefix "replay" {
  set post_btrace_disasm [disassemble "gettimeofday"]

  set test "disassembly output matched"
  if ![string pre_btrace_disasm $post_btrace_disasm]  {
    pass $test
  } else {
    fail $test
  }
}

Also, how about making sure we're actually testing the
vdso code, by e.g., expecting "Dump of assembler code for
function __vdso_gettimeofday", or making sure the disassembled
region is within what "info proc maps" shows is the
vdso region?

The idea would be that if the kernel ever changes, we'd
realize that the test is no longer disassembling the
vdso (due to a FAIL/UNSUPPORTED, etc.), thus no longer
exercising what the test was meant to exercise.

-- 
Pedro Alves

Re: [PATCH] btrace, vdso: add vdso target sections

[Pedro Alves]

On 05/19/2014 12:30 PM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Metzger, Markus T
>> Sent: Monday, May 19, 2014 10:06 AM
> 
> 
>>>> +# trace the test code
>>>> +gdb_test_no_output "record btrace"
>>>> +gdb_test "next"
>>>
>>> Please add a pattern that makes sure the "next" actually
>>> finished successfully.
> 
> There's another problem that showed when I added such a
> pattern for the "reverse-stepi" command.
> 
> The command prints "Cannot access memory at address 0x4004b0".
> The error occurs during frame unwind when we try to
> disassemble an instruction in order to get its length.
> 
> The problem is that the GDB memory cache may turn reads from
> one section into reads from a different section or from memory
> regions outside of any section.
> 
> The address, 0x4004b0 is the first entry in .plt, a read-only code
> section.  The disassembler tries to read 1 byte from this address.
> The memory cache turns this into a request for 64 bytes from
> 0x400480, which lies in a different section, .rela.plt in my case.
> 
> The read still succeeds in my example since the other section is
> also readonly, but there's no guarantee for this.
> 
> The memory read passes through record_btrace_xfer_partial
> which reduces the length to fit into a single section, so the target's
> read memory function tries to read the remainder of the cache line.

I got a bit confused by the above sentence.  You must mean, a
function in target.c (target_read, etc.), not the target's
read memory function.

> This eventually fails since the cache line contains a memory region
> that is not contained in any section and record_btrace_xfer_partial
> returns TARGET_XFER_UNAVAILABLE.
> I would argue that the memory cache should not extend the original
> read request beyond section boundaries.  What do you think?

Even in absence of section information, the cache should still be able to
handle the case of the target returning TARGET_XFER_UNAVAILABLE or
TARGET_XFER_E_IO or any error for memory that is outside the region
that the original caller of the memory read routine asked for.
Looks like that fallback is missing.

Does this patch fix it ?

---

 gdb/dcache.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/dcache.c b/gdb/dcache.c
index d3b546b..e75f583 100644
--- a/gdb/dcache.c
+++ b/gdb/dcache.c
@@ -497,8 +497,11 @@ dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache,

   if (i == 0)
     {
-      /* FIXME: We lose the real error status.  */
-      return TARGET_XFER_E_IO;
+      /* Even though reading the whole line failed, we may be able to
+	 read a piece starting where the caller wanted.  */
+      return ops->to_xfer_partial (ops, TARGET_OBJECT_RAW_MEMORY, NULL,
+				   myaddr, NULL, memaddr, len,
+				   xfered_len);
     }
   else
     {

RE: [PATCH] btrace, vdso: add vdso target sections

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Monday, May 19, 2014 11:41 PM
> To: Metzger, Markus T

> Even in absence of section information, the cache should still be able to
> handle the case of the target returning TARGET_XFER_UNAVAILABLE or
> TARGET_XFER_E_IO or any error for memory that is outside the region
> that the original caller of the memory read routine asked for.
> Looks like that fallback is missing.
> 
> Does this patch fix it ?

It does when we request TARGET_OBJECT_MEMORY.

Thanks,
Markus.


> On 05/19/2014 12:30 PM, Metzger, Markus T wrote:
> >> -----Original Message-----
> >> From: Metzger, Markus T
> >> Sent: Monday, May 19, 2014 10:06 AM
> >
> >
> >>>> +# trace the test code
> >>>> +gdb_test_no_output "record btrace"
> >>>> +gdb_test "next"
> >>>
> >>> Please add a pattern that makes sure the "next" actually
> >>> finished successfully.
> >
> > There's another problem that showed when I added such a
> > pattern for the "reverse-stepi" command.
> >
> > The command prints "Cannot access memory at address 0x4004b0".
> > The error occurs during frame unwind when we try to
> > disassemble an instruction in order to get its length.
> >
> > The problem is that the GDB memory cache may turn reads from
> > one section into reads from a different section or from memory
> > regions outside of any section.
> >
> > The address, 0x4004b0 is the first entry in .plt, a read-only code
> > section.  The disassembler tries to read 1 byte from this address.
> > The memory cache turns this into a request for 64 bytes from
> > 0x400480, which lies in a different section, .rela.plt in my case.
> >
> > The read still succeeds in my example since the other section is
> > also readonly, but there's no guarantee for this.
> >
> > The memory read passes through record_btrace_xfer_partial
> > which reduces the length to fit into a single section, so the target's
> > read memory function tries to read the remainder of the cache line.
> 
> I got a bit confused by the above sentence.  You must mean, a
> function in target.c (target_read, etc.), not the target's
> read memory function.
> 
> > This eventually fails since the cache line contains a memory region
> > that is not contained in any section and record_btrace_xfer_partial
> > returns TARGET_XFER_UNAVAILABLE.
> > I would argue that the memory cache should not extend the original
> > read request beyond section boundaries.  What do you think?
> 
> Even in absence of section information, the cache should still be able to
> handle the case of the target returning TARGET_XFER_UNAVAILABLE or
> TARGET_XFER_E_IO or any error for memory that is outside the region
> that the original caller of the memory read routine asked for.
> Looks like that fallback is missing.
> 
> Does this patch fix it ?
> 
> ---
> 
>  gdb/dcache.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/dcache.c b/gdb/dcache.c
> index d3b546b..e75f583 100644
> --- a/gdb/dcache.c
> +++ b/gdb/dcache.c
> @@ -497,8 +497,11 @@ dcache_read_memory_partial (struct target_ops
> *ops, DCACHE *dcache,
> 
>    if (i == 0)
>      {
> -      /* FIXME: We lose the real error status.  */
> -      return TARGET_XFER_E_IO;
> +      /* Even though reading the whole line failed, we may be able to
> +	 read a piece starting where the caller wanted.  */
> +      return ops->to_xfer_partial (ops, TARGET_OBJECT_RAW_MEMORY,
> NULL,
> +				   myaddr, NULL, memaddr, len,
> +				   xfered_len);
>      }
>    else
>      {

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH] btrace, vdso: add vdso target sections

[Pedro Alves]

On 05/20/2014 07:40 AM, Metzger, Markus T wrote:

>> Does this patch fix it ?
> 
> It does when we request TARGET_OBJECT_MEMORY.

You mean, the patch should have been:

 -     return ops->to_xfer_partial (ops, TARGET_OBJECT_RAW_MEMORY,
 +     return ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY,

?

-- 
Pedro Alves

RE: [PATCH] btrace, vdso: add vdso target sections

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, May 20, 2014 1:12 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH] btrace, vdso: add vdso target sections
> 
> On 05/20/2014 07:40 AM, Metzger, Markus T wrote:
> 
> >> Does this patch fix it ?
> >
> > It does when we request TARGET_OBJECT_MEMORY.
> 
> You mean, the patch should have been:
> 
>  -     return ops->to_xfer_partial (ops, TARGET_OBJECT_RAW_MEMORY,
>  +     return ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY,
> 
> ?

Yes.

Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

[PATCH] Make the dcache (code/stack cache) handle line reading errors better.

[Pedro Alves]

On 05/20/2014 12:14 PM, Metzger, Markus T wrote:
>> From: Pedro Alves [mailto:palves@redhat.com]
>> On 05/20/2014 07:40 AM, Metzger, Markus T wrote:
>>
>>>> Does this patch fix it ?
>>>
>>> It does when we request TARGET_OBJECT_MEMORY.
>>
>> You mean, the patch should have been:
>>
>>  -     return ops->to_xfer_partial (ops, TARGET_OBJECT_RAW_MEMORY,
>>  +     return ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY,
>>
>> ?
> 
> Yes.

Oh, indeed.  The targets should never see requests for any other
kind of memory but TARGET_OBJECT_MEMORY.  I even recall now
pushing btrace in that direction.  :-)

Here's a full patch.  I managed to come up with a test that
doesn't depend on btrace.  Let me know what you think.

8<---------------
From: Pedro Alves <palves@redhat.com>
Date: Tue, 20 May 2014 12:26:01 +0100
Subject: [PATCH] Make the dcache (code/stack cache) handle line reading errors
 better.

The dcache (code/stack cache) is supposed to be transparent, but it's
actually not in one case.  dcache tries to read chunks (cache lines)
at a time off of the target.  This may end up trying to read
unaccessible or unavailable memory.  Currently the caller an xfer error
in this case.  But if the specific bits of memory the caller actually
wanted are available and accessible, then the caller should get the
memory it wanted, not an error.

gdb/
2014-05-20  Pedro Alves  <palves@redhat.com>

	* dcache.c (dcache_read_memory_partial): If reading the cache line
	fails, fallback to reading just the memory the caller wanted.

gdb/testsuite/
2014-05-20  Pedro Alves  <palves@redhat.com>

	* gdb.base/dcache-line-read-error.c
	* gdb.base/dcache-line-read-error.exp
---
 gdb/dcache.c                                      |   7 +-
 gdb/testsuite/gdb.base/dcache-line-read-error.c   | 108 ++++++++++++++++++++++
 gdb/testsuite/gdb.base/dcache-line-read-error.exp |  84 +++++++++++++++++
 3 files changed, 197 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dcache-line-read-error.c
 create mode 100644 gdb/testsuite/gdb.base/dcache-line-read-error.exp

diff --git a/gdb/dcache.c b/gdb/dcache.c
index d3b546b..9780f4d 100644
--- a/gdb/dcache.c
+++ b/gdb/dcache.c
@@ -497,8 +497,11 @@ dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache,
 
   if (i == 0)
     {
-      /* FIXME: We lose the real error status.  */
-      return TARGET_XFER_E_IO;
+      /* Even though reading the whole line failed, we may be able to
+	 read a piece starting where the caller wanted.  */
+      return ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY, NULL,
+				   myaddr, NULL, memaddr, len,
+				   xfered_len);
     }
   else
     {
diff --git a/gdb/testsuite/gdb.base/dcache-line-read-error.c b/gdb/testsuite/gdb.base/dcache-line-read-error.c
new file mode 100644
index 0000000..86e512d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dcache-line-read-error.c
@@ -0,0 +1,108 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2014 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>
+
+size_t pg_size;
+void *first_mapped_page;
+void *first_unmapped_page;
+void *last_mapped_page;
+void *last_unmapped_page;
+
+void
+breakpt (void)
+{
+  /* Nothing. */
+}
+
+int
+main (void)
+{
+  void *p;
+  int pg_count;
+  size_t i;
+
+  /* Map 6 contiguous pages, and then unmap all second first, and
+     second last.
+
+     From GDB we will disassemble each of the _mapped_ pages, with a
+     code-cache (dcache) line size bigger than the page size (twice
+     bigger).  This makes GDB try to read one page before the mapped
+     page once, and the page after another time.  GDB should give no
+     error in either case.
+
+     That is, depending on what the kernel gives up, we get either:
+
+      .---.---.---.---.---.---.
+      | U | M | U | U | M | U |
+      '---'---'---'---'---'---.
+      |       |       |       |  <- line alignment
+       ^^^^^^^         ^^^^^^^
+          |               |
+          + line1         + line2
+
+     Or:
+
+      .---.---.---.---.---.---.
+      | U | M | U | U | M | U |
+      '---'---'---'---'---'---.
+          |       |       |      <- line alignment
+           ^^^^^^^ ^^^^^^^
+              |       |
+        line1 +       + line2
+
+    Note we really want to test that dcache behaves correctly when
+    reading a cache line fails.  We're just using unmapped memory as
+    proxy for any kind of error.  */
+
+  pg_size = getpagesize ();
+  pg_count = 6;
+
+  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;
+    }
+
+  /* Disassembling 0s should behave on all targets.  */
+  memset (p, 0, pg_count * pg_size);
+
+  for (i = 0; i < pg_count; i++)
+    {
+      if (i == 1 || i == 4)
+	continue;
+
+      if (munmap (p + (i * pg_size), pg_size) == -1)
+	{
+	  perror ("munmap");
+	  return EXIT_FAILURE;
+	}
+    }
+
+  first_mapped_page = p + 1 * pg_size;;
+  last_mapped_page = p + 4 * pg_size;
+
+  breakpt ();
+
+  return EXIT_SUCCESS;
+}
diff --git a/gdb/testsuite/gdb.base/dcache-line-read-error.exp b/gdb/testsuite/gdb.base/dcache-line-read-error.exp
new file mode 100644
index 0000000..c3b2f9f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dcache-line-read-error.exp
@@ -0,0 +1,84 @@
+# Copyright 2014 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/>.
+
+# Test that dcache behaves correctly when reading a cache line fails.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile}] } {
+    return -1
+}
+
+if ![runto breakpt] {
+    return -1
+}
+
+# Disassemble WHAT.
+
+proc disassemble { what } {
+    global hex gdb_prompt
+
+    set cmd "disassemble $what"
+    gdb_test_multiple $cmd $cmd {
+	-re "Cannot access memory.*$gdb_prompt $" {
+	    fail $cmd
+	}
+	-re "End of assembler dump\.\r\n$gdb_prompt $" {
+	    pass $cmd
+	}
+    }
+}
+
+# Issue the "delete mem" command.  This makes GDB ignore the
+# target-provided list, if any.
+
+proc delete_mem {} {
+    global gdb_prompt
+
+    set test "delete mem"
+    gdb_test_multiple $test $test {
+       -re "Delete all memory regions.*y or n.*$" {
+           send_gdb "y\n"
+           exp_continue
+       }
+       -re "$gdb_prompt $" {
+	   pass $test
+       }
+    }
+}
+
+# Make the dcache line size bigger than the pagesize.
+set pagesize [get_integer_valueof "pg_size" -1]
+set linesize [expr $pagesize * 2]
+
+gdb_test_no_output "set dcache line-size $linesize" \
+    "set dcache line size to twice the pagesize"
+
+gdb_test "info dcache" \
+    "Dcache 4096 lines of $linesize bytes each.\r\nNo data cache available."
+
+# Make sure dcache doesn't automatically skip unmapped regions.
+delete_mem
+
+gdb_test "info mem" \
+    "Using user-defined memory regions.\r\nThere are no memory regions defined\."
+
+# Given the line size is bigger than the page size, we have
+# alternating mapped and unmapped pages, these make dcache fail to
+# fill in the cache line.  GDB used to have a bug where that failure
+# would end up as user-visible error.  The range being disassembled is
+# wholly available, so GDB should succeed.
+disassemble "first_mapped_page, +10"
+disassemble "last_mapped_page, +10"
-- 
1.9.0

[PATCH v2] Make the dcache (code/stack cache) handle line reading errors better

[Pedro Alves]

On 05/21/2014 07:51 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
>> owner@sourceware.org] On Behalf Of Pedro Alves
>> Sent: Tuesday, May 20, 2014 7:15 PM
> 
>> 8<---------------
>> From: Pedro Alves <palves@redhat.com>
>> Date: Tue, 20 May 2014 12:26:01 +0100
>> Subject: [PATCH] Make the dcache (code/stack cache) handle line reading
>> errors
>>  better.
>>
>> The dcache (code/stack cache) is supposed to be transparent, but it's
>> actually not in one case.  dcache tries to read chunks (cache lines)
>> at a time off of the target.  This may end up trying to read
>> unaccessible or unavailable memory.  Currently the caller an xfer error
> 
> 'gets' is missing.

Thanks, fixed.  I've fixed a couple typos in the test .c file as well.

> 
>> in this case.  But if the specific bits of memory the caller actually
>> wanted are available and accessible, then the caller should get the
>> memory it wanted, not an error.
> 
> 
>> +   Copyright 2012-2014 Free Software Foundation, Inc.
> 
> Why 2012-?

Because the file was originally copied from
gdb.base/find-unmapped.c and it still retains bits under
2012-2014 copyright.

> 
> 
>> +void *first_mapped_page;
>> +void *first_unmapped_page;
>> +void *last_mapped_page;
>> +void *last_unmapped_page;
> 
> The _unmapped_page variants seem not to be used.

Ah, they were needed in an earlier version.  Thanks, removed.

> 
> 
>> +  /* Disassembling 0s should behave on all targets.  */
>> +  memset (p, 0, pg_count * pg_size);
> 
> Shouldn't be necessary.  The pages are supposed to be zero-initialized.

Indeed, removed.  (find-unmapped.c also has it)

> 
> 
>> +# Test that dcache behaves correctly when reading a cache line fails.
>> +
>> +standard_testfile
>> +
>> +if { [prepare_for_testing "failed to prepare" ${testfile}] } {
> 
> And I always thought you had to supply $srcfile, as well.  Can
> prepare_for_testing derive the source name from the executable name
> and some path magic?

Yeah:

 # Prepares for testing, by calling build_executable, and then clean_restart.
 # Please refer to build_executable for parameter description.
 proc prepare_for_testing { testname executable {sources ""} {options {debug}}} {
 
     if {[build_executable $testname $executable $sources $options] == -1} {
         return -1
     }

{sources ""} is how one specifies optional arguments.  "" is the default
in this case.  And then:

 # Build executable named EXECUTABLE, from SOURCES.  If SOURCES are not
 # provided, uses $EXECUTABLE.c.  The TESTNAME paramer is the name of test
 # to pass to untested, if something is wrong.  OPTIONS are passed
 # to gdb_compile directly.
 proc build_executable { testname executable {sources ""} {options {debug}} } {
     if {[llength $sources]==0} {
         set sources ${executable}.c
     }

Hmm, for standard_testfile tests, looks like we could even
make $testname and $executable be optional arguments.

> 
> 
>> +proc disassemble { what } {
>> +    global hex gdb_prompt
>> +
>> +    set cmd "disassemble $what"
>> +    gdb_test_multiple $cmd $cmd {
>> +	-re "Cannot access memory.*$gdb_prompt $" {
>> +	    fail $cmd
>> +	}
>> +	-re "End of assembler dump\.\r\n$gdb_prompt $" {
>> +	    pass $cmd
>> +	}
> 
> Wouldn't simply checking for "End of assembler dump\.\r\n" suffice?

Yeah.  I was just being explicit, but indeed there's no need.
I've now switched to just use gdb_test.

> Wouldn't it even be more robust as it would catch all kinds of errors?

Nope, gdb_test_multiple issues a FAIL if none of the passed in patterns
match.   And it wouldn't normally timeout internally gdb_test_multiple's
issues a FAIL if it sees:

	-re "\r\n$gdb_prompt $" {
            ...
    		fail "$message"
            ...
        }

Below's v2.  Let me know how it looks.

8<----------------
From f433bc8ad604afa15421c10a7387503257eb9754 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 20 May 2014 12:26:01 +0100
Subject: [PATCH] Make the dcache (code/stack cache) handle line reading errors
 better

The dcache (code/stack cache) is supposed to be transparent, but it's
actually not in one case.  dcache tries to read chunks (cache lines)
at a time off of the target.  This may end up trying to read
unaccessible or unavailable memory.  Currently the caller gets an xfer
error in this case.  But if the specific bits of memory the caller
actually wanted are available and accessible, then the caller should
get the memory it wanted, not an error.

gdb/
2014-05-21  Pedro Alves  <palves@redhat.com>

	* dcache.c (dcache_read_memory_partial): If reading the cache line
	fails, fallback to reading just the memory the caller wanted.

gdb/testsuite/
2014-05-21  Pedro Alves  <palves@redhat.com>

	* gdb.base/dcache-line-read-error.c: New.
	* gdb.base/dcache-line-read-error.exp: New.
---
 gdb/dcache.c                                      |   7 +-
 gdb/testsuite/gdb.base/dcache-line-read-error.c   | 107 ++++++++++++++++++++++
 gdb/testsuite/gdb.base/dcache-line-read-error.exp |  68 ++++++++++++++
 3 files changed, 180 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dcache-line-read-error.c
 create mode 100644 gdb/testsuite/gdb.base/dcache-line-read-error.exp

diff --git a/gdb/dcache.c b/gdb/dcache.c
index d3b546b..9780f4d 100644
--- a/gdb/dcache.c
+++ b/gdb/dcache.c
@@ -497,8 +497,11 @@ dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache,
 
   if (i == 0)
     {
-      /* FIXME: We lose the real error status.  */
-      return TARGET_XFER_E_IO;
+      /* Even though reading the whole line failed, we may be able to
+	 read a piece starting where the caller wanted.  */
+      return ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY, NULL,
+				   myaddr, NULL, memaddr, len,
+				   xfered_len);
     }
   else
     {
diff --git a/gdb/testsuite/gdb.base/dcache-line-read-error.c b/gdb/testsuite/gdb.base/dcache-line-read-error.c
new file mode 100644
index 0000000..4120593
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dcache-line-read-error.c
@@ -0,0 +1,107 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2014 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>
+
+size_t pg_size;
+void *first_mapped_page;
+void *last_mapped_page;
+
+void
+breakpt (void)
+{
+  /* Nothing. */
+}
+
+int
+main (void)
+{
+  void *p;
+  int pg_count;
+  size_t i;
+
+  /* Map 6 contiguous pages, and then unmap all second first, and
+     second last.
+
+     From GDB we will disassemble each of the _mapped_ pages, with a
+     code-cache (dcache) line size bigger than the page size (twice
+     bigger).  This makes GDB try to read one page before the mapped
+     page once, and the page after another time.  GDB should give no
+     error in either case.
+
+     That is, depending on where the kernel aligns the pages, we get
+     either:
+
+      .---.---.---.---.---.---.
+      | U | M | U | U | M | U |
+      '---'---'---'---'---'---.
+      |       |       |       |  <- line alignment
+       ^^^^^^^         ^^^^^^^
+          |               |
+          + line1         + line2
+
+     Or:
+
+      .---.---.---.---.---.---.
+      | U | M | U | U | M | U |
+      '---'---'---'---'---'---.
+          |       |       |      <- line alignment
+           ^^^^^^^ ^^^^^^^
+              |       |
+        line1 +       + line2
+
+    Note we really want to test that dcache behaves correctly when
+    reading a cache line fails.  We're just using unmapped memory as
+    proxy for any kind of error.  */
+
+  pg_size = getpagesize ();
+  pg_count = 6;
+
+  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;
+    }
+
+  /* Leave memory zero-initialized.  Disassembling 0s should behave on
+     all targets.  */
+
+  for (i = 0; i < pg_count; i++)
+    {
+      if (i == 1 || i == 4)
+	continue;
+
+      if (munmap (p + (i * pg_size), pg_size) == -1)
+	{
+	  perror ("munmap");
+	  return EXIT_FAILURE;
+	}
+    }
+
+  first_mapped_page = p + 1 * pg_size;;
+  last_mapped_page = p + 4 * pg_size;
+
+  breakpt ();
+
+  return EXIT_SUCCESS;
+}
diff --git a/gdb/testsuite/gdb.base/dcache-line-read-error.exp b/gdb/testsuite/gdb.base/dcache-line-read-error.exp
new file mode 100644
index 0000000..7e75460
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dcache-line-read-error.exp
@@ -0,0 +1,68 @@
+# Copyright 2014 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/>.
+
+# Test that dcache behaves correctly when reading a cache line fails.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile}] } {
+    return -1
+}
+
+if ![runto breakpt] {
+    return -1
+}
+
+# Issue the "delete mem" command.  This makes GDB ignore the
+# target-provided list, if any.
+
+proc delete_mem {} {
+    global gdb_prompt
+
+    set test "delete mem"
+    gdb_test_multiple $test $test {
+	-re "Delete all memory regions.*y or n.*$" {
+	    send_gdb "y\n"
+	    exp_continue
+	}
+	-re "$gdb_prompt $" {
+	    pass $test
+	}
+    }
+}
+
+# Make the dcache line size bigger than the pagesize.
+set pagesize [get_integer_valueof "pg_size" -1]
+set linesize [expr $pagesize * 2]
+
+gdb_test_no_output "set dcache line-size $linesize" \
+    "set dcache line size to twice the pagesize"
+
+gdb_test "info dcache" \
+    "Dcache 4096 lines of $linesize bytes each.\r\nNo data cache available."
+
+# Make sure dcache doesn't automatically skip unmapped regions.
+delete_mem
+
+gdb_test "info mem" \
+    "Using user-defined memory regions.\r\nThere are no memory regions defined\."
+
+# Given the line size is bigger than the page size, we have
+# alternating mapped and unmapped pages, these make dcache fail to
+# fill in the cache line.  GDB used to have a bug where that failure
+# would end up as user-visible error.  The range being disassembled is
+# wholly available, so GDB should succeed.
+gdb_test "disassemble first_mapped_page, +10" "End of assembler dump\."
+gdb_test "disassemble last_mapped_page, +10" "End of assembler dump\."
-- 
1.9.0

RE: [PATCH v2] Make the dcache (code/stack cache) handle line reading errors better

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Wednesday, May 21, 2014 11:45 AM
> To: Metzger, Markus T


> Below's v2.  Let me know how it looks.

Looks good to me.

Thanks,
Markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052