Fwd: Re: [patch] Add support for ARMv7M devices.

Fwd: Re: [patch] Add support for ARMv7M devices.

[Jonathan Larmour]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

Over a year ago, after discussion and some helpful pointers from Daniel, I
submitted this patch:

http://sourceware.org/ml/gdb-patches/2010-11/msg00142.html

Unfortunately it fell by the wayside.

This issue is coming to a head for the eCos project as we are updating our
ARM toolchain (we avoid the bleeding edge). However that means we really
need GDB to preserve backward compatibility, which it presently fails to do.

We don't want to break backward compatibility with installed GDB stubs,
and the XML solution is complicated for users, opaque, not really suitable
for Eclipse (although there may be proprietary plugins around which try to
solve this, that isn't relevant for public GDB). Most importantly the
attached patch should flexibly work everywhere, and not break anyone's
compatibility. And furthermore, it allows us a clean migration path to the
changed 'g' packet length.

So I'm attaching an updated version of the patch for current GDB trunk. If
it helps, I do have an FSF assignment and, from prehistory, commit
permissions.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine


[-- Attachment #2: mprofile-backcompat.20120309.patch --]
[-- Type: text/plain, Size: 6653 bytes --]

Index: arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.358
diff -u -5 -p -r1.358 arm-tdep.c
--- arm-tdep.c	2 Feb 2012 20:19:12 -0000	1.358
+++ arm-tdep.c	9 Mar 2012 04:19:39 -0000
@@ -41,10 +41,11 @@
 #include "gdbtypes.h"
 #include "prologue-value.h"
 #include "target-descriptions.h"
 #include "user-regs.h"
 #include "observer.h"
+#include "remote.h"
 
 #include "arm-tdep.h"
 #include "gdb/sim-arm.h"
 
 #include "elf-bfd.h"
@@ -53,10 +54,11 @@
 
 #include "gdb_assert.h"
 #include "vec.h"
 
 #include "features/arm-with-m.c"
+#include "features/arm-with-m-fpa.c"
 #include "features/arm-with-iwmmxt.c"
 #include "features/arm-with-vfpv2.c"
 #include "features/arm-with-vfpv3.c"
 #include "features/arm-with-neon.c"
 
@@ -9796,11 +9798,11 @@ arm_gdbarch_init (struct gdbarch_info in
 		 numerically greater than TAG_CPU_ARCH_V7).  */
 	      if (!tdesc_has_registers (tdesc)
 		  && (attr_arch == TAG_CPU_ARCH_V6_M
 		      || attr_arch == TAG_CPU_ARCH_V6S_M
 		      || attr_profile == 'M'))
-		tdesc = tdesc_arm_with_m;
+		is_m = 1;
 #endif
 	    }
 
 	  if (fp_model == ARM_FLOAT_AUTO)
 	    {
@@ -9858,10 +9860,12 @@ arm_gdbarch_init (struct gdbarch_info in
 	  if (feature == NULL)
 	    return NULL;
 	  else
 	    is_m = 1;
 	}
+      else
+        is_m = 0;
 
       tdesc_data = tdesc_data_alloc ();
 
       valid_p = 1;
       for (i = 0; i < ARM_SP_REGNUM; i++)
@@ -10241,10 +10245,33 @@ arm_gdbarch_init (struct gdbarch_info in
      and does no harm, since nothing ever lists user registers.  */
   for (i = 0; i < ARRAY_SIZE (arm_register_aliases); i++)
     user_reg_add (gdbarch, arm_register_aliases[i].name,
 		  value_of_arm_user_reg, &arm_register_aliases[i].regnum);
 
+  if (is_m)
+    {
+      /* For backward-compatibility we allow two 'g' packet lengths with
+         the remote protocol depending on whether FPA registers are
+         supplied. M-profile targets do not have FPA registers, but some
+         stubs already exist in the wild which use a 'g' packet which
+         supplies them albeit with dummy values. The packet format which
+         includes FPA registers should be considered deprecated for
+         M-profile targets.
+
+         The register sizes are fixed for these target descriptions in the
+         XML files, so we can hard-code them: r0-12,sp,lr,pc,xpsr for
+         tdesc_arm_with_m; and additionally f0-f7 and fps for
+         tdesc_arm_with_m_fpa.  */
+      
+      register_remote_g_packet_guess (gdbarch,
+                                      17 * INT_REGISTER_SIZE,
+                                      tdesc_arm_with_m);
+      register_remote_g_packet_guess (gdbarch,
+                                      18 * INT_REGISTER_SIZE +
+                                      8 * FP_REGISTER_SIZE,
+                                      tdesc_arm_with_m_fpa);
+    }
   return gdbarch;
 }
 
 static void
 arm_dump_tdep (struct gdbarch *gdbarch, struct ui_file *file)
@@ -10289,10 +10316,11 @@ _initialize_arm_tdep (void)
 				  bfd_target_elf_flavour,
 				  arm_elf_osabi_sniffer);
 
   /* Initialize the standard target descriptions.  */
   initialize_tdesc_arm_with_m ();
+  initialize_tdesc_arm_with_m_fpa ();
   initialize_tdesc_arm_with_iwmmxt ();
   initialize_tdesc_arm_with_vfpv2 ();
   initialize_tdesc_arm_with_vfpv3 ();
   initialize_tdesc_arm_with_neon ();
 
Index: features/arm-with-m-fpa.c
===================================================================
RCS file: features/arm-with-m-fpa.c
diff -N features/arm-with-m-fpa.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ features/arm-with-m-fpa.c	9 Mar 2012 04:19:39 -0000
@@ -0,0 +1,46 @@
+/* THIS FILE IS GENERATED.  Original: arm-with-m-fpa.xml */
+
+#include "defs.h"
+#include "osabi.h"
+#include "target-descriptions.h"
+
+struct target_desc *tdesc_arm_with_m_fpa;
+static void
+initialize_tdesc_arm_with_m_fpa (void)
+{
+  struct target_desc *result = allocate_target_description ();
+  struct tdesc_feature *feature;
+  struct tdesc_type *field_type, *type;
+
+  feature = tdesc_create_feature (result, "org.gnu.gdb.arm.m-profile");
+  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "sp", 13, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "lr", 14, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "pc", 15, 1, NULL, 32, "code_ptr");
+  tdesc_create_reg (feature, "xpsr", 25, 1, NULL, 32, "int");
+
+  feature = tdesc_create_feature (result, "org.gnu.gdb.arm.fpa");
+  tdesc_create_reg (feature, "f0", 16, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "f1", 17, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "f2", 18, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "f3", 19, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "f4", 20, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "f5", 21, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "f6", 22, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "f7", 23, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "fps", 24, 1, NULL, 32, "int");
+
+  tdesc_arm_with_m_fpa = result;
+}
Index: features/arm-with-m-fpa.xml
===================================================================
RCS file: features/arm-with-m-fpa.xml
diff -N features/arm-with-m-fpa.xml
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ features/arm-with-m-fpa.xml	9 Mar 2012 04:19:39 -0000
@@ -0,0 +1,12 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2010 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE target SYSTEM "gdb-target.dtd">
+<target>
+  <xi:include href="arm-m-profile.xml"/>
+  <xi:include href="arm-fpa.xml"/>
+</target>

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Pedro Alves]

