gdbarch: Add pc_signed field and use it when adjusting BP addresses
Commit Message
From: Vlad Ivanov <vlad.ivanov@lab-systems.ru>
MIPS targets use signed PC values. Since commit a0de8c21
single-stepping on these targets didn't work due to the addition of
`address_significant` in adjust_breakpoint_address. This commit
adds a new field to gdbarch struct which indicates the signedness
of the program counter. This field is set to 1 for MIPS by default.
* gdbarch.c: Add pc_signed field and access functions.
* gdbarch.h: Add pc_signed access functions declarations.
* mips-tdep.c (mips_gdbarch_init): Set PC to signed by default.
* breakpoint.c (adjust_breakpoint_address): Check for PC
signedness when calling `address_significant`.
---
gdb/breakpoint.c | 5 ++++-
gdb/gdbarch.c | 20 ++++++++++++++++++++
gdb/gdbarch.h | 4 ++++
gdb/mips-tdep.c | 1 +
4 files changed, 29 insertions(+), 1 deletion(-)
Comments
On Mär 15 2018, vlad.ivanov@lab-systems.ru wrote:
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 454fda7684..247ec34857 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -6999,7 +6999,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
> adjusted_bpaddr = gdbarch_adjust_breakpoint_address (gdbarch, bpaddr);
> }
>
> - adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
> + /* Don't cut out "insignificant" address bits on targets with
> + signed PC. */
> + if (!gdbarch_pc_signed (gdbarch))
> + adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
Shouldn't it be sign-extended instead?
Andreas.
15.03.2018, 14:33, "Andreas Schwab" <schwab@suse.de>:
> On Mär 15 2018, vlad.ivanov@lab-systems.ru wrote:
>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 454fda7684..247ec34857 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -6999,7 +6999,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
>> adjusted_bpaddr = gdbarch_adjust_breakpoint_address (gdbarch, bpaddr);
>> }
>>
>> - adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
>> + /* Don't cut out "insignificant" address bits on targets with
>> + signed PC. */
>> + if (!gdbarch_pc_signed (gdbarch))
>> + adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
>
> Shouldn't it be sign-extended instead?
>
> Andreas.
>
MIPS backend already returns a sign-extended value, and address_significant
cuts out bits 63 to 32. This makes breakpoint address comparison in step
routines to misbehave.
Regards,
Vlad
15.03.2018, 14:33, "Andreas Schwab" <schwab@suse.de>:
> On Mär 15 2018, vlad.ivanov@lab-systems.ru wrote:
>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 454fda7684..247ec34857 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -6999,7 +6999,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
>> adjusted_bpaddr = gdbarch_adjust_breakpoint_address (gdbarch, bpaddr);
>> }
>>
>> - adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
>> + /* Don't cut out "insignificant" address bits on targets with
>> + signed PC. */
>> + if (!gdbarch_pc_signed (gdbarch))
>> + adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
>
> Shouldn't it be sign-extended instead?
Another way would be to extend it after calling address_significant
(I think that's what you meant, sorry). However, I think it would require
even more additions to gdbarch
Regards,
Vlad
15.03.2018, 15:59, "Ulrich Weigand" <uweigand@de.ibm.com>:
> If the address is already correct, why don't you simply set
> gdbarch_significant_addr_bit
> to 64 in the mips back-end instead of adding a new gdbarch routine?
That would affect address printing. Moreover, semantically it's
probably not a good practice because the code would imply that for
all kind of MIPS all 64 bits of address are significant, whereas
in reality it differs from target to target.
Regards,
Vlad
On Mär 15 2018, Vlad Ivanov <vlad.ivanov@lab-systems.ru> wrote:
> 15.03.2018, 14:33, "Andreas Schwab" <schwab@suse.de>:
>> On Mär 15 2018, vlad.ivanov@lab-systems.ru wrote:
>>
>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>>> index 454fda7684..247ec34857 100644
>>> --- a/gdb/breakpoint.c
>>> +++ b/gdb/breakpoint.c
>>> @@ -6999,7 +6999,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
>>> adjusted_bpaddr = gdbarch_adjust_breakpoint_address (gdbarch, bpaddr);
>>> }
>>>
>>> - adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
>>> + /* Don't cut out "insignificant" address bits on targets with
>>> + signed PC. */
>>> + if (!gdbarch_pc_signed (gdbarch))
>>> + adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
>>
>> Shouldn't it be sign-extended instead?
>>
>> Andreas.
>>
>
> MIPS backend already returns a sign-extended value, and address_significant
> cuts out bits 63 to 32. This makes breakpoint address comparison in step
> routines to misbehave.
What happens if you start with a non-canonical address? The call to
address_significant wouldn't be needed if adjusted_bpaddr were always
canonical to begin with.
Andreas.
15.03.2018, 16:46, "Andreas Schwab" <schwab@suse.de>:
>
> What happens if you start with a non-canonical address? The call to
> address_significant wouldn't be needed if adjusted_bpaddr were always
> canonical to begin with.
I don't think MIPS backend can return an address without sign extension.
The call to address_significant was added there before — looking at the
original commit message, another target (AArch64) can return "tagged"
address that needs trimming.
Regards,
Vlad
15.03.2018, 17:06, "Ulrich Weigand" <uweigand@de.ibm.com>:>
>
> I don't see how it can affect address printing. gdbarch_significant_addr_bit
> is not involved in that at all.
>
You're right, I confused it with addr_bits. I'll send another patch.
Regards,
Vlad
@@ -6999,7 +6999,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
adjusted_bpaddr = gdbarch_adjust_breakpoint_address (gdbarch, bpaddr);
}
- adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
+ /* Don't cut out "insignificant" address bits on targets with
+ signed PC. */
+ if (!gdbarch_pc_signed (gdbarch))
+ adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
/* An adjusted breakpoint address can significantly alter
a user's expectations. Print a warning if an adjustment
@@ -191,6 +191,7 @@ struct gdbarch
int addr_bit;
int dwarf2_addr_size;
int char_signed;
+ int pc_signed;
gdbarch_read_pc_ftype *read_pc;
gdbarch_write_pc_ftype *write_pc;
gdbarch_virtual_frame_pointer_ftype *virtual_frame_pointer;
@@ -397,6 +398,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
gdbarch->floatformat_for_type = default_floatformat_for_type;
gdbarch->ptr_bit = gdbarch->int_bit;
gdbarch->char_signed = -1;
+ gdbarch->pc_signed = -1;
gdbarch->virtual_frame_pointer = legacy_virtual_frame_pointer;
gdbarch->num_regs = -1;
gdbarch->sp_regnum = -1;
@@ -550,6 +552,8 @@ verify_gdbarch (struct gdbarch *gdbarch)
gdbarch->dwarf2_addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
if (gdbarch->char_signed == -1)
gdbarch->char_signed = 1;
+ if (gdbarch->pc_signed == -1)
+ gdbarch->pc_signed = 0;
/* Skip verify of read_pc, has predicate. */
/* Skip verify of write_pc, has predicate. */
/* Skip verify of virtual_frame_pointer, invalid_p == 0 */
@@ -1810,6 +1814,22 @@ set_gdbarch_wchar_signed (struct gdbarch *gdbarch,
gdbarch->wchar_signed = wchar_signed;
}
+int
+gdbarch_pc_signed (struct gdbarch *gdbarch)
+{
+ gdb_assert (gdbarch != NULL);
+ gdb_assert (gdbarch->pc_signed != -1);
+ if (gdbarch_debug >= 2)
+ fprintf_unfiltered (gdb_stdlog, "gdbarch_pc_signed called\n");
+ return gdbarch->pc_signed;
+}
+
+void
+set_gdbarch_pc_signed (struct gdbarch *gdbarch, int pc_signed)
+{
+ gdbarch->pc_signed = pc_signed;
+}
+
const struct floatformat **
gdbarch_floatformat_for_type (struct gdbarch *gdbarch, const char *name, int length)
{
@@ -194,6 +194,10 @@ extern void set_gdbarch_wchar_bit (struct gdbarch *gdbarch, int wchar_bit);
extern int gdbarch_wchar_signed (struct gdbarch *gdbarch);
extern void set_gdbarch_wchar_signed (struct gdbarch *gdbarch, int wchar_signed);
+/* One if PC values are signed, zero if unsigned. */
+extern int gdbarch_pc_signed (struct gdbarch *gdbarch);
+extern void set_gdbarch_pc_signed (struct gdbarch *gdbarch, int pc_signed);
+
/* Returns the floating-point format to be used for values of length LENGTH.
NAME, if non-NULL, is the type name, which may be used to distinguish
different target formats of the same length. */
@@ -8685,6 +8685,7 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
/* Add/remove bits from an address. The MIPS needs be careful to
ensure that all 32 bit addresses are sign extended to 64 bits. */
set_gdbarch_addr_bits_remove (gdbarch, mips_addr_bits_remove);
+ set_gdbarch_pc_signed(gdbarch, 1);
/* Unwind the frame. */
set_gdbarch_unwind_pc (gdbarch, mips_unwind_pc);