AArch64: Allow additional sizes in prologue
Commit Message
When saving registers to the stack at the start of a function, not all state
needs to be saved. For example, only the first 64bits of float registers need
saving. However, a program may choose to store extra state if it wishes,
there is nothing preventing it doing so.
The aarch64_analyze_prologue will error if it detects extra state being
stored. Relex this restriction.
Tested via aarch64-prologue test.
gdb/ChangeLog:
2019-08-05 Alan Hayward <alan.hayward@arm.com>
* aarch64-tdep.c (aarch64_analyze_prologue): Allow any valid
register sizes.
gdb/testsuite/ChangeLog:
2019-08-05 Alan Hayward <alan.hayward@arm.com>
* gdb.arch/aarch64-prologue.c: New test.
* gdb.arch/aarch64-prologue.exp: New file.
---
gdb/aarch64-tdep.c | 28 ++-----
gdb/testsuite/gdb.arch/aarch64-prologue.c | 83 +++++++++++++++++++++
gdb/testsuite/gdb.arch/aarch64-prologue.exp | 36 +++++++++
3 files changed, 127 insertions(+), 20 deletions(-)
create mode 100644 gdb/testsuite/gdb.arch/aarch64-prologue.c
create mode 100644 gdb/testsuite/gdb.arch/aarch64-prologue.exp
Comments
>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
Alan> When saving registers to the stack at the start of a function, not all state
Alan> needs to be saved. For example, only the first 64bits of float registers need
Alan> saving. However, a program may choose to store extra state if it wishes,
Alan> there is nothing preventing it doing so.
Alan> The aarch64_analyze_prologue will error if it detects extra state being
Alan> stored. Relex this restriction.
I don't know anything about AArch64, so I can't really comment on the
content of the patch, but I did happen to see a nit:
Alan> stack.store (pv_add_constant (regs[rn],
Alan> inst.operands[1].addr.offset.imm),
Alan> - is64 ? 8 : 4, regs[rt]);
Alan> + size, regs[rt]);
...this addition looked mis-indented to me.
Tom
> On 5 Aug 2019, at 18:43, Tom Tromey <tom@tromey.com> wrote:
>
>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
>
> Alan> When saving registers to the stack at the start of a function, not all state
> Alan> needs to be saved. For example, only the first 64bits of float registers need
> Alan> saving. However, a program may choose to store extra state if it wishes,
> Alan> there is nothing preventing it doing so.
>
> Alan> The aarch64_analyze_prologue will error if it detects extra state being
> Alan> stored. Relex this restriction.
>
> I don't know anything about AArch64, so I can't really comment on the
> content of the patch, but I did happen to see a nit:
>
> Alan> stack.store (pv_add_constant (regs[rn],
> Alan> inst.operands[1].addr.offset.imm),
> Alan> - is64 ? 8 : 4, regs[rt]);
> Alan> + size, regs[rt]);
>
> ...this addition looked mis-indented to me.
Thanks.
I probably missed that because the correct formatting looks horrible:
stack.store (pv_add_constant (regs[rn],
inst.operands[1].addr.offset.imm),
size, regs[rt]);
Instead, I can update it to the following
stack.store (
pv_add_constant (regs[rn], inst.operands[1].addr.offset.imm),
size, regs[rt]);
>
> Tom
On 06/08/2019 09:58, Alan Hayward wrote:
>
>
>> On 5 Aug 2019, at 18:43, Tom Tromey <tom@tromey.com> wrote:
>>
>>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
>>
>> Alan> When saving registers to the stack at the start of a function, not all state
>> Alan> needs to be saved. For example, only the first 64bits of float registers need
>> Alan> saving. However, a program may choose to store extra state if it wishes,
>> Alan> there is nothing preventing it doing so.
>>
>> Alan> The aarch64_analyze_prologue will error if it detects extra state being
>> Alan> stored. Relex this restriction.
>>
>> I don't know anything about AArch64, so I can't really comment on the
>> content of the patch, but I did happen to see a nit:
>>
>> Alan> stack.store (pv_add_constant (regs[rn],
>> Alan> inst.operands[1].addr.offset.imm),
>> Alan> - is64 ? 8 : 4, regs[rt]);
>> Alan> + size, regs[rt]);
>>
>> ...this addition looked mis-indented to me.
>
> Thanks.
> I probably missed that because the correct formatting looks horrible:
>
> stack.store (pv_add_constant (regs[rn],
> inst.operands[1].addr.offset.imm),
> size, regs[rt]);
>
> Instead, I can update it to the following
>
> stack.store (
> pv_add_constant (regs[rn], inst.operands[1].addr.offset.imm),
> size, regs[rt]);
>
I think the coding convention in that case is to write:
stack.store
(pv_add_constant (regs[rn], inst.operands[1].addr.offset.imm),
size, regs[rt]);
R.
>>
>> Tom
>
> On 6 Aug 2019, at 10:56, Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote:
>
> On 06/08/2019 09:58, Alan Hayward wrote:
>>> On 5 Aug 2019, at 18:43, Tom Tromey <tom@tromey.com> wrote:
>>>
>>>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
>>>
>>> Alan> When saving registers to the stack at the start of a function, not all state
>>> Alan> needs to be saved. For example, only the first 64bits of float registers need
>>> Alan> saving. However, a program may choose to store extra state if it wishes,
>>> Alan> there is nothing preventing it doing so.
>>>
>>> Alan> The aarch64_analyze_prologue will error if it detects extra state being
>>> Alan> stored. Relex this restriction.
>>>
>>> I don't know anything about AArch64, so I can't really comment on the
>>> content of the patch, but I did happen to see a nit:
>>>
>>> Alan> stack.store (pv_add_constant (regs[rn],
>>> Alan> inst.operands[1].addr.offset.imm),
>>> Alan> - is64 ? 8 : 4, regs[rt]);
>>> Alan> + size, regs[rt]);
>>>
>>> ...this addition looked mis-indented to me.
>> Thanks.
>> I probably missed that because the correct formatting looks horrible:
>> stack.store (pv_add_constant (regs[rn],
>> inst.operands[1].addr.offset.imm),
>> size, regs[rt]);
>> Instead, I can update it to the following
>> stack.store (
>> pv_add_constant (regs[rn], inst.operands[1].addr.offset.imm),
>> size, regs[rt]);
>
> I think the coding convention in that case is to write:
>
>
> stack.store
> (pv_add_constant (regs[rn], inst.operands[1].addr.offset.imm),
> size, regs[rt]);
>
> R.
Ok, will use that way.
Alan.
>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
Alan> I probably missed that because the correct formatting looks horrible:
Alan> stack.store (pv_add_constant (regs[rn],
Alan> inst.operands[1].addr.offset.imm),
Alan> size, regs[rt]);
This is a pretty normal style you'd see in gdb.
Alan> Instead, I can update it to the following
Alan> stack.store (
Alan> pv_add_constant (regs[rn], inst.operands[1].addr.offset.imm),
Alan> size, regs[rt]);
gdb doesn't normally use the trailing open paren style like that.
Tom
@@ -384,8 +384,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
{
unsigned rt = inst.operands[0].reg.regno;
unsigned rn = inst.operands[1].addr.base_regno;
- int is64
- = (aarch64_get_qualifier_esize (inst.operands[0].qualifier) == 8);
+ int size = aarch64_get_qualifier_esize (inst.operands[0].qualifier);
gdb_assert (aarch64_num_of_operands (inst.opcode) == 2);
gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt);
@@ -394,7 +393,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
stack.store (pv_add_constant (regs[rn],
inst.operands[1].addr.offset.imm),
- is64 ? 8 : 4, regs[rt]);
+ size, regs[rt]);
}
else if ((inst.opcode->iclass == ldstpair_off
|| (inst.opcode->iclass == ldstpair_indexed
@@ -406,6 +405,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
unsigned rt2;
unsigned rn = inst.operands[2].addr.base_regno;
int32_t imm = inst.operands[2].addr.offset.imm;
+ int size = aarch64_get_qualifier_esize (inst.operands[0].qualifier);
gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt
|| inst.operands[0].type == AARCH64_OPND_Ft);
@@ -427,17 +427,12 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
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);
rt1 += AARCH64_X_REGISTER_COUNT;
rt2 += AARCH64_X_REGISTER_COUNT;
}
- stack.store (pv_add_constant (regs[rn], imm), 8,
- regs[rt1]);
- stack.store (pv_add_constant (regs[rn], imm + 8), 8,
- regs[rt2]);
+ stack.store (pv_add_constant (regs[rn], imm), size, regs[rt1]);
+ stack.store (pv_add_constant (regs[rn], imm + size), size, regs[rt2]);
if (inst.operands[2].addr.writeback)
regs[rn] = pv_add_constant (regs[rn], imm);
@@ -454,21 +449,14 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
unsigned int rt = inst.operands[0].reg.regno;
int32_t imm = inst.operands[1].addr.offset.imm;
unsigned int rn = inst.operands[1].addr.base_regno;
- bool is64
- = (aarch64_get_qualifier_esize (inst.operands[0].qualifier) == 8);
+ int size = aarch64_get_qualifier_esize (inst.operands[0].qualifier);
gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt
|| inst.operands[0].type == AARCH64_OPND_Ft);
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);
- rt += AARCH64_X_REGISTER_COUNT;
- }
+ rt += AARCH64_X_REGISTER_COUNT;
- stack.store (pv_add_constant (regs[rn], imm),
- is64 ? 8 : 4, regs[rt]);
+ stack.store (pv_add_constant (regs[rn], imm), size, regs[rt]);
if (inst.operands[1].addr.writeback)
regs[rn] = pv_add_constant (regs[rn], imm);
}
new file mode 100644
@@ -0,0 +1,83 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2019 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 <stdio.h>
+
+extern void excessiveprologue ();
+
+void
+innerfunc ()
+{
+ printf ("inner\n");
+}
+
+/* excessiveprologue saves to the stack in multiple ways. */
+
+asm ("\t.section .gnu.sgstubs,\"ax\",%progbits\n"
+ "\t.global excessiveprologue\n"
+ "\t.type excessiveprologue, %function\n"
+ "excessiveprologue:\n"
+ "\tstp x29, x30, [sp, #-208]!\n"
+ "\tmov x29, sp\n"
+ "\tstp w0,w1,[sp,16]\n"
+ "\tstp x2,x3,[sp,24]\n"
+ "\tstr w4,[sp,40]\n"
+ "\tstr x5,[sp,48]\n"
+ "\tstur w6,[sp,52]\n"
+ "\tstur x7,[sp,56]\n"
+ "\tstp s0,s1,[sp,64]\n"
+ "\tstp d2,d3,[sp,72]\n"
+ "\tstp q4,q5,[sp,96]\n"
+ "\tstr b6,[sp,128]\n"
+ "\tstr h7,[sp,132]\n"
+ "\tstr s8,[sp,136]\n"
+ "\tstr d9,[sp,140]\n"
+ "\tstr q10,[sp,148]\n"
+ "\tstur b11,[sp,164]\n"
+ "\tstur h12,[sp,160]\n"
+ "\tstur s13,[sp,172]\n"
+ "\tstur d14,[sp,176]\n"
+ "\tstur q15,[sp,184]\n"
+ "\tbl innerfunc\n"
+ "\tldp w0,w1,[sp,16]\n"
+ "\tldp x2,x3,[sp,24]\n"
+ "\tldr w4,[sp,40]\n"
+ "\tldr x5,[sp,48]\n"
+ "\tldur w6,[sp,52]\n"
+ "\tldur x7,[sp,56]\n"
+ "\tldp s0,s1,[sp,64]\n"
+ "\tldp d2,d3,[sp,72]\n"
+ "\tldp q4,q5,[sp,96]\n"
+ "\tldr b6,[sp,128]\n"
+ "\tldr h7,[sp,132]\n"
+ "\tldr s8,[sp,136]\n"
+ "\tldr d9,[sp,140]\n"
+ "\tldr q10,[sp,148]\n"
+ "\tldur b11,[sp,164]\n"
+ "\tldur h12,[sp,160]\n"
+ "\tldur s13,[sp,172]\n"
+ "\tldur d14,[sp,176]\n"
+ "\tldur q15,[sp,184]\n"
+ "\tldp x29, x30, [sp], #208\n"
+ "ret\n");
+
+int
+main (void)
+{
+ excessiveprologue ();
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,36 @@
+# Copyright 2019 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/>.
+
+# This file is part of the gdb testsuite.
+
+if {![is_aarch64_target]} {
+ verbose "Skipping ${gdb_test_file_name}."
+ return 1
+}
+
+standard_testfile
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile {nodebug}]} {
+ return -1
+}
+
+if ![runto_main] {
+ untested "could not run to main"
+ return -1
+}
+
+# Ensure gdb can break at excessiveprologue then continue.
+gdb_breakpoint "excessiveprologue"
+gdb_continue_to_breakpoint "excessiveprologue"
+gdb_continue_to_end "excessiveprologue" "continue" 1