On 03/09/2012 04:24 AM, Jonathan Larmour wrote:

> Over a year ago, after discussion and some helpful pointers from Daniel, I
> submitted this patch:
> 
> http://sourceware.org/ml/gdb-patches/2010-11/msg00142.html
> 
> Unfortunately it fell by the wayside.
> 
> This issue is coming to a head for the eCos project as we are updating our
> ARM toolchain (we avoid the bleeding edge). However that means we really
> need GDB to preserve backward compatibility, which it presently fails to do.
> 
> We don't want to break backward compatibility with installed GDB stubs,

> and the XML solution is complicated for users, opaque, not really suitable

> for Eclipse (although there may be proprietary plugins around which try to
> solve this, that isn't relevant for public GDB). Most importantly the
> attached patch should flexibly work everywhere, and not break anyone's
> compatibility. And furthermore, it allows us a clean migration path to the
> changed 'g' packet length.
> 
> So I'm attaching an updated version of the patch for current GDB trunk. If
> it helps, I do have an FSF assignment and, from prehistory, commit
> permissions.


I support this.  I wrote essentially the same without being aware of
your patch: <http://sourceware.org/ml/gdb-patches/2011-04/msg00372.html>.

Wish I had seen yours before that.

If there are no other comments in a week or so, I say put this in.

On 03/09/2012 04:24 AM, Jonathan Larmour wrote:
>  	}
> +      else
> +        is_m = 0;
>

I think this is unnecessary though.  The variable is initialized to 0.

-- 
Pedro Alves

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Jonathan Larmour]

On 09/03/12 11:44, Pedro Alves wrote:
> On 03/09/2012 04:24 AM, Jonathan Larmour wrote:
> I support this.  I wrote essentially the same without being aware of
> your patch: <http://sourceware.org/ml/gdb-patches/2011-04/msg00372.html>.
> 
> Wish I had seen yours before that.
> 
> If there are no other comments in a week or so, I say put this in.

I have noticed one slight practical difference with your patch... Mine used:
+  <xi:include href="arm-fpa.xml"/>

whereas yours enumerates all the FPA registers, but with the name set to
"", which is better - I hadn't been aware of that property of not showing
a reg if the name is empty.

> On 03/09/2012 04:24 AM, Jonathan Larmour wrote:
>>  	}
>> +      else
>> +        is_m = 0;
>>
> 
> I think this is unnecessary though.  The variable is initialized to 0.

True. I thought I needed to reset it if it got set further up, but now I
see that couldn't happen ( because !tdesc_has_registers in that case)

So perhaps should we just go with your version of the patch after all?
It's effectively identical other than the above FPA name improvement.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
      **  Visit us at the ESC Expo at Design West in San Jose  **
      **  27-29 March, McEnery Convention Center - Stand #846  **
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Pedro Alves]

On 03/09/2012 03:52 PM, Jonathan Larmour wrote:

