diff mbox

gdbarch: Add pc_signed field and use it when adjusting BP addresses

Message ID 20180315112111.15247-1-vlad.ivanov@lab-systems.ru
State New
Headers show

Commit Message

vlad.ivanov@lab-systems.ru March 15, 2018, 11:21 a.m. UTC
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

Andreas Schwab March 15, 2018, 11:33 a.m. UTC | #1
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.
vlad.ivanov@lab-systems.ru March 15, 2018, 11:36 a.m. UTC | #2
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
vlad.ivanov@lab-systems.ru March 15, 2018, 11:43 a.m. UTC | #3
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
vlad.ivanov@lab-systems.ru March 15, 2018, 1:10 p.m. UTC | #4
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
Andreas Schwab March 15, 2018, 1:46 p.m. UTC | #5
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.
vlad.ivanov@lab-systems.ru March 15, 2018, 1:56 p.m. UTC | #6
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
vlad.ivanov@lab-systems.ru March 15, 2018, 2:23 p.m. UTC | #7
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
diff mbox

Patch

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);
 
       /* An adjusted breakpoint address can significantly alter
          a user's expectations.  Print a warning if an adjustment
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index b8703e5a55..4d464c28f7 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -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)
 {
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 5cb131de1d..7af40154e0 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -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. */
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index f9f84c4d48..6389bdbb95 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -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);