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

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

[vlad.ivanov]

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(-)

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);
-- 
2.14.3

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Vlad Ivanov]

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

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Vlad Ivanov]

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

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Ulrich Weigand]

Vlad Ivanov 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.

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?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Vlad Ivanov]

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

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Vlad Ivanov]

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

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Ulrich Weigand]

Vlad Ivanov wrote:
> 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.

I don't see how it can affect address printing.  gdbarch_significant_addr_bit
is not involved in that at all.

> 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.

Well, all 64 bits *are* significant in the relevant sense here.
"Significant" here only means, the bit already has the correct value
that it needs to have if you want to access the intended memory word
via ptrace.

The only reason to ever set gdbarch_significant_addr_bit to anything
less than the host word size is if addresses contain some tags or
other extraneous bits that actively need to be trimmed.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Vlad Ivanov]

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

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Maciej W. Rozycki]

On Thu, 15 Mar 2018, Ulrich Weigand wrote:

> > 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.
> 
> 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?

 I think it's the default from commit a738ea1d41da ("Clear non-significant 
bits of address on memory access") that is wrong.  The default is set to 
`gdbarch_addr_bit (gdbarch)'.  Instead I think it should be set to the BFD 
VMA width, i.e. `8 * sizeof (bfd_vma)' or suchlike, so that the internal 
representation does not get truncated inadvertently.

 More targets will be affected; e.g. as I recall SH also has signed 
addresses.

  Maciej

Re: [PATCH] gdbarch: Add pc_signed field and use it when adjusting BP addresses

[Ulrich Weigand]

Maciej W. Rozycki wrote:
> On Thu, 15 Mar 2018, Ulrich Weigand wrote:
> 
> > > 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.
> > 
> > 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?
> 
>  I think it's the default from commit a738ea1d41da ("Clear non-significant 
> bits of address on memory access") that is wrong.  The default is set to 
> `gdbarch_addr_bit (gdbarch)'.  Instead I think it should be set to the BFD 
> VMA width, i.e. `8 * sizeof (bfd_vma)' or suchlike, so that the internal 
> representation does not get truncated inadvertently.

Good point.  I agree it makes sense to change the default here.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com