diff mbox

Record ARM THUMB2 PLD/PLI cache instructions

Message ID 20180928230437.4329-1-tpiepho@impinj.com
State New, archived
Headers

Commit Message

Trent Piepho Sept. 28, 2018, 11:04 p.m. UTC 
  These weren't decoded correctly and trigger an unknown instruction error
when recording.  The ARM format was handled, but not the 32-bit THUMB2
format.

Since they are only hints that may affect cache state, there is nothing
to record.

gdb/ChangeLog
2018-09-28  Trent Piepho  <tpiepho@impinj.com>

        PR gdb/23725
        * gdb/arm-tdep.c (thumb2_record_ld_mem_hints): Decode thumb2 PLD/PLI
---
 gdb/arm-tdep.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Simon Marchi Sept. 30, 2018, 2:21 p.m. UTC | #1 
On 2018-09-28 7:04 p.m., Trent Piepho wrote:
> These weren't decoded correctly and trigger an unknown instruction error
> when recording.  The ARM format was handled, but not the 32-bit THUMB2
> format.
> 
> Since they are only hints that may affect cache state, there is nothing
> to record.
> 
> gdb/ChangeLog
> 2018-09-28  Trent Piepho  <tpiepho@impinj.com>
> 
>         PR gdb/23725
>         * gdb/arm-tdep.c (thumb2_record_ld_mem_hints): Decode thumb2 PLD/PLI
> ---
>  gdb/arm-tdep.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index c3280ee211..90936ada8e 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -12683,6 +12683,14 @@ thumb2_record_ld_mem_hints (insn_decode_record *thumb2_insn_r)
>                  record_buf);
>        return ARM_RECORD_SUCCESS;
>      }
> +  else
> +    {
> +      if (bits (thumb2_insn_r->arm_insn, 20, 22) == 0x1)
> +	{
> +	  /* Handle PLD, PLI affect only caches, so nothing to record */
> +	  return ARM_RECORD_SUCCESS;
> +	}
> +    }
>  
>    return ARM_RECORD_FAILURE;
>  }
> 

Hi Trent,

Thanks for the patch.  After staring at the ARM architecture reference manual enough, I
think this is fine.

In the manual, however, in table "Table A5-20 Load byte, memory hints", some encodings with
Rt == 0b1111 decode to "UNPREDICTABLE".  Should the record fail for those?  I think currently
with your patch we will accept them.  I am thinking it would be good to fail, because since
we can't know the side effects of such instruction, we risk showing some false information if
we just assume nothing has changed.

If you are motivated, it would be nice to add a test for this instruction in arm_record_test,
but I won't require it, since the current state is that this test isn't meant to test all
possible instruction, and I don't want to impose that burden on you.

Simon
Trent Piepho Oct. 1, 2018, 10:05 p.m. UTC | #2 
On Sun, 2018-09-30 at 10:21 -0400, Simon Marchi wrote:
> On 2018-09-28 7:04 p.m., Trent Piepho wrote:

> > These weren't decoded correctly and trigger an unknown instruction error

> > when recording.  The ARM format was handled, but not the 32-bit THUMB2

> > format.

> > 

> > Since they are only hints that may affect cache state, there is nothing

> > to record.

> > 

> > gdb/ChangeLog

> > 2018-09-28  Trent Piepho  <tpiepho@impinj.com>

> > 

> >         PR gdb/23725

> >         * gdb/arm-tdep.c (thumb2_record_ld_mem_hints): Decode thumb2 PLD/PLI

> > ---

> >  gdb/arm-tdep.c | 8 ++++++++

> >  1 file changed, 8 insertions(+)

> > 

> > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c

> > index c3280ee211..90936ada8e 100644

> > --- a/gdb/arm-tdep.c

> > +++ b/gdb/arm-tdep.c

> > @@ -12683,6 +12683,14 @@ thumb2_record_ld_mem_hints (insn_decode_record *thumb2_insn_r)

