btrace, vdso: add vdso target sections

Message ID 537A7A80.3050801@redhat.com
State Not applicable
Headers

Commit Message

Pedro Alves May 19, 2014, 9:41 p.m. UTC
  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(-)
  

Comments

Markus Metzger May 20, 2014, 6:40 a.m. UTC | #1
> -----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
  
Pedro Alves May 20, 2014, 11:12 a.m. UTC | #2
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,

?
  
Markus Metzger May 20, 2014, 11:14 a.m. UTC | #3
> -----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

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
     {