[patch rfc] generic POP_FRAME

[Mark Kettenis]

Andrew Cagney <ac131313@redhat.com> writes:

> I'll look to commit this in a few days.
> 
> 2003-03-10  Andrew Cagney  <cagney@redhat.com>
> 
> 	Eliminate the need for POP_FRAME.
> 	* frame-unwind.h (frame_unwind_pop_ftype): Delete definition.
> 	(struct frame_unwind): Update.
> 	* frame.c (do_frame_unwind_register): New function.
> 	(frame_pop): When no POP_FRAME, pop the frame using register
> 	unwind and a scratch regcache.
> 	(frame_saved_regs_pop): Delete function.
> 	(trad_frame_unwinder): Update.
> 	* d10v-tdep.c (d10v_frame_pop): Delete function.
> 	(d10v_frame_unwind): Update.
> 	* sentinel-frame.c (sentinel_frame_pop): Delete function.
> 	(sentinel_frame_unwinder): Update.
> 	* dummy-frame.c (dummy_frame_pop): Delete function.
> 	(dummy_frame_unwind): Update.

Andrew,

This patch has a serious problem:
> Index: frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/frame.c,v
> retrieving revision 1.75
> diff -u -r1.75 frame.c
> --- frame.c	10 Mar 2003 15:28:40 -0000	1.75
> +++ frame.c	10 Mar 2003 20:08:38 -0000
> @@ -185,16 +185,44 @@
>    return this_frame->pc_unwind_cache;
>  }
>  
> +static int
> +do_frame_unwind_register (void *src, int regnum, void *buf)
> +{
> +  frame_unwind_register (src, regnum, buf);
> +  return 1;
> +}
> +
>  void
>  frame_pop (struct frame_info *frame)
>  {
> -  /* FIXME: cagney/2003-01-18: There is probably a chicken-egg problem
> -     with passing in current_regcache.  The pop function needs to be
> -     written carefully so as to not overwrite registers whose [old]
> -     values are needed to restore other registers.  Instead, this code
> -     should pass in a scratch cache and, as a second step, restore the
> -     registers using that.  */
> -  frame->unwind->pop (frame, &frame->unwind_cache, current_regcache);
> +  struct regcache *scratch_regcache;
> +  struct cleanup *cleanups;
> +
> +  if (POP_FRAME_P ())
> +    {
> +      /* A legacy architecture that has implemented a custom pop
> +	 function.  All new architectures should instead be using the
> +	 generic code below.  */
> +      POP_FRAME;
> +    }
> +  else
> +    {
> +      /* Make a copy of all the register values unwound from this
> +	 frame.  Save them in a scratch buffer so that there isn't a
> +	 race betweening trying to extract the old values from the
> +	 current_regcache while, at the same time writing new values
> +	 into that same cache.  */
> +      struct regcache *scratch = regcache_xmalloc (current_gdbarch);
> +      struct cleanup *cleanups = make_cleanup_regcache_xfree (scratch);
> +      regcache_save (scratch, do_frame_unwind_register, frame);
> +      /* Now copy those saved registers into the current regcache.
> +         Here, regcache_cpy() calls regcache_restore().  */
> +      regcache_cpy (current_regcache, scratch);
> +      do_cleanups (cleanups);
> +    }
> +  /* We've made right mess of GDB's local state, just discard
> +     everything.  */
> +  target_store_registers (-1);
>    flush_cached_frames ();
>  }

The call to target_store_registers you introduced here, writes back
all registers, including those registers that weren't in the register
cache at all.  As a result, on the i386, this typically makes us write
bogus values into the floating-point registers.  This makes several
tests in gdb.base/return2.exp fail on my i386-unknown-freebsd4.7
system, since executing a floating-point instruction with the thrashed
floating-point state generates a SIGFPE.  I'd expect to see these same
failures on i686-pc-linux-gnu, but I assume that was the target you
tested on.  Therefore I suspect that the Linux kernel doesn't let you
write to the floating-point registers since the floating-point state
wasn't initialized yet[1].

Moving the call to target_store_registers() call into the
!POP_FRAME_P() block solves the problem.  Do you agree that's the
right approach?

Mark

[1] Initialization of the floating-point state usually happens in a
"lazy" fashion on most i386 OS'es, i.e. upon execution of the first
floating-point rinstruction.