> On 09/03/12 11:44, Pedro Alves wrote:
>> On 03/09/2012 04:24 AM, Jonathan Larmour wrote:
>> I support this.  I wrote essentially the same without being aware of
>> your patch: <http://sourceware.org/ml/gdb-patches/2011-04/msg00372.html>.
>>
>> Wish I had seen yours before that.
>>
>> If there are no other comments in a week or so, I say put this in.
> 
> I have noticed one slight practical difference with your patch... Mine used:
> +  <xi:include href="arm-fpa.xml"/>
> 
> whereas yours enumerates all the FPA registers, but with the name set to
> "", which is better - I hadn't been aware of that property of not showing
> a reg if the name is empty.


Ah, yes.

> 
>> On 03/09/2012 04:24 AM, Jonathan Larmour wrote:
>>>  	}
>>> +      else
>>> +        is_m = 0;
>>>
>>
>> I think this is unnecessary though.  The variable is initialized to 0.
> 
> True. I thought I needed to reset it if it got set further up, but now I
> see that couldn't happen ( because !tdesc_has_registers in that case)
> 
> So perhaps should we just go with your version of the patch after all?
> It's effectively identical other than the above FPA name improvement.


I suppose so.  Your patch predates mine, so it'll be fair to put your name
in the ChangeLog, like:

2012-03-09  Jonathan Larmour  <jifl@eCosCentric.com>
            Pedro Alves  <pedro@codesourcery.com>

Can you do that and repost the final patch?  Feel free to adjust comments and
such if you prefer.  Then if there are no further comments, we can check it in.

-- 
Pedro Alves

RE: Fwd: Re: [patch] Add support for ARMv7M devices.

[Terry Guo]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Friday, March 09, 2012 7:44 PM
> To: Jonathan Larmour
> Cc: gdb-patches@sourceware.org
> Subject: Re: Fwd: Re: [patch] Add support for ARMv7M devices.
> 
> On 03/09/2012 04:24 AM, Jonathan Larmour wrote:
> 
> > Over a year ago, after discussion and some helpful pointers from
> Daniel, I
> > submitted this patch:
> >
> > http://sourceware.org/ml/gdb-patches/2010-11/msg00142.html
> >
> > Unfortunately it fell by the wayside.
> >
> > This issue is coming to a head for the eCos project as we are
> updating our
> > ARM toolchain (we avoid the bleeding edge). However that means we
> really
> > need GDB to preserve backward compatibility, which it presently fails
> to do.
> >
> > We don't want to break backward compatibility with installed GDB
> stubs,
> 
> > and the XML solution is complicated for users, opaque, not really
> suitable
> 
> > for Eclipse (although there may be proprietary plugins around which
> try to
> > solve this, that isn't relevant for public GDB). Most importantly the
> > attached patch should flexibly work everywhere, and not break
> anyone's
> > compatibility. And furthermore, it allows us a clean migration path
> to the
> > changed 'g' packet length.
> >
> > So I'm attaching an updated version of the patch for current GDB
> trunk. If
> > it helps, I do have an FSF assignment and, from prehistory, commit
> > permissions.
> 
> 
> I support this.  I wrote essentially the same without being aware of
> your patch: <http://sourceware.org/ml/gdb-patches/2011-
> 04/msg00372.html>.
> 
> Wish I had seen yours before that.
> 
> If there are no other comments in a week or so, I say put this in.
> 
> On 03/09/2012 04:24 AM, Jonathan Larmour wrote:
> >  	}
> > +      else
> > +        is_m = 0;
> >
> 
> I think this is unnecessary though.  The variable is initialized to 0.
> 
> --
> Pedro Alves

Hi,

Sorry for bringing it back.

Can somebody please tell me how the "guess" mechanism works? How can the
buf_len in function process_g_packet become correct with "guess"?

Can we just make the arm-with-m.xml always include unnamed FPA registers? So
that it can cope with the case with or without FPA registers.

Thanks.
Terry

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Jonathan Larmour]

On 16/04/12 08:55, Terry Guo wrote:
> 
> Can somebody please tell me how the "guess" mechanism works? How can the
> buf_len in function process_g_packet become correct with "guess"?

Have a look at remote_read_description(), which is called early on when
connecting to the target. Once the target description (tdesc) is set from
there, subsequent calls to process_g_packet() will have an appropriate
value for sizeof_g_packet.

> Can we just make the arm-with-m.xml always include unnamed FPA registers? So
> that it can cope with the case with or without FPA registers.

I think that it might have been possible to arrange this if the FPA
registers happened to be sent at the very end of the "normal" register set
(courtesy of some handling in process_g_packet() of smaller 'g' packets).
But i don't think that's possible here given the FPA regs come after the
PC but before the XPSR at the end; at least not without adding much more
machinery somewhere, including extending the target description syntax. I
might be wrong though, as I don't know the ins-and-outs of this code as
well as others here.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
  **   Visit us at NEW:UK - the UK Embedded & Electronics Exhibition  **
  **   ARM Partner Pavilion/Stand #114. 18-19 April. Birmingham NEC   **
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

RE: Fwd: Re: [patch] Add support for ARMv7M devices.

[Terry Guo]

> -----Original Message-----
> From: Jonathan Larmour [mailto:jifl@ecoscentric.com]
> Sent: Monday, April 16, 2012 10:28 PM
> To: Terry Guo
> Cc: gdb-patches@sourceware.org; Joey Ye; Matthew Gretton-Dann
> Subject: Re: Fwd: Re: [patch] Add support for ARMv7M devices.
> 
> On 16/04/12 08:55, Terry Guo wrote:
> >
> > Can somebody please tell me how the "guess" mechanism works? How can
> the
> > buf_len in function process_g_packet become correct with "guess"?
> 
> Have a look at remote_read_description(), which is called early on when
> connecting to the target. Once the target description (tdesc) is set
> from
> there, subsequent calls to process_g_packet() will have an appropriate
> value for sizeof_g_packet.
> 