> >                  record_buf);

> >        return ARM_RECORD_SUCCESS;

> >      }

> > +  else

> > +    {

> > +      if (bits (thumb2_insn_r->arm_insn, 20, 22) == 0x1)

> > +	{

> > +	  /* Handle PLD, PLI affect only caches, so nothing to record */

> > +	  return ARM_RECORD_SUCCESS;

> > +	}

> > +    }

> >  

> >    return ARM_RECORD_FAILURE;

> >  }

> > 

> 

> Hi Trent,

> 

> Thanks for the patch.  After staring at the ARM architecture reference manual enough, I

> think this is fine.

> 

> In the manual, however, in table "Table A5-20 Load byte, memory hints", some encodings with

> Rt == 0b1111 decode to "UNPREDICTABLE".  Should the record fail for those?  I think currently

> with your patch we will accept them.  I am thinking it would be good to fail, because since

> we can't know the side effects of such instruction, we risk showing some false information if

> we just assume nothing has changed.


I'm not sure what document this is from, but in https://static.docs.arm
.com/ddi0406/c/DDI0406C_C_arm_architecture_reference_manual.pdf

Table A6-20 is titled as above.

Rather than this, I used the thumb2 supplement I found here: http://her
mes.wings.cs.wisc.edu/files/Thumb-2SupplementReferenceManual.pdf

Section 3.3.3 had the most useful table and exhaustive list of possible
encodings for this type of thumb2 instruction.

I see now that not every possible addressing mode is supported for
PLD/PLI, and there are ways to encode a reserved addressing mode for
all instructions of this type.

I've prepared a follow on patch that should provide an exhaustive check
for PLD and PLI instructions.  It also enhances the check for other
instructions of this general format, but I've not verified that the
code is exhaustive there.  It is at least better than it was.

> If you are motivated, it would be nice to add a test for this instruction in arm_record_test,

> but I won't require it, since the current state is that this test isn't meant to test all

> possible instruction, and I don't want to impose that burden on you.


I might that be THAT motivated, since I've never even used that test
feature.
Simon Marchi Oct. 2, 2018, 5:19 p.m. UTC | #3 
On 2018-10-01 18:05, Trent Piepho wrote:
>> In the manual, however, in table "Table A5-20 Load byte, memory 
>> hints", some encodings with
>> Rt == 0b1111 decode to "UNPREDICTABLE".  Should the record fail for 
>> those?  I think currently
>> with your patch we will accept them.  I am thinking it would be good 
>> to fail, because since
>> we can't know the side effects of such instruction, we risk showing 
>> some false information if
>> we just assume nothing has changed.
> 
> I'm not sure what document this is from, but in https://static.docs.arm
> .com/ddi0406/c/DDI0406C_C_arm_architecture_reference_manual.pdf
> 
> Table A6-20 is titled as above.
> 
> Rather than this, I used the thumb2 supplement I found here: http://her
> mes.wings.cs.wisc.edu/files/Thumb-2SupplementReferenceManual.pdf
> 
> Section 3.3.3 had the most useful table and exhaustive list of possible
> encodings for this type of thumb2 instruction.

Not sure which section 3.3.3 you are referring to.  But I must have been 
using an outdated version of the manual, in the version you linked there 
is indeed no unpredictable encodings.

> I see now that not every possible addressing mode is supported for
> PLD/PLI, and there are ways to encode a reserved addressing mode for
> all instructions of this type.
> 
> I've prepared a follow on patch that should provide an exhaustive check
> for PLD and PLI instructions.  It also enhances the check for other
> instructions of this general format, but I've not verified that the
> code is exhaustive there.  It is at least better than it was.
> 
>> If you are motivated, it would be nice to add a test for this 
>> instruction in arm_record_test,
>> but I won't require it, since the current state is that this test 
>> isn't meant to test all
>> possible instruction, and I don't want to impose that burden on you.
> 
> I might that be THAT motivated, since I've never even used that test
> feature.

