Re: [PATCH v3 4/5] Continue making GDB use frame_info_ptr

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

<snip>

> diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
> index 7f30a0d3126..e4c931ad3b9 100644
> --- a/gdb/dwarf2/frame-tailcall.c
> +++ b/gdb/dwarf2/frame-tailcall.c
> @@ -39,7 +39,7 @@ static htab_t cache_htab;
>  struct tailcall_cache
>  {
>    /* It must be the first one of this struct.  It is the furthest callee.  */
> -  struct frame_info *next_bottom_frame;
> +  frame_info *next_bottom_frame;
>  
>    /* Reference count.  The whole chain of virtual tail call frames shares one
>       tailcall_cache.  */
> @@ -90,12 +90,12 @@ cache_eq (const void *arg1, const void *arg2)
>     tailcall_cache.  */
>  
>  static struct tailcall_cache *
> -cache_new_ref1 (struct frame_info *next_bottom_frame)
> +cache_new_ref1 (frame_info_ptr next_bottom_frame)
>  {
>    struct tailcall_cache *cache = XCNEW (struct tailcall_cache);
>    void **slot;
>  
> -  cache->next_bottom_frame = next_bottom_frame;
> +  cache->next_bottom_frame = next_bottom_frame.get();

Ooops!  Missing space before ().

<snip>

> diff --git a/gdb/guile/scm-frame.c b/gdb/guile/scm-frame.c
> index d49233661e6..6152659c940 100644
> --- a/gdb/guile/scm-frame.c
> +++ b/gdb/guile/scm-frame.c
> @@ -204,7 +204,7 @@ gdbscm_frame_p (SCM scm)
>     Returns a <gdb:exception> object if there is an error.  */
>  
>  static SCM
> -frscm_scm_from_frame (struct frame_info *frame, struct inferior *inferior)
> +frscm_scm_from_frame (frame_info_ptr frame, struct inferior *inferior)
>  {
>    frame_smob *f_smob, f_smob_for_lookup;
>    SCM f_scm;
> @@ -263,7 +263,7 @@ frscm_scm_from_frame (struct frame_info *frame, struct inferior *inferior)
>     A Scheme exception is thrown if there is an error.  */
>  
>  static SCM
> -frscm_scm_from_frame_unsafe (struct frame_info *frame,
> +frscm_scm_from_frame_unsafe (frame_info_ptr frame,
>  			     struct inferior *inferior)
>  {
>    SCM f_scm = frscm_scm_from_frame (frame, inferior);
> @@ -287,7 +287,7 @@ frscm_get_frame_arg_unsafe (SCM self, int arg_pos, const char *func_name)
>  }
>  
>  /* There is no gdbscm_scm_to_frame function because translating
> -   a frame SCM object to a struct frame_info * can throw a GDB error.
> +   a frame SCM object to a frame_info_ptr  can throw a GDB error.
>     Thus code working with frames has to handle both Scheme errors (e.g., the
>     object is not a frame) and GDB errors (e.g., the frame lookup failed).
>  
> @@ -331,10 +331,10 @@ frscm_get_frame_smob_arg_unsafe (SCM self, int arg_pos, const char *func_name)
>     This function calls GDB routines, so don't assume a GDB error will
>     not be thrown.  */
>  
> -struct frame_info *
> +frame_info_ptr
>  frscm_frame_smob_to_frame (frame_smob *f_smob)
>  {
> -  struct frame_info *frame;
> +  frame_info_ptr frame;
>  
>    frame = frame_find_by_id (f_smob->frame_id);
>    if (frame == NULL)
> @@ -386,7 +386,7 @@ static SCM
>  gdbscm_frame_valid_p (SCM self)
>  {
>    frame_smob *f_smob;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
>  
> @@ -414,7 +414,7 @@ gdbscm_frame_name (SCM self)
>    frame_smob *f_smob;
>    gdb::unique_xmalloc_ptr<char> name;
>    enum language lang = language_minimal;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>    SCM result;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
> @@ -454,7 +454,7 @@ gdbscm_frame_type (SCM self)
>  {
>    frame_smob *f_smob;
>    enum frame_type type = NORMAL_FRAME;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
>  
> @@ -487,7 +487,7 @@ static SCM
>  gdbscm_frame_arch (SCM self)
>  {
>    frame_smob *f_smob;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
>  
> @@ -518,7 +518,7 @@ static SCM
>  gdbscm_frame_unwind_stop_reason (SCM self)
>  {
>    frame_smob *f_smob;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>    enum unwind_stop_reason stop_reason;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
> @@ -553,7 +553,7 @@ gdbscm_frame_pc (SCM self)
>  {
>    frame_smob *f_smob;
>    CORE_ADDR pc = 0;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
>  
> @@ -587,7 +587,7 @@ gdbscm_frame_block (SCM self)
>  {
>    frame_smob *f_smob;
>    const struct block *block = NULL, *fn_block;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
>  
> @@ -639,7 +639,7 @@ gdbscm_frame_function (SCM self)
>  {
>    frame_smob *f_smob;
>    struct symbol *sym = NULL;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
>  
> @@ -676,8 +676,8 @@ static SCM
>  gdbscm_frame_older (SCM self)
>  {
>    frame_smob *f_smob;
> -  struct frame_info *prev = NULL;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr prev = NULL;
> +  frame_info_ptr frame = NULL;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
>  
> @@ -714,8 +714,8 @@ static SCM
>  gdbscm_frame_newer (SCM self)
>  {
>    frame_smob *f_smob;
> -  struct frame_info *next = NULL;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr next = NULL;
> +  frame_info_ptr frame = NULL;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
>  
> @@ -752,7 +752,7 @@ gdbscm_frame_sal (SCM self)
>  {
>    frame_smob *f_smob;
>    struct symtab_and_line sal;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
>  
> @@ -786,7 +786,7 @@ gdbscm_frame_read_register (SCM self, SCM register_scm)
>  {
>    char *register_str;
>    struct value *value = NULL;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>    frame_smob *f_smob;
>  
>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
> @@ -847,7 +847,7 @@ gdbscm_frame_read_var (SCM self, SCM symbol_scm, SCM rest)
>    frame_smob *f_smob;
>    int block_arg_pos = -1;
>    SCM block_scm = SCM_UNDEFINED;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;

Unfortunately, at least this change, and possibly other changes in the
guile code isn't safe, this causes a test failure in gdb.guile/scm-frame.exp.

The guile API is not great for embedding into C++ as it makes use of
longjmp for exception handling.  Any call that ends up at scm_throw will
perform a longjmp, this includes things like GDBSCM_HANDLE_GDB_EXCEPTION
and gdbscm_invalid_object_error.

We either need to be really careful about the scope of any
frame_info_ptr objects in guile code, or, avoid frame_info_ptr in guile
code and just fall back to raw frame_info*.

<snip>

> diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
> index ab4e52d16d7..7eea8e72616 100644
> --- a/gdb/ia64-tdep.c
> +++ b/gdb/ia64-tdep.c
> @@ -1217,7 +1217,7 @@ ia64_convert_register_p (struct gdbarch *gdbarch, int regno, struct type *type)
>  }
>  
>  static int
> -ia64_register_to_value (struct frame_info *frame, int regnum,
> +ia64_register_to_value (frame_info_ptr frame, int regnum,
>  			struct type *valtype, gdb_byte *out,
>  			int *optimizedp, int *unavailablep)
>  {
> @@ -1238,7 +1238,7 @@ ia64_register_to_value (struct frame_info *frame, int regnum,
>  }
>  
>  static void
> -ia64_value_to_register (struct frame_info *frame, int regnum,
> +ia64_value_to_register (frame_info_ptr frame, int regnum,
>  			 struct type *valtype, const gdb_byte *in)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (frame);
> @@ -1358,7 +1358,7 @@ ia64_alloc_frame_cache (void)
>  
>  static CORE_ADDR
>  examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc,
> -		  struct frame_info *this_frame,
> +		  frame_info_ptr this_frame,
>  		  struct ia64_frame_cache *cache)
>  {
>    CORE_ADDR next_pc;
> @@ -1839,7 +1839,7 @@ ia64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>  /* Normal frames.  */
>  
>  static struct ia64_frame_cache *
> -ia64_frame_cache (struct frame_info *this_frame, void **this_cache)
> +ia64_frame_cache (frame_info_ptr this_frame, void **this_cache)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> @@ -1884,7 +1884,7 @@ ia64_frame_cache (struct frame_info *this_frame, void **this_cache)
>  }
>  
>  static void
> -ia64_frame_this_id (struct frame_info *this_frame, void **this_cache,
> +ia64_frame_this_id (frame_info_ptr this_frame, void **this_cache,
>  		    struct frame_id *this_id)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
> @@ -1901,11 +1901,11 @@ ia64_frame_this_id (struct frame_info *this_frame, void **this_cache,
>  		paddress (gdbarch, this_id->code_addr),
>  		paddress (gdbarch, this_id->stack_addr),
>  		paddress (gdbarch, cache->bsp),
> -		host_address_to_string (this_frame));
> +		host_address_to_string (this_frame.get ()));
>  }
>  
>  static struct value *
> -ia64_frame_prev_register (struct frame_info *this_frame, void **this_cache,
> +ia64_frame_prev_register (frame_info_ptr this_frame, void **this_cache,
>  			  int regnum)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
> @@ -2173,7 +2173,7 @@ static const struct frame_unwind ia64_frame_unwind =
>  /* Signal trampolines.  */
>  
>  static void
> -ia64_sigtramp_frame_init_saved_regs (struct frame_info *this_frame,
> +ia64_sigtramp_frame_init_saved_regs (frame_info_ptr this_frame,
>  				     struct ia64_frame_cache *cache)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
> @@ -2227,7 +2227,7 @@ ia64_sigtramp_frame_init_saved_regs (struct frame_info *this_frame,
>  }
>  
>  static struct ia64_frame_cache *
> -ia64_sigtramp_frame_cache (struct frame_info *this_frame, void **this_cache)
> +ia64_sigtramp_frame_cache (frame_info_ptr this_frame, void **this_cache)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> @@ -2258,7 +2258,7 @@ ia64_sigtramp_frame_cache (struct frame_info *this_frame, void **this_cache)
>  }
>  
>  static void
> -ia64_sigtramp_frame_this_id (struct frame_info *this_frame,
> +ia64_sigtramp_frame_this_id (frame_info_ptr this_frame,
>  			     void **this_cache, struct frame_id *this_id)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
> @@ -2275,11 +2275,11 @@ ia64_sigtramp_frame_this_id (struct frame_info *this_frame,
>  		paddress (gdbarch, this_id->code_addr),
>  		paddress (gdbarch, this_id->stack_addr),
>  		paddress (gdbarch, cache->bsp),
> -		host_address_to_string (this_frame));
> +		host_address_to_string (this_frame.get ()));
>  }
>  
>  static struct value *
> -ia64_sigtramp_frame_prev_register (struct frame_info *this_frame,
> +ia64_sigtramp_frame_prev_register (frame_info_ptr this_frame,
>  				   void **this_cache, int regnum)
>  {
>    struct ia64_frame_cache *cache =
> @@ -2332,7 +2332,7 @@ ia64_sigtramp_frame_prev_register (struct frame_info *this_frame,
>  
>  static int
>  ia64_sigtramp_frame_sniffer (const struct frame_unwind *self,
> -			     struct frame_info *this_frame,
> +			     frame_info_ptr this_frame,
>  			     void **this_cache)
>  {
>    gdbarch *arch = get_frame_arch (this_frame);
> @@ -2362,7 +2362,7 @@ static const struct frame_unwind ia64_sigtramp_frame_unwind =
>  \f
>  
>  static CORE_ADDR
> -ia64_frame_base_address (struct frame_info *this_frame, void **this_cache)
> +ia64_frame_base_address (frame_info_ptr this_frame, void **this_cache)
>  {
>    struct ia64_frame_cache *cache = ia64_frame_cache (this_frame, this_cache);
>  
> @@ -2483,7 +2483,7 @@ ia64_access_reg (unw_addr_space_t as, unw_regnum_t uw_regnum, unw_word_t *val,
>  {
>    int regnum = ia64_uw2gdb_regnum (uw_regnum);
>    unw_word_t bsp, sof, cfm, psr, ip;
> -  struct frame_info *this_frame = (struct frame_info *) arg;
> +  frame_info_ptr this_frame = (frame_info_ptr ) arg;

I'm a little suspicious about this change.  ARG arrives as a void*,
while a frame_info_ptr is larger than a single void*.  I suspect there's
probably a place in ia64-libunwind-tdep.c, maybe in
libunwind_frame_cache where we are trying to pass in a frame_info_ptr
when we probably need to pass a raw frame_info*.

I suspect (but haven't checked) that this code is only compiled on a
native ia64 target.

<snip>

> diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
> index 366f3745b9d..b5a8280b350 100644
> --- a/gdb/python/py-framefilter.c
> +++ b/gdb/python/py-framefilter.c
> @@ -419,7 +419,7 @@ enumerate_args (PyObject *iter,
>  		struct ui_out *out,
>  		enum ext_lang_frame_args args_type,
>  		int print_args_field,
> -		struct frame_info *frame)
> +		frame_info_ptr frame)
>  {
>    struct value_print_options opts;
>  
> @@ -550,7 +550,7 @@ enumerate_locals (PyObject *iter,
>  		  int indent,
>  		  enum ext_lang_frame_args args_type,
>  		  int print_args_field,
> -		  struct frame_info *frame)
> +		  frame_info_ptr frame)
>  {
>    struct value_print_options opts;
>  
> @@ -638,7 +638,7 @@ static enum ext_lang_bt_status
>  py_mi_print_variables (PyObject *filter, struct ui_out *out,
>  		       struct value_print_options *opts,
>  		       enum ext_lang_frame_args args_type,
> -		       struct frame_info *frame)
> +		       frame_info_ptr frame)
>  {
>    gdbpy_ref<> args_iter (get_py_iter_from_func (filter, "frame_args"));
>    if (args_iter == NULL)
> @@ -672,7 +672,7 @@ py_print_locals (PyObject *filter,
>  		 struct ui_out *out,
>  		 enum ext_lang_frame_args args_type,
>  		 int indent,
> -		 struct frame_info *frame)
> +		 frame_info_ptr frame)
>  {
>    gdbpy_ref<> locals_iter (get_py_iter_from_func (filter, "frame_locals"));
>    if (locals_iter == NULL)
> @@ -697,7 +697,7 @@ static enum ext_lang_bt_status
>  py_print_args (PyObject *filter,
>  	       struct ui_out *out,
>  	       enum ext_lang_frame_args args_type,
> -	       struct frame_info *frame)
> +	       frame_info_ptr frame)
>  {
>    gdbpy_ref<> args_iter (get_py_iter_from_func (filter, "frame_args"));
>    if (args_iter == NULL)
> @@ -754,7 +754,7 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
>    int has_addr = 0;
>    CORE_ADDR address = 0;
>    struct gdbarch *gdbarch = NULL;
> -  struct frame_info *frame = NULL;
> +  frame_info_ptr frame = NULL;
>    struct value_print_options opts;
>  
>    int print_level, print_frame_info, print_args, print_locals;
> @@ -859,11 +859,11 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
>        && (location_print
>  	  || (out->is_mi_like_p () && (print_frame_info || print_args))))
>      {
> -      struct frame_info **slot;
> +      frame_info_ptr *slot;
>        int level;
>  
> -      slot = (struct frame_info **) htab_find_slot (levels_printed,
> -						    frame, INSERT);
> +      slot = (frame_info_ptr *) htab_find_slot (levels_printed,
> +						frame.get(), INSERT);

This doesn't look right, and is causing test failures for
gdb.python/py-framefilter-mi.exp.

As you use 'frame.get ()' in the updated htab_find_slot call, which is
passing the raw frame_info*, effectively, the htab_find_slot call hasn't
changed with this patch.  That you are now casting the return type to a
different type indicates the bug.

You might be tempted to pass '&frame' to htab_find_slot, but don't do
that, the frame is a local variable, so hashing based on its address is
not going to work.

I think that levels_printed really does need to hold the raw
frame_info*.

Thanks,
Andrew