That's exact how the "guess" works! I get it. Thanks.

> > Can we just make the arm-with-m.xml always include unnamed FPA
> registers? So
> > that it can cope with the case with or without FPA registers.
> 
> I think that it might have been possible to arrange this if the FPA
> registers happened to be sent at the very end of the "normal" register
> set
> (courtesy of some handling in process_g_packet() of smaller 'g'
> packets).
> But i don't think that's possible here given the FPA regs come after
> the
> PC but before the XPSR at the end; at least not without adding much
> more
> machinery somewhere, including extending the target description syntax.
> I
> might be wrong though, as I don't know the ins-and-outs of this code as
> well as others here.
> 
> Jifl

I get your point and think you are correct. If we always assume faked FPA
registers, we will mess things up when the stub only returns core register +
xpsr.

BR,
Terry

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Yao Qi]

On 03/09/2012 12:24 PM, Jonathan Larmour wrote:
> -		tdesc = tdesc_arm_with_m;
> +		is_m = 1;

`tdesc' is used later in the function,

  /* Check any target description for validity.  */
  if (tdesc_has_registers (tdesc))

Is it correct to skip updating `tdesc'?

-- 
Yao (齐尧)

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Jonathan Larmour]

On 09/03/12 15:38, Yao Qi wrote:
> On 03/09/2012 12:24 PM, Jonathan Larmour wrote:
>> -		tdesc = tdesc_arm_with_m;
>> +		is_m = 1;
> 
> `tdesc' is used later in the function,
> 
>   /* Check any target description for validity.  */
>   if (tdesc_has_registers (tdesc))
> 
> Is it correct to skip updating `tdesc'?

Yes we do not want that condition to match. We want the tdesc to still be
undetermined by the point of the 'if' test if this is Cortex-M. The code
prior to this change only set tdesc because it thought it knew exactly
what register set to use by that point.

But you have made me think of one improvement: we should probably not call
register_remote_g_packet_guess() if tdesc_has_registers (tdesc) - because
if someone has directly supplied a target description, we should solely
use that, and avoid any guessing. This would be true for both my and
Pedro's patch.

That's trivial to handle though, e.g. for Pedro's patch, just replace the
call with:

  if (!tdesc_has_registers (tdesc))
    arm_register_g_packet_guesses (gdbarch);

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Pedro Alves]

On 03/09/2012 04:13 PM, Jonathan Larmour wrote:

> On 09/03/12 15:38, Yao Qi wrote:
>> On 03/09/2012 12:24 PM, Jonathan Larmour wrote:
>>> -		tdesc = tdesc_arm_with_m;
>>> +		is_m = 1;
>>
>> `tdesc' is used later in the function,
>>
>>   /* Check any target description for validity.  */
>>   if (tdesc_has_registers (tdesc))
>>
>> Is it correct to skip updating `tdesc'?
> 
> Yes we do not want that condition to match. We want the tdesc to still be
> undetermined by the point of the 'if' test if this is Cortex-M. The code
> prior to this change only set tdesc because it thought it knew exactly
> what register set to use by that point.
> 
> But you have made me think of one improvement: we should probably not call
> register_remote_g_packet_guess() if tdesc_has_registers (tdesc) - because
> if someone has directly supplied a target description, we should solely
> use that, and avoid any guessing. 


I think that's always true, irrespective of a g packet guess being
installed.  See target_find_description: it's always "file > target xml > g-guesses",

> This would be true for both my and Pedro's patch.
> 
> That's trivial to handle though, e.g. for Pedro's patch, just replace the
> call with:
> 
>   if (!tdesc_has_registers (tdesc))
>     arm_register_g_packet_guesses (gdbarch);


so if we unconditionally register the guesses, then even "set tdesc foo;
file foo; unset tdesc filename; tar rem ..." works correctly.

-- 
Pedro Alves

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Jonathan Larmour]