Err I'm not sure if you you meant that you are that motivated, or you 
are _not_ that motivated.  In case you are, I'll be happy to provide any 
help you would need :).

Simon
Trent Piepho Oct. 3, 2018, 12:34 a.m. UTC | #4 
On Tue, 2018-10-02 at 13:19 -0400, Simon Marchi wrote:
> On 2018-10-01 18:05, Trent Piepho wrote:

> > > In the manual, however, in table "Table A5-20 Load byte, memory 

> > > hints", some encodings with

> > > Rt == 0b1111 decode to "UNPREDICTABLE".  Should the record fail for 

> > > those?  I think currently

> > > with your patch we will accept them.  I am thinking it would be good 

> > > to fail, because since

> > > we can't know the side effects of such instruction, we risk showing 

> > > some false information if

> > > we just assume nothing has changed.

> > 

> > 

> > Rather than this, I used the thumb2 supplement I found here: http://her

> > mes.wings.cs.wisc.edu/files/Thumb-2SupplementReferenceManual.pdf

> > 

> > Section 3.3.3 had the most useful table and exhaustive list of possible

> > encodings for this type of thumb2 instruction.

> 

> Not sure which section 3.3.3 you are referring to.  But I must have been 

> using an outdated version of the manual, in the version you linked there 

> is indeed no unpredictable encodings.


The Thumb-2SupplementReferenceManual.pdf file (link above, 1st google
hit for file name).  It's not a full manual, just a thumb2 description.
 I found the organization of the thumb2 decoding tables more useful in
that one than in the other manual, the full manual on ARM's site.

> > > If you are motivated, it would be nice to add a test for this 

> > > instruction in arm_record_test,

> > > but I won't require it, since the current state is that this test 

> > > isn't meant to test all

> > > possible instruction, and I don't want to impose that burden on you.

> > 

> > I might that be THAT motivated, since I've never even used that test

> > feature.

> 

> Err I'm not sure if you you meant that you are that motivated, or you 

> are _not_ that motivated.  In case you are, I'll be happy to provide any 

> help you would need :).


Sorry, not that motivated.  I wanted to make reverse debugging work on
ARM, and it works for me now.  Don't really want to dive into adding
more to the self tests, which I've never used.  There's really no call
to do that on company time anyway, while for reverse debugging there
was.
Pedro Alves Oct. 3, 2018, 5:19 p.m. UTC | #5 
On 10/03/2018 01:34 AM, Trent Piepho wrote:

> Sorry, not that motivated.  I wanted to make reverse debugging work on
> ARM, and it works for me now.  Don't really want to dive into adding
> more to the self tests, which I've never used.  There's really no call
> to do that on company time anyway, while for reverse debugging there
> was.

Those tests in question _are_ about reverse debugging, and adding
tests can be considered (and often required) part of a patch.

arm_record_test doesn't cover many instructions today because when
record support was added, we didn't have the self testing
infrastructure in place yet.  See:

 https://sourceware.org/ml/gdb-patches/2017-03/msg00031.html

Can't tell whether adding self tests for these instructions would
be really useful, but if you ever want to try it, it's quite easy
to trigger those tests -- type "maint selftest arm-record" in gdb.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index c3280ee211..90936ada8e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -12683,6 +12683,14 @@  thumb2_record_ld_mem_hints (insn_decode_record *thumb2_insn_r)
                 record_buf);
       return ARM_RECORD_SUCCESS;
     }
+  else
+    {
+      if (bits (thumb2_insn_r->arm_insn, 20, 22) == 0x1)
+	{
+	  /* Handle PLD, PLI affect only caches, so nothing to record */
+	  return ARM_RECORD_SUCCESS;
+	}
+    }
 
   return ARM_RECORD_FAILURE;
 }