RE: [draft patch] unwinder for btrace [Re: [rfc 3/5] record: make it build again]

Metzger, Markus T
2013-02-13T07:35:04Z

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Monday, February 11, 2013 6:13 PM

Thanks, Jan,

I'll look at it once the "record disassembly" and "record list" commands are working for record-btrace.

Did you commit this to some archer branch, as well?

Thanks,
Markus.

> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org; markus.t.metzger@gmail.com
> Subject: [draft patch] <unavailable> unwinder for btrace [Re: [rfc 3/5] record: make it build again]
>
> On Mon, 11 Feb 2013 15:14:51 +0100, Jan Kratochvil wrote:
> > FYI I have some draft <unavailable>-limitation unwinder for btrace here today,
> > to post it later.
>
> ./gdb -q ./gdb -ex start -ex record -ex stepi -ex reverse-stepi -ex bt -ex q
>
> No more reverse-execution history.
> main (argc=<unavailable>, argv=<unavailable>) at gdb.c:29
> 29 memset (&args, 0, sizeof args);
> #0 main (argc=<unavailable>, argv=<unavailable>) at gdb.c:29
> Backtrace stopped: not enough registers or memory available to unwind further
>
>
> That get_frame_unwind_stop_reason "optimization" to avoid calling an unwinder
> was present already in the initial post:
> [rfc, frame] Add backtrace stop reasons
> http://sourceware.org/ml/gdb-patches/2006-08/msg00129.html
> Message-ID: <20060819154646.GA25238@nevyn.them.org>
> I don't know its real reason but it is needed for this patch otherwise:
> main (argc=<error reading variable: can't compute CFA for this frame>, argv=<error reading variable: can't compute CFA for this
> frame>) at gdb.c:29
>
>
> For btrace you will need ptid_t of the unwound thread as the RECORD_IS_REPLAY
> flag is apparently irrelevant there. I find inferior_ptid to use there
> acceptable despite it is discouraged now, passing there ptid_t from the
> unwinder framework needs some larger changes.
>
> No real regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu and in
> gdbserver mode, with the patch as is the various *reverse* tests apparently
> FAIL due to the <unavailable> values (both displayed and implicitly missing).
>
> There should be a testcase but that will be dependent upon the btrace
> integration.
>
>
> Regards,
> Jan
>
>
> gdb/
> 2013-02-11 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> New record unwinder reporting <unavailable>.
> * dwarf2-frame.c (dwarf2_frame_cfa): Move UNWIND_UNAVAILABLE check
> earlier.
> * frame-unwind.c: Include target.h.
> (frame_unwind_try_unwinder): New function with code from ...
> (frame_unwind_find_by_frame): ... here. New variable
> unwinder_from_target, call also target_get_unwinder and
> frame_unwind_try_unwinder for it.
> * frame.c (get_frame_unwind_stop_reason): Unconditionally call
> get_prev_frame_1.
> * record.c: Include frame-unwind.h.
> (record_frame_unwind_stop_reason, record_frame_this_id)
> (record_frame_prev_register, record_frame_sniffer, record_frame_unwind):
> New.
> (init_record_ops, init_record_core_ops): Install it.
> * target.c (target_get_unwinder): New.
> * target.h (struct target_ops): New field to_get_unwinder.
> (target_get_unwinder): New declaration.
>
> diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
> index ec4edfa..45be107 100644
> --- a/gdb/dwarf2-frame.c
> +++ b/gdb/dwarf2-frame.c
> @@ -1497,16 +1497,16 @@ dwarf2_frame_cfa (struct frame_info *this_frame)
> {
> while (get_frame_type (this_frame) == INLINE_FRAME)
> this_frame = get_prev_frame (this_frame);
> + if (get_frame_unwind_stop_reason (this_frame) == UNWIND_UNAVAILABLE)
> + throw_error (NOT_AVAILABLE_ERROR,
> + _("can't compute CFA for this frame: "
> + "required registers or memory are unavailable"));
> /* This restriction could be lifted if other unwinders are known to
> compute the frame base in a way compatible with the DWARF
> unwinder. */
> if (!frame_unwinder_is (this_frame, &dwarf2_frame_unwind)
> && !frame_unwinder_is (this_frame, &dwarf2_tailcall_frame_unwind))
> error (_("can't compute CFA for this frame"));
> - if (get_frame_unwind_stop_reason (this_frame) == UNWIND_UNAVAILABLE)
> - throw_error (NOT_AVAILABLE_ERROR,
> - _("can't compute CFA for this frame: "
> - "required registers or memory are unavailable"));
> return get_frame_base (this_frame);
> }
>

> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index b66febf..cf33147 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -27,6 +27,7 @@
> #include "exceptions.h"
> #include "gdb_assert.h"
> #include "gdb_obstack.h"
> +#include "target.h"
>
> static struct gdbarch_data *frame_unwind_data;
>
> @@ -88,6 +89,48 @@ frame_unwind_append_unwinder (struct gdbarch *gdbarch,
> (*ip)->unwinder = unwinder;
> }
>
> +/* Call SNIFFER from UNWINDER. If it succeeded set UNWINDER for
> + THIS_FRAME and return 1. Otherwise the function keeps THIS_FRAME
> + unchanged and returns 0. */
> +
> +static int
> +frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache,
> + const struct frame_unwind *unwinder)
> +{
> + struct cleanup *old_cleanup;
> + volatile struct gdb_exception ex;
> + int res = 0;
> +
> + old_cleanup = frame_prepare_for_sniffer (this_frame, unwinder);
> +
> + TRY_CATCH (ex, RETURN_MASK_ERROR)
> + {
> + res = unwinder->sniffer (unwinder, this_frame, this_cache);
> + }
> + if (ex.reason < 0 && ex.error == NOT_AVAILABLE_ERROR)
> + {
> + /* This usually means that not even the PC is available,
> + thus most unwinders aren't able to determine if they're
> + the best fit. Keep trying. Fallback prologue unwinders
> + should always accept the frame. */
> + do_cleanups (old_cleanup);
> + return 0;
> + }
> + else if (ex.reason < 0)
> + throw_exception (ex);
> + else if (res)
> + {
> + discard_cleanups (old_cleanup);
> + return 1;
> + }
> + else
> + {
> + do_cleanups (old_cleanup);
> + return 0;
> + }
> + gdb_assert_not_reached ("frame_unwind_try_unwinder");
> +}
> +
> /* Iterate through sniffers for THIS_FRAME frame until one returns with an
> unwinder implementation. THIS_FRAME->UNWIND must be NULL, it will get set
> by this function. Possibly initialize THIS_CACHE. */
> @@ -98,37 +141,18 @@ frame_unwind_find_by_frame (struct frame_info *this_frame, void **this_cache)
> struct gdbarch *gdbarch = get_frame_arch (this_frame);
> struct frame_unwind_table *table = gdbarch_data (gdbarch, frame_unwind_data);
> struct frame_unwind_table_entry *entry;
> + const struct frame_unwind *unwinder_from_target;
> +
> + unwinder_from_target = target_get_unwinder ();
> + if (unwinder_from_target != NULL
> + && frame_unwind_try_unwinder (this_frame, this_cache,
> + unwinder_from_target))
> + return;
>
> for (entry = table->list; entry != NULL; entry = entry->next)
> - {
> - struct cleanup *old_cleanup;
> - volatile struct gdb_exception ex;
> - int res = 0;
> -
> - old_cleanup = frame_prepare_for_sniffer (this_frame, entry->unwinder);
> -
> - TRY_CATCH (ex, RETURN_MASK_ERROR)
> - {
> - res = entry->unwinder->sniffer (entry->unwinder, this_frame,
> - this_cache);
> - }
> - if (ex.reason < 0 && ex.error == NOT_AVAILABLE_ERROR)
> - {
> - /* This usually means that not even the PC is available,
> - thus most unwinders aren't able to determine if they're
> - the best fit. Keep trying. Fallback prologue unwinders
> - should always accept the frame. */
> - }
> - else if (ex.reason < 0)
> - throw_exception (ex);
> - else if (res)
> - {
> - discard_cleanups (old_cleanup);
> - return;
> - }
> + if (frame_unwind_try_unwinder (this_frame, this_cache, entry->unwinder))
> + return;
>
> - do_cleanups (old_cleanup);
> - }
> internal_error (__FILE__, __LINE__, _("frame_unwind_find_by_frame failed"));
> }
>
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 0821b6e..8fc1ca9 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -2357,13 +2357,10 @@ get_frame_sp (struct frame_info *this_frame)
> enum unwind_stop_reason
> get_frame_unwind_stop_reason (struct frame_info *frame)
> {
> - /* If we haven't tried to unwind past this point yet, then assume
> - that unwinding would succeed. */
> - if (frame->prev_p == 0)
> - return UNWIND_NO_REASON;
> + /* Fill-in STOP_REASON. */
> + get_prev_frame_1 (frame);
> + gdb_assert (frame->prev_p);
>
> - /* Otherwise, we set a reason when we succeeded (or failed) to
> - unwind. */
> return frame->stop_reason;
> }
>
> diff --git a/gdb/record.c b/gdb/record.c
> index 1a68738..dffce98 100644
> --- a/gdb/record.c
> +++ b/gdb/record.c
> @@ -34,6 +34,7 @@
> #include "inf-loop.h"
> #include "gdb_bfd.h"
> #include "observer.h"
> +#include "frame-unwind.h"
>
> #include <signal.h>
>
> @@ -1953,6 +1954,59 @@ record_execution_direction (void)
> return record_execution_dir;
> }
>
> +/* Implement stop_reason method for record_frame_unwind. */
> +
> +static enum unwind_stop_reason
> +record_frame_unwind_stop_reason (struct frame_info *this_frame,
> + void **this_cache)
> +{
> + return UNWIND_UNAVAILABLE;
> +}
> +
> +/* Implement this_id method for record_frame_unwind. */
> +
> +static void
> +record_frame_this_id (struct frame_info *this_frame, void **this_cache,
> + struct frame_id *this_id)
> +{
> + /* Leave there the outer_frame_id value. */
> +}
> +
> +/* Implement prev_register method for record_frame_unwind. */
> +
> +static struct value *
> +record_frame_prev_register (struct frame_info *this_frame, void **this_cache,
> + int regnum)
> +{
> + throw_error (NOT_AVAILABLE_ERROR,
> + _("Registers are not available in btrace record history"));
> +}
> +
> +/* Implement sniffer method for record_frame_unwind. */
> +
> +static int
> +record_frame_sniffer (const struct frame_unwind *self,
> + struct frame_info *this_frame, void **this_cache)
> +{
> + return RECORD_IS_REPLAY;
> +}
> +
> +/* btrace recording does not store previous memory content, neither the stack
> + frames content. Any unwinding would return errorneous results as the stack
> + contents no longer matches the changed PC value restored from history.
> + Therefore this unwinder reports any possibly unwound registers as
> + <unavailable>. */
> +
> +static const struct frame_unwind record_frame_unwind =
> +{
> + NORMAL_FRAME,
> + record_frame_unwind_stop_reason,
> + record_frame_this_id,
> + record_frame_prev_register,
> + NULL,
> + record_frame_sniffer
> +};
> +
> static void
> init_record_ops (void)
> {
> @@ -1984,6 +2038,7 @@ init_record_ops (void)
> record_ops.to_can_async_p = record_can_async_p;
> record_ops.to_is_async_p = record_is_async_p;
> record_ops.to_execution_direction = record_execution_direction;
> + record_ops.to_get_unwinder = &record_frame_unwind;
> record_ops.to_magic = OPS_MAGIC;
> }
>
> @@ -2209,6 +2264,7 @@ init_record_core_ops (void)
> record_core_ops.to_can_async_p = record_can_async_p;
> record_core_ops.to_is_async_p = record_is_async_p;
> record_core_ops.to_execution_direction = record_execution_direction;
> + record_core_ops.to_get_unwinder = &record_frame_unwind;
> record_core_ops.to_magic = OPS_MAGIC;
> }
>
> diff --git a/gdb/target.c b/gdb/target.c
> index 9d8bf6e..28f00af 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -4147,6 +4147,20 @@ target_ranged_break_num_registers (void)
> return -1;
> }
>
> +/* See target.h. */
> +
> +const struct frame_unwind *
> +target_get_unwinder (void)
> +{
> + struct target_ops *t;
> +
> + for (t = current_target.beneath; t != NULL; t = t->beneath)
> + if (t->to_get_unwinder != NULL)
> + return t->to_get_unwinder;
> +
> + return NULL;
> +}
> +
> static void
> debug_to_prepare_to_store (struct regcache *regcache)
> {
> diff --git a/gdb/target.h b/gdb/target.h
> index 1971265..95bfb47 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -857,6 +857,10 @@ struct target_ops
> /* Is the target able to use agent in current state? */
> int (*to_can_use_agent) (void);
>
> + /* This unwinder is tried before any other arch unwinders. Use NULL if it
> + is not used. */
> + const struct frame_unwind *to_get_unwinder;
> +
> int to_magic;
> /* Need sub-structure for target machine related rather than comm related?
> */
> @@ -1735,6 +1739,9 @@ extern char *target_fileio_read_stralloc (const char *filename);
>
> extern int target_core_of_thread (ptid_t ptid);
>
> +/* See to_get_unwinder in struct target_ops. */
> +extern const struct frame_unwind *target_get_unwinder (void);
> +
> /* Verify that the memory in the [MEMADDR, MEMADDR+SIZE) range matches
> the contents of [DATA,DATA+SIZE). Returns 1 if there's a match, 0
> if there's a mismatch, and -1 if an error is encountered while
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052