[-- Attachment #1: Type: text/plain, Size: 2411 bytes --]

On 09/03/12 16:28, Pedro Alves wrote:
> On 03/09/2012 04:13 PM, Jonathan Larmour wrote:
>>
>> But you have made me think of one improvement: we should probably not call
>> register_remote_g_packet_guess() if tdesc_has_registers (tdesc) - because
>> if someone has directly supplied a target description, we should solely
>> use that, and avoid any guessing. 
> 
> 
> I think that's always true, irrespective of a g packet guess being
> installed.  See target_find_description: it's always "file > target xml > g-guesses",

Can you just clarify to me how, for example, a program using VFP registers
(such as for Cortex-M4) would use the correct 'g' packet size? The
registers correspond to the tdesc, and not to either of the guessed sizes.
I guess if I could understand that example, I'll be happy. You can do this
off list if you like, to save others from boredom.

Nevertheless, on the entirely plausible assumption I'm wrong and the patch
was roughly correct before, I'm attaching an updated version against
current CVS - I merged in my comments and use of {INT,FP}_REGISTER_SIZE to
make the code a bit more self-documenting.

2012-03-09  Jonathan Larmour  <jifl@eCosCentric.com>
            Pedro Alves  <pedro@codesourcery.com>

	* arm-tdep.c: Include "remote.h" and
	"features/arm-with-m-fpa-layout.c".
	(arm_register_g_packet_guesses): New function.
	(arm_gdbarch_init): Don't force a target description with
	registers when the executable is detected as M-profile.  Instead
	set gdbarch->tdep->is_m.  Register `g' packet guesses.
	(_initialize_arm_tdep): Initialize the new target description.
	* features/arm-with-m-fpa-layout.xml: New description.
	* features/arm-with-m-fpa-layout.c: New, generated.

arm-tdep.c                         |   42 +++++++++++++++++++++++++++++++++-
features/arm-with-m-fpa-layout.c   |   44 ++++++++++++++++++++++++++++++++++++
features/arm-with-m-fpa-layout.xml |   45
+++++++++++++++++++++++++++++++++++++
3 files changed, 130 insertions(+), 1 deletion(-)

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
      **  Visit us at the ESC Expo at Design West in San Jose  **
      **  27-29 March, McEnery Convention Center - Stand #846  **
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

[-- Attachment #2: mprofile-backcompat.v2.patch --]
[-- Type: text/plain, Size: 8230 bytes --]

Index: arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.358
diff -u -5 -p -r1.358 arm-tdep.c
--- arm-tdep.c	2 Feb 2012 20:19:12 -0000	1.358
+++ arm-tdep.c	9 Mar 2012 17:58:50 -0000
@@ -38,10 +38,11 @@
 #include "trad-frame.h"
 #include "objfiles.h"
 #include "dwarf2-frame.h"
 #include "gdbtypes.h"
 #include "prologue-value.h"
+#include "remote.h"
 #include "target-descriptions.h"
 #include "user-regs.h"
 #include "observer.h"
 
 #include "arm-tdep.h"
@@ -53,10 +54,11 @@
 
 #include "gdb_assert.h"
 #include "vec.h"
 
 #include "features/arm-with-m.c"
+#include "features/arm-with-m-fpa-layout.c"
 #include "features/arm-with-iwmmxt.c"
 #include "features/arm-with-vfpv2.c"
 #include "features/arm-with-vfpv3.c"
 #include "features/arm-with-neon.c"
 
@@ -9663,10 +9665,45 @@ arm_register_reggroup_p (struct gdbarch 
   else
     return default_register_reggroup_p (gdbarch, regnum, group);
 }
 
 \f
+/* For backward-compatibility we allow two 'g' packet lengths with
+   the remote protocol depending on whether FPA registers are
+   supplied. M-profile targets do not have FPA registers, but some
+   stubs already exist in the wild which use a 'g' packet which
+   supplies them albeit with dummy values. The packet format which
+   includes FPA registers should be considered deprecated for
+   M-profile targets.  */
+
+static void
+arm_register_g_packet_guesses (struct gdbarch *gdbarch)
+{
+  if (gdbarch_tdep (gdbarch)->is_m)
+    {
+      /* If we know from the executable this is an M-profile target,
+	 cater for remote targets whose register set layout is the
+	 same as the FPA layout.  */
+      register_remote_g_packet_guess (gdbarch,
+				      /* r0-r12,sp,lr,pc; f0-f7; fps,cpsr */
+				      (16 * INT_REGISTER_SIZE)
+				      + (8 * FP_REGISTER_SIZE)
+				      + (2 * INT_REGISTER_SIZE),
+				      tdesc_arm_with_m_fpa_layout);
+
+      /* The regular M-profile layout.  */
+      register_remote_g_packet_guess (gdbarch,
+				      /* r0-r12,sp,lr,pc; xpsr */
+				      (16 * INT_REGISTER_SIZE)
+				      + INT_REGISTER_SIZE,
+				      tdesc_arm_with_m);
+    }
+
+  /* Otherwise we don't have a useful guess.  */
+}
+
+\f
 /* Initialize the current architecture based on INFO.  If possible,
    re-use an architecture from ARCHES, which is a list of
    architectures already created during this debugging session.
 
    Called e.g. at program startup, when reading a core file, and when
@@ -9796,11 +9833,11 @@ arm_gdbarch_init (struct gdbarch_info in
 		 numerically greater than TAG_CPU_ARCH_V7).  */
 	      if (!tdesc_has_registers (tdesc)
 		  && (attr_arch == TAG_CPU_ARCH_V6_M
 		      || attr_arch == TAG_CPU_ARCH_V6S_M
 		      || attr_profile == 'M'))
-		tdesc = tdesc_arm_with_m;
+		is_m = 1;
 #endif
 	    }
 
 	  if (fp_model == ARM_FLOAT_AUTO)
 	    {
@@ -10053,10 +10090,12 @@ arm_gdbarch_init (struct gdbarch_info in
   tdep->have_vfp_registers = have_vfp_registers;
   tdep->have_vfp_pseudos = have_vfp_pseudos;
   tdep->have_neon_pseudos = have_neon_pseudos;
   tdep->have_neon = have_neon;
 
+  arm_register_g_packet_guesses (gdbarch);
+
   /* Breakpoints.  */
   switch (info.byte_order_for_code)
     {
     case BFD_ENDIAN_BIG:
       tdep->arm_breakpoint = arm_default_arm_be_breakpoint;
@@ -10289,10 +10328,11 @@ _initialize_arm_tdep (void)
 				  bfd_target_elf_flavour,
 				  arm_elf_osabi_sniffer);
 
   /* Initialize the standard target descriptions.  */
   initialize_tdesc_arm_with_m ();
+  initialize_tdesc_arm_with_m_fpa_layout ();
   initialize_tdesc_arm_with_iwmmxt ();
   initialize_tdesc_arm_with_vfpv2 ();
   initialize_tdesc_arm_with_vfpv3 ();
   initialize_tdesc_arm_with_neon ();
 
Index: features/arm-with-m-fpa-layout.c
===================================================================
RCS file: features/arm-with-m-fpa-layout.c
diff -N features/arm-with-m-fpa-layout.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ features/arm-with-m-fpa-layout.c	9 Mar 2012 17:58:50 -0000
@@ -0,0 +1,44 @@
+/* THIS FILE IS GENERATED.  Original: arm-with-m-fpa-layout.xml */
+
+#include "defs.h"
+#include "osabi.h"
+#include "target-descriptions.h"
+
+struct target_desc *tdesc_arm_with_m_fpa_layout;
+static void
+initialize_tdesc_arm_with_m_fpa_layout (void)
+{
+  struct target_desc *result = allocate_target_description ();
+  struct tdesc_feature *feature;
+  struct tdesc_type *field_type, *type;
+
+  feature = tdesc_create_feature (result, "org.gnu.gdb.arm.m-profile");
+  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "sp", 13, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "lr", 14, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "pc", 15, 1, NULL, 32, "code_ptr");
+  tdesc_create_reg (feature, "", 16, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "", 17, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "", 18, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "", 19, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "", 20, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "", 21, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "", 22, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "", 23, 1, NULL, 96, "arm_fpa_ext");
+  tdesc_create_reg (feature, "", 24, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "xpsr", 25, 1, NULL, 32, "int");
+
+  tdesc_arm_with_m_fpa_layout = result;
+}
Index: features/arm-with-m-fpa-layout.xml
===================================================================
RCS file: features/arm-with-m-fpa-layout.xml
diff -N features/arm-with-m-fpa-layout.xml
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ features/arm-with-m-fpa-layout.xml	9 Mar 2012 17:58:50 -0000
@@ -0,0 +1,45 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!-- A target description for an M-profile device, for stubs that
+     transfer registers using the historical fpa layout.  -->
+
+<!DOCTYPE target SYSTEM "gdb-target.dtd">
+<target>
+<feature name="org.gnu.gdb.arm.m-profile">
+  <reg name="r0" bitsize="32"/>
+  <reg name="r1" bitsize="32"/>
+  <reg name="r2" bitsize="32"/>
+  <reg name="r3" bitsize="32"/>
+  <reg name="r4" bitsize="32"/>
+  <reg name="r5" bitsize="32"/>
+  <reg name="r6" bitsize="32"/>
+  <reg name="r7" bitsize="32"/>
+  <reg name="r8" bitsize="32"/>
+  <reg name="r9" bitsize="32"/>
+  <reg name="r10" bitsize="32"/>
+  <reg name="r11" bitsize="32"/>
+  <reg name="r12" bitsize="32"/>
+  <reg name="sp" bitsize="32" type="data_ptr"/>
+  <reg name="lr" bitsize="32"/>
+  <reg name="pc" bitsize="32" type="code_ptr"/>
+
+  <!-- Slack for unused FPA registers (f0-f7 + fps).
+       See arm-fpa.xml.  -->
+  <reg name="" bitsize="96" type="arm_fpa_ext" regnum="16"/>
+  <reg name="" bitsize="96" type="arm_fpa_ext"/>
+  <reg name="" bitsize="96" type="arm_fpa_ext"/>
+  <reg name="" bitsize="96" type="arm_fpa_ext"/>
+  <reg name="" bitsize="96" type="arm_fpa_ext"/>
+  <reg name="" bitsize="96" type="arm_fpa_ext"/>
+  <reg name="" bitsize="96" type="arm_fpa_ext"/>
+  <reg name="" bitsize="96" type="arm_fpa_ext"/>
+  <reg name="" bitsize="32"/>
+
+  <reg name="xpsr" bitsize="32" regnum="25"/>
+</feature>
+</target>

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Pedro Alves]

On 03/11/2012 03:36 AM, Jonathan Larmour wrote:

> On 09/03/12 16:28, Pedro Alves wrote:
>> On 03/09/2012 04:13 PM, Jonathan Larmour wrote:
>>>
>>> But you have made me think of one improvement: we should probably not call
>>> register_remote_g_packet_guess() if tdesc_has_registers (tdesc) - because
>>> if someone has directly supplied a target description, we should solely
>>> use that, and avoid any guessing. 
>>
>>
>> I think that's always true, irrespective of a g packet guess being
>> installed.  See target_find_description: it's always "file > target xml > g-guesses",
> 
> Can you just clarify to me how, for example, a program using VFP registers
> (such as for Cortex-M4) would use the correct 'g' packet size? The
> registers correspond to the tdesc, and not to either of the guessed sizes.
> I guess if I could understand that example, I'll be happy. You can do this
> off list if you like, to save others from boredom.


Even without a description, and before connecting to the remote side,
GDB already has a clue of the target's architecture, inferred from the
executable.  GDB updates target_gdbarch based on that, and sets an initial
expected size of the g packet based on the register set it things the
target has (based on what it figured out from the executable).

static void *
init_remote_state (struct gdbarch *gdbarch)
{
...
  /* Record the maximum possible size of the g packet - it may turn out
     to be smaller.  */
  rsa->sizeof_g_packet = map_regcache_remote_table (gdbarch, rsa->regs);

If it turns out to be smaller, GDB will re-adjust (process_g_packet).

Or did you mean, in the case where the target does send over a
target description?  The g packet size guesses are only used when the target
does _not_ send in a target description.  So, GDB, during the initial
remote connection, calls target_find_description:

void
target_find_description (void)
{
  /* If we've already fetched a description from the target, don't do
     it again.  This allows a target to fetch the description early,
     during its to_open or to_create_inferior, if it needs extra
     information about the target to initialize.  */
  if (target_desc_fetched)
    return;

  /* The current architecture should not have any target description
     specified.  It should have been cleared, e.g. when we
     disconnected from the previous target.  */
  gdb_assert (gdbarch_target_desc (target_gdbarch) == NULL);

  /* First try to fetch an XML description from the user-specified
     file.  */
  current_target_desc = NULL;
  if (target_description_filename != NULL
      && *target_description_filename != '\0')
    current_target_desc
      = file_read_description_xml (target_description_filename);

  /* Next try to read the description from the current target using
     target objects.  */
  if (current_target_desc == NULL)
    current_target_desc = target_read_description_xml (&current_target);

  /* If that failed try a target-specific hook.  */
  if (current_target_desc == NULL)
    current_target_desc = target_read_description (&current_target);

Which you can see first checks for a description as set by "set tdesc filename",
and if that doesn't work, tries to fetch a description off the target, and only
if that doesn't work, it'll call target_read_description, which maps to
remote_read_description, which returns a guess based on the size of the g
packet, as registered on the gdbarch.  So if the target replied back a
xml target description, we'll never read the target_read_description call.
There's actually a wrinkle: during the first call to target_find_description,
we haven't yet fetched the remote thread and inferior's status, and haven't yet
added them to our tables.  So that first very initial time,
remote_read_description returns nothing.  Right after fetching the remote
target's status, we then try again:

      /* If we could not find a description using qXfer, and we know
	 how to do it some other way, try again.  This is not
	 supported for non-stop; it could be, but it is tricky if
	 there are no stopped threads when we connect.  */
      if (remote_read_description_p (target)
	  && gdbarch_target_desc (target_gdbarch) == NULL)
	{
	  target_clear_description ();
	  target_find_description ();
	}

But again, if the target or the user have already specified a description,
the if predicate is false.  If we don't have a description yet, we'll end up
in target_find_description -> remote_read_description, which will no return the
best description for the gdbarch based on the g packet size.

Of course, the g-guesses are registered on specific gdbarch's.  So GDB already
needs to have a clue of the target's gdbarch for g-packet guesses to work.
And again, it gets that initial target_gdbarch from the executable.

-- 
Pedro Alves

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Jonathan Larmour]

On 14/03/12 16:06, Pedro Alves wrote:
> On 03/11/2012 03:36 AM, Jonathan Larmour wrote:
>>
>> Can you just clarify to me how, for example, a program using VFP registers
>> (such as for Cortex-M4) would use the correct 'g' packet size? The
>> registers correspond to the tdesc, and not to either of the guessed sizes.
>> I guess if I could understand that example, I'll be happy. You can do this
>> off list if you like, to save others from boredom.
> 
> Even without a description, and before connecting to the remote side,
> GDB already has a clue of the target's architecture, inferred from the
> executable. GDB updates target_gdbarch based on that, and sets an initial
> expected size of the g packet based on the register set it things the
> target has (based on what it figured out from the executable).
> 
> static void *
> init_remote_state (struct gdbarch *gdbarch)
> {
> ...
>   /* Record the maximum possible size of the g packet - it may turn out
>      to be smaller.  */
>   rsa->sizeof_g_packet = map_regcache_remote_table (gdbarch, rsa->regs);
> 
> If it turns out to be smaller, GDB will re-adjust (process_g_packet).

Ah ok, that's what I was missing. And that uses gdbarch_num_regs() which
corresponds to arm-tdep.h's ARM_NUM_REGS which includes all possible
registers, both VFP and FPA. And later the 'g' packet is shrunk.

So, just to be clear, if neither the user nor target supplies a tdesc, the
block in arm_gdbarch_init() starting with:
   if (tdesc_has_registers (tdesc))
will never be run? If that's the case, no need to answer. Or is
arm_gdbarch_init() called a second time after the remote 'g' packet
guessing occurs (which has selected a tdesc)?

> Or did you mean, in the case where the target does send over a
> target description?

No I was thinking more of when GDB only has the executable to go on. But
thanks for that information - it fills other gaps in my knowledge.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
      **  Visit us at the ESC Expo at Design West in San Jose  **
      **  27-29 March, McEnery Convention Center - Stand #846  **
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Pedro Alves]

On 03/15/2012 06:26 AM, Jonathan Larmour wrote:

> On 14/03/12 16:06, Pedro Alves wrote:
>> On 03/11/2012 03:36 AM, Jonathan Larmour wrote:
>>>
>>> Can you just clarify to me how, for example, a program using VFP registers
>>> (such as for Cortex-M4) would use the correct 'g' packet size? The
>>> registers correspond to the tdesc, and not to either of the guessed sizes.
>>> I guess if I could understand that example, I'll be happy. You can do this
>>> off list if you like, to save others from boredom.
>>
>> Even without a description, and before connecting to the remote side,
>> GDB already has a clue of the target's architecture, inferred from the
>> executable. GDB updates target_gdbarch based on that, and sets an initial
>> expected size of the g packet based on the register set it things the
>> target has (based on what it figured out from the executable).
>>
>> static void *
>> init_remote_state (struct gdbarch *gdbarch)
>> {
>> ...
>>   /* Record the maximum possible size of the g packet - it may turn out
>>      to be smaller.  */
>>   rsa->sizeof_g_packet = map_regcache_remote_table (gdbarch, rsa->regs);
>>
>> If it turns out to be smaller, GDB will re-adjust (process_g_packet).
> 
> Ah ok, that's what I was missing. And that uses gdbarch_num_regs() which
> corresponds to arm-tdep.h's ARM_NUM_REGS which includes all possible
> registers, both VFP and FPA. And later the 'g' packet is shrunk.
> 
> So, just to be clear, if neither the user nor target supplies a tdesc, the
> block in arm_gdbarch_init() starting with:
>    if (tdesc_has_registers (tdesc))
> will never be run? If that's the case, no need to answer. Or is
> arm_gdbarch_init() called a second time after the remote 'g' packet
> guessing occurs (which has selected a tdesc)?


It is called a second time.  You can try putting a breakpoint
there and see what happens.

From reading the code, we'll reach:

      /* If we could not find a description using qXfer, and we know
	 how to do it some other way, try again.  This is not
	 supported for non-stop; it could be, but it is tricky if
	 there are no stopped threads when we connect.  */
      if (remote_read_description_p (target)
	  && gdbarch_target_desc (target_gdbarch) == NULL)
	{
	  target_clear_description ();
	  target_find_description ();
	}

and target_find_description will call gdbarch_update_p, which should
end up in gdbarch_find_by_info calling:

  /* Ask the tdep code for an architecture that matches "info".  */
  new_gdbarch = rego->init (info, rego->arches);

Which will end up in arm_gdbarch_init creating a new gdbarch
modelled on the guessed target description (after the checks), since no
gdbarch has this tdesc yet.

-- 
Pedro Alves

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Jonathan Larmour]

On 15/03/12 17:08, Pedro Alves wrote:
> On 03/15/2012 06:26 AM, Jonathan Larmour wrote:
>> So, just to be clear, if neither the user nor target supplies a tdesc, the
>> block in arm_gdbarch_init() starting with:
>>    if (tdesc_has_registers (tdesc))
>> will never be run? If that's the case, no need to answer. Or is
>> arm_gdbarch_init() called a second time after the remote 'g' packet
>> guessing occurs (which has selected a tdesc)?
> 
> 
> It is called a second time.  You can try putting a breakpoint
> there and see what happens.
[snip useful info]

Thanks for that. It'll help me in future (and from the mailing list
archives, hopefully others!).

So back to the topic in hand, is there agreement that the most recent
version of the patch is acceptable? As in the patch contained here:

http://sourceware.org/ml/gdb-patches/2012-03/msg00377.html

Let me know if I should commit it.

Jifl

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Pedro Alves]

On 03/15/2012 06:32 PM, Jonathan Larmour wrote:

> Let me know if I should commit it.


Let's put it in.

Just a small nit:

On 03/11/2012 03:36 AM, Jonathan Larmour wrote:
> +/* For backward-compatibility we allow two 'g' packet lengths with
> +   the remote protocol depending on whether FPA registers are
> +   supplied. M-profile targets do not have FPA registers, but some
> +   stubs already exist in the wild which use a 'g' packet which
> +   supplies them albeit with dummy values. The packet format which
> +   includes FPA registers should be considered deprecated for
> +   M-profile targets.  */

Double-space after all periods, not just the last.

Thanks!

-- 
Pedro Alves

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Jonathan Larmour]

On 15/03/12 18:43, Pedro Alves wrote:
> On 03/15/2012 06:32 PM, Jonathan Larmour wrote:
> 
>> Let me know if I should commit it.
> 
> Let's put it in.
> Just a small nit: [snip]
> Double-space after all periods, not just the last.

Committed with that change. Thanks for all the help. I kept the ChangeLog
entry with @codesourcery.com for you as that's what you requested, hope
that's ok.

Jifl

Re: Fwd: Re: [patch] Add support for ARMv7M devices.

[Pedro Alves]

On 03/15/2012 06:55 PM, Jonathan Larmour wrote:

> Committed with that change. Thanks for all the help. I kept the ChangeLog
> entry with @codesourcery.com for you as that's what you requested, hope
> that's ok.


Yes, that's okay.  Thanks again.

-- 
Pedro Alves