[PATCH] Modify Aarch64 prologue analyzer to accept 128-bit registers

[PATCH] Modify Aarch64 prologue analyzer to accept 128-bit registers

[pcarroll]

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

GDB has a routine, aarch64_analyze_prologue, that looks through a function's prologue, to see how it affects the stack.
This was changed for Bugzilla #20682 to add support for recognizing the 'stp' instruction with floating-point registers.
That patch worked when 64-bit floating-point registers are used in a function prologue.
However, it is also possible to specify 128-bit floating-point registers with the 'stp' instruction.
My patch extends that function so it works for either 64-bit or 128-bit floating-point registers.
The patch takes care of tracking the appropriate memory locations that would be affected by the use of either size of register.

The assumption is that it is not important to know whether the register being saved is 64-bits or 128-bits in size, as long as the memory is tracked appropriately.
Instead, it is only important to know that a floating-point register (D) was being stored, rather than a normal register (X).
That behavior is unchanged.

2017-11-10  Paul Carroll  <pcarroll@codesourcery.com>

             * aarch64-tdep.c (aarch64_analyze_prologue):  Added support for
             128-bit registers with the 'stp' instruction.

[-- Attachment #2: aarch64_stp_patch --]
[-- Type: application/octet-stream, Size: 2729 bytes --]

diff -rup a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
--- a/gdb/aarch64-tdep.c	2017-11-10 12:15:43.129386073 -0800
+++ b/gdb/aarch64-tdep.c	2017-11-10 12:41:51.275661344 -0800
@@ -356,6 +356,7 @@ aarch64_analyze_prologue (struct gdbarch
 	  unsigned rt2;
 	  unsigned rn = inst.operands[2].addr.base_regno;
 	  int32_t imm = inst.operands[2].addr.offset.imm;
+	  CORE_ADDR reg_size = 8;
 
 	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt
 		      || inst.operands[0].type == AARCH64_OPND_Ft);
@@ -364,29 +365,36 @@ aarch64_analyze_prologue (struct gdbarch
 	  gdb_assert (inst.operands[2].type == AARCH64_OPND_ADDR_SIMM7);
 	  gdb_assert (!inst.operands[2].addr.offset.is_reg);
 
+	  /* Take care of the case where 128-bit registers need to be stored */
+	  if (inst.operands[0].type == AARCH64_OPND_Ft &&
+	      inst.operands[0].qualifier == AARCH64_OPND_QLF_S_Q)
+	    reg_size = 16;
+
 	  /* If recording this store would invalidate the store area
 	     (perhaps because rn is not known) then we should abandon
 	     further prologue analysis.  */
 	  if (stack.store_would_trash (pv_add_constant (regs[rn], imm)))
 	    break;
 
-	  if (stack.store_would_trash (pv_add_constant (regs[rn], imm + 8)))
+	  if (stack.store_would_trash (pv_add_constant (regs[rn], 
+	  						imm + reg_size)))
 	    break;
 
 	  rt1 = inst.operands[0].reg.regno;
 	  rt2 = inst.operands[1].reg.regno;
 	  if (inst.operands[0].type == AARCH64_OPND_Ft)
 	    {
-	      /* Only bottom 64-bit of each V register (D register) need
-		 to be preserved.  */
-	      gdb_assert (inst.operands[0].qualifier == AARCH64_OPND_QLF_S_D);
+	      /* Only bottom 64-bit of each V register (D register) needs
+		 to be tracked.  */
+	      gdb_assert (inst.operands[0].qualifier == AARCH64_OPND_QLF_S_D ||
+	      		  inst.operands[0].qualifier == AARCH64_OPND_QLF_S_Q);
 	      rt1 += AARCH64_X_REGISTER_COUNT;
 	      rt2 += AARCH64_X_REGISTER_COUNT;
 	    }
 
-	  stack.store (pv_add_constant (regs[rn], imm), 8,
+	  stack.store (pv_add_constant (regs[rn], imm), reg_size,
 		       regs[rt1]);
-	  stack.store (pv_add_constant (regs[rn], imm + 8), 8,
+	  stack.store (pv_add_constant (regs[rn], imm + reg_size), reg_size,
 		       regs[rt2]);
 
 	  if (inst.operands[2].addr.writeback)
diff -rup a/gdb/ChangeLog b/gdb/ChangeLog
--- a/gdb/ChangeLog	2017-11-10 12:15:43.081386126 -0800
+++ b/gdb/ChangeLog	2017-11-10 12:36:31.052037951 -0800
@@ -1,3 +1,8 @@
+2017-11-10  Paul Carroll  <pcarroll@codesourcery.com>
+
+	* aarch64-tdep.c (aarch64_analyze_prologue): Added support for
+	128-bit registers with the 'stp' instruction.
+
 2017-11-09  Joel Brobecker  <brobecker@adacore.com>
 
 	* ada-lang.c: Fix some typos in the general command documenting

Re: [PATCH] Modify Aarch64 prologue analyzer to accept 128-bit registers

[Andrew Pinski]

On Mon, Nov 13, 2017 at 9:28 AM, pcarroll@codesourcery.com
<pcarroll@codesourcery.com> wrote:
> GDB has a routine, aarch64_analyze_prologue, that looks through a function's prologue, to see how it affects the stack.
> This was changed for Bugzilla #20682 to add support for recognizing the 'stp' instruction with floating-point registers.
> That patch worked when 64-bit floating-point registers are used in a function prologue.
> However, it is also possible to specify 128-bit floating-point registers with the 'stp' instruction.
> My patch extends that function so it works for either 64-bit or 128-bit floating-point registers.
> The patch takes care of tracking the appropriate memory locations that would be affected by the use of either size of register.
>
> The assumption is that it is not important to know whether the register being saved is 64-bits or 128-bits in size, as long as the memory is tracked appropriately.
> Instead, it is only important to know that a floating-point register (D) was being stored, rather than a normal register (X).
> That behavior is unchanged.

Hmm,  The normal elf aarch64 ABI says only 64bits is saved.  Is there
another ABI which says 128bits of the SIMD register is saved?

Thanks,
Andrew

>
> 2017-11-10  Paul Carroll  <pcarroll@codesourcery.com>
>
>              * aarch64-tdep.c (aarch64_analyze_prologue):  Added support for
>              128-bit registers with the 'stp' instruction.

Re: [PATCH] Modify Aarch64 prologue analyzer to accept 128-bit registers

[Paul Carroll]

(Sending again, due to problems with the mailing list not liking my 
previous post)

On 11/13/2017 12:32 PM, Andrew Pinski wrote:
> Hmm,  The normal elf aarch64 ABI says only 64bits is saved.  Is there
> another ABI which says 128bits of the SIMD register is saved?

Thanks for the comment, Andrew.
In this case, the code in question is an interrupt routine.
As such, it is not following the ABI, except when making calls itself.
When gdb processes the start of the interrupt routine, it finds the 
'stp' with the 128-bit register references and asserts.
That is a problem for debugging embedded applications, and is what this 
patch is trying to avoid.

Re: [PATCH] Modify Aarch64 prologue analyzer to accept 128-bit registers

[Luis Machado]

On 11/14/2017 11:41 AM, Paul Carroll wrote:
> (Sending again, due to problems with the mailing list not liking my 
> previous post)
> 
> On 11/13/2017 12:32 PM, Andrew Pinski wrote:
>> Hmm,  The normal elf aarch64 ABI says only 64bits is saved.  Is there
>> another ABI which says 128bits of the SIMD register is saved?
> 
> Thanks for the comment, Andrew.
> In this case, the code in question is an interrupt routine.
> As such, it is not following the ABI, except when making calls itself.
> When gdb processes the start of the interrupt routine, it finds the 
> 'stp' with the 128-bit register references and asserts.
> That is a problem for debugging embedded applications, and is what this 
> patch is trying to avoid.
> 

I take it this is RTOS-specific?

If so, I can see a couple alternatives. One way would be to patch it in 
the RTOS-specific file in GDB, keeping the fix contained in the right 
context instead of in general code that will likely not benefit from it.

The second alternative is to augment the code with CFI information so 
GDB doesn't have to use the prologue analyzers. There will always be a 
particular combination of instructions GDB won't recognize, especially 
for custom code that doesn't follow the ABI.

The plus side of fixing this on the RTOS code itself is that GDB doesn't 
have to be patched every time an unknown prologue sequence is 
encountered. But attempting to debug binaries stripped of symbols and 
debug information will required GDB to be patched.