[2/8] Add new gdbarch method, unconditional_branch_address
Commit Message
Add new gdbarch method, unconditional_branch_address.
gdb/ChangeLog:
* gdbarch.sh (unconditional_branch_address): New gdbarch method.
* gdbarch.h, gdbarch.c: Regenerate.
* arch-utils.h (default_unconditional_branch_address): Declare.
* arch-utils.c (default_unconditional_branch_address): New function.
---
gdb/arch-utils.c | 6 ++++++
gdb/arch-utils.h | 2 ++
gdb/gdbarch.c | 23 +++++++++++++++++++++++
gdb/gdbarch.h | 12 +++++++++++-
gdb/gdbarch.sh | 7 +++++++
5 files changed, 49 insertions(+), 1 deletion(-)
Comments
On 08/19/2015 08:00 AM, Kevin Buettner wrote:
> +# Examine instruction at PC. If instruction at PC is an unconditional
> +# branch, return the address to which control is transferred when the
> +# branch is taken. Return 0 when this method is not implemented by
> +# architecture, PC refers to an invalid address, or instruction at PC
> +# is not an unconditional branch.
> +m:CORE_ADDR:unconditional_branch_address:CORE_ADDR pc:pc::default_unconditional_branch_address::0
> +
In addition to the comments in the other patch, I wonder if we would
better name this in terms of a higher level concept, around
"breakpoint address" instead of a "low level branch destination"?
Thanks,
Pedro Alves
* Kevin Buettner <kevinb@redhat.com> [2015-08-19 00:00:02 -0700]:
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index c1e2c1a..1770960 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -924,7 +924,7 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i
> If your architecture doesn't need to adjust instructions before
> single-stepping them, consider using simple_displaced_step_copy_insn
> here.
> -
> +
> If the instruction cannot execute out of line, return NULL. The
> core falls back to stepping past the instruction in-line instead in
> that case. */
> @@ -1478,6 +1478,16 @@ typedef int (gdbarch_addressable_memory_unit_size_ftype) (struct gdbarch *gdbarc
> extern int gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch);
> extern void set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch, gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size);
>
> +/* Examine instruction at PC. If instruction at PC is an unconditional
> + branch, return the address to which control is transferred when the
> + branch is taken. Return 0 when this method is not implemented by
> + architecture, PC refers to an invalid address, or instruction at PC
> + is not an unconditional branch. */
> +
> +typedef CORE_ADDR (gdbarch_unconditional_branch_address_ftype) (struct gdbarch *gdbarch, CORE_ADDR pc);
> +extern CORE_ADDR gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, CORE_ADDR pc);
> +extern void set_gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, gdbarch_unconditional_branch_address_ftype *unconditional_branch_address);
Personally I'm not a fan of overloading the return values on these
functions, especially when the return value is an address, on some
targets 0 is a valid address. I know there are lots of other places
where we use 0 as a special address in gdb, so this would be just one
more... but...
How would you feel about changing the function so that it returned a
bool and placed the address into a CORE_ADDRESS passed by pointer?
Just a thought,
thanks
Andrew
* Andrew Burgess <andrew.burgess@embecosm.com> [2015-09-18 13:01:57 +0100]:
> * Kevin Buettner <kevinb@redhat.com> [2015-08-19 00:00:02 -0700]:
>
> > diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> > index c1e2c1a..1770960 100644
> > --- a/gdb/gdbarch.h
> > +++ b/gdb/gdbarch.h
> > @@ -924,7 +924,7 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i
> > If your architecture doesn't need to adjust instructions before
> > single-stepping them, consider using simple_displaced_step_copy_insn
> > here.
> > -
> > +
> > If the instruction cannot execute out of line, return NULL. The
> > core falls back to stepping past the instruction in-line instead in
> > that case. */
> > @@ -1478,6 +1478,16 @@ typedef int (gdbarch_addressable_memory_unit_size_ftype) (struct gdbarch *gdbarc
> > extern int gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch);
> > extern void set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch, gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size);
> >
> > +/* Examine instruction at PC. If instruction at PC is an unconditional
> > + branch, return the address to which control is transferred when the
> > + branch is taken. Return 0 when this method is not implemented by
> > + architecture, PC refers to an invalid address, or instruction at PC
> > + is not an unconditional branch. */
> > +
> > +typedef CORE_ADDR (gdbarch_unconditional_branch_address_ftype) (struct gdbarch *gdbarch, CORE_ADDR pc);
> > +extern CORE_ADDR gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, CORE_ADDR pc);
> > +extern void set_gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, gdbarch_unconditional_branch_address_ftype *unconditional_branch_address);
<snip>
> How would you feel about changing the function so that it returned a
> bool and placed the address into a CORE_ADDRESS passed by pointer?
Or I could actually spot your revised patch and see you've already
done this :-/
Sorry for the noise.
Andrew
On Fri, 18 Sep 2015 13:01:57 +0100
Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> * Kevin Buettner <kevinb@redhat.com> [2015-08-19 00:00:02 -0700]:
>
> > diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> > index c1e2c1a..1770960 100644
> > --- a/gdb/gdbarch.h
> > +++ b/gdb/gdbarch.h
> > @@ -924,7 +924,7 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i
> > If your architecture doesn't need to adjust instructions before
> > single-stepping them, consider using simple_displaced_step_copy_insn
> > here.
> > -
> > +
> > If the instruction cannot execute out of line, return NULL. The
> > core falls back to stepping past the instruction in-line instead in
> > that case. */
> > @@ -1478,6 +1478,16 @@ typedef int (gdbarch_addressable_memory_unit_size_ftype) (struct gdbarch *gdbarc
> > extern int gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch);
> > extern void set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch, gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size);
> >
> > +/* Examine instruction at PC. If instruction at PC is an unconditional
> > + branch, return the address to which control is transferred when the
> > + branch is taken. Return 0 when this method is not implemented by
> > + architecture, PC refers to an invalid address, or instruction at PC
> > + is not an unconditional branch. */
> > +
> > +typedef CORE_ADDR (gdbarch_unconditional_branch_address_ftype) (struct gdbarch *gdbarch, CORE_ADDR pc);
> > +extern CORE_ADDR gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, CORE_ADDR pc);
> > +extern void set_gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, gdbarch_unconditional_branch_address_ftype *unconditional_branch_address);
>
> Personally I'm not a fan of overloading the return values on these
> functions, especially when the return value is an address, on some
> targets 0 is a valid address. I know there are lots of other places
> where we use 0 as a special address in gdb, so this would be just one
> more... but...
>
> How would you feel about changing the function so that it returned a
> bool and placed the address into a CORE_ADDRESS passed by pointer?
Pedro had a similar observation. The new patch that I posted does this.
(Though it uses int instead of bool.)
Kevin
On Fri, 18 Sep 2015 13:06:06 +0100
Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> * Andrew Burgess <andrew.burgess@embecosm.com> [2015-09-18 13:01:57 +0100]:
>
> > * Kevin Buettner <kevinb@redhat.com> [2015-08-19 00:00:02 -0700]:
> >
> > > diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> > > index c1e2c1a..1770960 100644
> > > --- a/gdb/gdbarch.h
> > > +++ b/gdb/gdbarch.h
> > > @@ -924,7 +924,7 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i
> > > If your architecture doesn't need to adjust instructions before
> > > single-stepping them, consider using simple_displaced_step_copy_insn
> > > here.
> > > -
> > > +
> > > If the instruction cannot execute out of line, return NULL. The
> > > core falls back to stepping past the instruction in-line instead in
> > > that case. */
> > > @@ -1478,6 +1478,16 @@ typedef int (gdbarch_addressable_memory_unit_size_ftype) (struct gdbarch *gdbarc
> > > extern int gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch);
> > > extern void set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch, gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size);
> > >
> > > +/* Examine instruction at PC. If instruction at PC is an unconditional
> > > + branch, return the address to which control is transferred when the
> > > + branch is taken. Return 0 when this method is not implemented by
> > > + architecture, PC refers to an invalid address, or instruction at PC
> > > + is not an unconditional branch. */
> > > +
> > > +typedef CORE_ADDR (gdbarch_unconditional_branch_address_ftype) (struct gdbarch *gdbarch, CORE_ADDR pc);
> > > +extern CORE_ADDR gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, CORE_ADDR pc);
> > > +extern void set_gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, gdbarch_unconditional_branch_address_ftype *unconditional_branch_address);
>
> <snip>
>
> > How would you feel about changing the function so that it returned a
> > bool and placed the address into a CORE_ADDRESS passed by pointer?
>
> Or I could actually spot your revised patch and see you've already
> done this :-/
Oops. I'm guilty of this too. (I should have looked ahead to see this email.)
Kevin
Kevin Buettner <kevinb@redhat.com> writes:
> Add new gdbarch method, unconditional_branch_address.
>
> gdb/ChangeLog:
>
> * gdbarch.sh (unconditional_branch_address): New gdbarch method.
> * gdbarch.h, gdbarch.c: Regenerate.
> * arch-utils.h (default_unconditional_branch_address): Declare.
> * arch-utils.c (default_unconditional_branch_address): New function.
Hi Kevin,
Did you consider using existing gdbarch method
adjust_breakpoint_address when you wrote the patch? Can we use
adjust_breakpoint_address here rather than adding a new gdbarch method?
On Tue, 22 Sep 2015 17:09:17 +0100
Yao Qi <qiyaoltc@gmail.com> wrote:
> Kevin Buettner <kevinb@redhat.com> writes:
>
> > Add new gdbarch method, unconditional_branch_address.
> >
> > gdb/ChangeLog:
> >
> > * gdbarch.sh (unconditional_branch_address): New gdbarch method.
> > * gdbarch.h, gdbarch.c: Regenerate.
> > * arch-utils.h (default_unconditional_branch_address): Declare.
> > * arch-utils.c (default_unconditional_branch_address): New function.
>
> Did you consider using existing gdbarch method
> adjust_breakpoint_address when you wrote the patch? Can we use
> adjust_breakpoint_address here rather than adding a new gdbarch method?
Hi Yao,
The adjust_breakpoint_address method is not suitable for this patch
set. It was originally added for the FR-V architecture. FR-V is a
VLIW architecture which places limits on where a breakpoint may be
placed within a bundle. For FR-V, this method ensures that a breakpoint
is placed at the beginning of the VLIW bundle. I see now that it has
also been used for MIPS to avoid placing breakpoints on branch delay
slots and also for ARM to avoid placing breakpoints within IT
(if-then) blocks. In each of these cases, the breakpoint is moved
because, if is not, the breakpoint might not be hit.
The gdbarch method that I'm introducing in this patch set needs to
limit itself to checking to see if the instruction at a given address
is an unconditional branch; if it is, it will return true and also
provide the branch destination address via an output parameter. An
existing gdbarch method that might be suitable (with some
modification) for this purpose is insn_is_jump, which is presently
used by the btrace code.
The insn_is_jump method is a predicate only; at the moment, it does
not provide the branch destination. It could be changed to do so,
but I think it makes sense to limit use of the insn_is_{jump,ret,call}
methods to btrace. This is the justification that I provided in
another reply to this thread:
I decided that the work I'm doing here should have it's own
method. If GCC should someday emit DWARF which sets is_stmt to 0
for the branch instruction at the beginning of a while loop, this
work might not be needed any longer. It'll be easier to rip it
out and clean things up again if I don't modify gdbarch methods
that were added for other purposes.
Kevin
@@ -820,6 +820,12 @@ default_gen_return_address (struct gdbarch *gdbarch,
error (_("This architecture has no method to collect a return address."));
}
+CORE_ADDR
+default_unconditional_branch_address (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+ return 0;
+}
+
int
default_return_in_first_hidden_param_p (struct gdbarch *gdbarch,
struct type *type)
@@ -204,4 +204,6 @@ extern char *default_gcc_target_options (struct gdbarch *gdbarch);
extern const char *default_gnu_triplet_regexp (struct gdbarch *gdbarch);
extern int default_addressable_memory_unit_size (struct gdbarch *gdbarch);
+extern CORE_ADDR default_unconditional_branch_address (struct gdbarch *gdbarch, CORE_ADDR pc);
+
#endif
@@ -330,6 +330,7 @@ struct gdbarch
gdbarch_gcc_target_options_ftype *gcc_target_options;
gdbarch_gnu_triplet_regexp_ftype *gnu_triplet_regexp;
gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size;
+ gdbarch_unconditional_branch_address_ftype *unconditional_branch_address;
};
/* Create a new ``struct gdbarch'' based on information provided by
@@ -432,6 +433,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
gdbarch->gcc_target_options = default_gcc_target_options;
gdbarch->gnu_triplet_regexp = default_gnu_triplet_regexp;
gdbarch->addressable_memory_unit_size = default_addressable_memory_unit_size;
+ gdbarch->unconditional_branch_address = default_unconditional_branch_address;
/* gdbarch_alloc() */
return gdbarch;
@@ -666,6 +668,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
/* Skip verify of gcc_target_options, invalid_p == 0 */
/* Skip verify of gnu_triplet_regexp, invalid_p == 0 */
/* Skip verify of addressable_memory_unit_size, invalid_p == 0 */
+ /* Skip verify of unconditional_branch_address, invalid_p == 0 */
buf = ui_file_xstrdup (log, &length);
make_cleanup (xfree, buf);
if (length > 0)
@@ -1356,6 +1359,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
"gdbarch_dump: target_desc = %s\n",
host_address_to_string (gdbarch->target_desc));
fprintf_unfiltered (file,
+ "gdbarch_dump: unconditional_branch_address = <%s>\n",
+ host_address_to_string (gdbarch->unconditional_branch_address));
+ fprintf_unfiltered (file,
"gdbarch_dump: gdbarch_unwind_pc_p() = %d\n",
gdbarch_unwind_pc_p (gdbarch));
fprintf_unfiltered (file,
@@ -4753,6 +4759,23 @@ set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch,
gdbarch->addressable_memory_unit_size = addressable_memory_unit_size;
}
+CORE_ADDR
+gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+ gdb_assert (gdbarch != NULL);
+ gdb_assert (gdbarch->unconditional_branch_address != NULL);
+ if (gdbarch_debug >= 2)
+ fprintf_unfiltered (gdb_stdlog, "gdbarch_unconditional_branch_address called\n");
+ return gdbarch->unconditional_branch_address (gdbarch, pc);
+}
+
+void
+set_gdbarch_unconditional_branch_address (struct gdbarch *gdbarch,
+ gdbarch_unconditional_branch_address_ftype unconditional_branch_address)
+{
+ gdbarch->unconditional_branch_address = unconditional_branch_address;
+}
+
/* Keep a registry of per-architecture data-pointers required by GDB
modules. */
@@ -924,7 +924,7 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i
If your architecture doesn't need to adjust instructions before
single-stepping them, consider using simple_displaced_step_copy_insn
here.
-
+
If the instruction cannot execute out of line, return NULL. The
core falls back to stepping past the instruction in-line instead in
that case. */
@@ -1478,6 +1478,16 @@ typedef int (gdbarch_addressable_memory_unit_size_ftype) (struct gdbarch *gdbarc
extern int gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch);
extern void set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch, gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size);
+/* Examine instruction at PC. If instruction at PC is an unconditional
+ branch, return the address to which control is transferred when the
+ branch is taken. Return 0 when this method is not implemented by
+ architecture, PC refers to an invalid address, or instruction at PC
+ is not an unconditional branch. */
+
+typedef CORE_ADDR (gdbarch_unconditional_branch_address_ftype) (struct gdbarch *gdbarch, CORE_ADDR pc);
+extern CORE_ADDR gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, CORE_ADDR pc);
+extern void set_gdbarch_unconditional_branch_address (struct gdbarch *gdbarch, gdbarch_unconditional_branch_address_ftype *unconditional_branch_address);
+
/* Definition for an unknown syscall, used basically in error-cases. */
#define UNKNOWN_SYSCALL (-1)
@@ -1123,6 +1123,13 @@ m:const char *:gnu_triplet_regexp:void:::default_gnu_triplet_regexp::0
# each address in memory.
m:int:addressable_memory_unit_size:void:::default_addressable_memory_unit_size::0
+# Examine instruction at PC. If instruction at PC is an unconditional
+# branch, return the address to which control is transferred when the
+# branch is taken. Return 0 when this method is not implemented by
+# architecture, PC refers to an invalid address, or instruction at PC
+# is not an unconditional branch.
+m:CORE_ADDR:unconditional_branch_address:CORE_ADDR pc:pc::default_unconditional_branch_address::0
+
EOF
}