[patch/rfc] Rewrite return_command

[Andrew Cagney <ac131313 at redhat dot com>]

Hello,

The attached patch changes the return_command so that it reports the 
fact that it can't handle the specified return value before, rather than 
after, is asked to confirm that the function should return.  This way 
the user can make a better informed decision.  The three outputs below 
illustrate the old vs new behavior:

YESTERDAY:

(gdb) return foo1
Make fun1 return now? (y or n) y
This architecture does not support specifying a struct or union 
return-value.
(gdb)

TODAY:

(gdb) return foo1
Make fun1 return now? (y or n) y
This architecture does not support specifying a struct or union 
return-value.
#0  0x018011a4 in main () at .../structs.c:231
231       L1  = fun1();
(gdb)

TOMORROW:

(gdb) return foo1
The location at which to store the function's return value is unknown.
Make fun1 return now? (y or n) y
#0  0x018011a4 in main () at .../structs.c:231
231       L1  = fun1();
(gdb)

The rewrite also eliminates deprecated frame references and the 
no-longer-called function set_return_value function.

Baring comment, I'll commit this in a day or so.

Andrew
2003-10-20  Andrew Cagney  <cagney@redhat.com>

	* Makefile.in (stack.o): Add $(regcache_h).
 	* stack.c: Include "regcache.h"
 	(return_command): Rewrite.  Use get_frame_id and
 	get_selected_frame.  Eliminate "deprecated_selected_frame".  Warn
 	about unhandled return-values.
 	* value.h (set_return_value): Delete declaration.
 	* values.c (set_return_value): Delete function.
 
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.458
diff -c -r1.458 Makefile.in
*** Makefile.in	18 Oct 2003 18:41:22 -0000	1.458
--- Makefile.in	20 Oct 2003 22:40:35 -0000
***************
*** 2339,2345 ****
  	$(gdbtypes_h) $(expression_h) $(language_h) $(frame_h) $(gdbcmd_h) \
  	$(gdbcore_h) $(target_h) $(source_h) $(breakpoint_h) $(demangle_h) \
  	$(inferior_h) $(annotate_h) $(ui_out_h) $(block_h) $(stack_h) \
! 	$(gdb_assert_h) $(dictionary_h) $(reggroups_h)
  standalone.o: standalone.c $(gdb_stat_h) $(defs_h) $(symtab_h) $(frame_h) \
  	$(inferior_h) $(gdb_wait_h)
  std-regs.o: std-regs.c $(defs_h) $(user_regs_h) $(frame_h) $(gdbtypes_h) \
--- 2339,2345 ----
  	$(gdbtypes_h) $(expression_h) $(language_h) $(frame_h) $(gdbcmd_h) \
  	$(gdbcore_h) $(target_h) $(source_h) $(breakpoint_h) $(demangle_h) \
  	$(inferior_h) $(annotate_h) $(ui_out_h) $(block_h) $(stack_h) \
! 	$(gdb_assert_h) $(dictionary_h) $(reggroups_h) $(regcache_h)
  standalone.o: standalone.c $(gdb_stat_h) $(defs_h) $(symtab_h) $(frame_h) \
  	$(inferior_h) $(gdb_wait_h)
  std-regs.o: std-regs.c $(defs_h) $(user_regs_h) $(frame_h) $(gdbtypes_h) \
Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.90
diff -c -r1.90 stack.c
*** stack.c	2 Oct 2003 20:28:30 -0000	1.90
--- stack.c	20 Oct 2003 22:40:35 -0000
***************
*** 44,49 ****
--- 44,50 ----
  #include "gdb_assert.h"
  #include "dictionary.h"
  #include "reggroups.h"
+ #include "regcache.h"
  
  /* Prototypes for exported functions. */
  
***************
*** 55,62 ****
  
  void _initialize_stack (void);
  
- void return_command (char *, int);
- 
  /* Prototypes for local functions. */
  
  static void down_command (char *, int);
--- 56,61 ----
***************
*** 1819,1912 ****
  return_command (char *retval_exp, int from_tty)
  {
    struct symbol *thisfun;
-   CORE_ADDR selected_frame_addr;
-   CORE_ADDR selected_frame_pc;
-   struct frame_info *frame;
    struct value *return_value = NULL;
  
!   if (deprecated_selected_frame == NULL)
      error ("No selected frame.");
!   thisfun = get_frame_function (deprecated_selected_frame);
!   selected_frame_addr = get_frame_base (deprecated_selected_frame);
!   selected_frame_pc = get_frame_pc (deprecated_selected_frame);
! 
!   /* Compute the return value (if any -- possibly getting errors here).  */
  
    if (retval_exp)
      {
        struct type *return_type = NULL;
  
        return_value = parse_and_eval (retval_exp);
  
!       /* Cast return value to the return type of the function.  */
        if (thisfun != NULL)
  	return_type = TYPE_TARGET_TYPE (SYMBOL_TYPE (thisfun));
        if (return_type == NULL)
  	return_type = builtin_type_int;
        return_value = value_cast (return_type, return_value);
  
!       /* Make sure we have fully evaluated it, since
!          it might live in the stack frame we're about to pop.  */
        if (VALUE_LAZY (return_value))
  	value_fetch_lazy (return_value);
-     }
- 
-   /* If interactive, require confirmation.  */
  
!   if (from_tty)
!     {
!       if (thisfun != 0)
  	{
! 	  if (!query ("Make %s return now? ", SYMBOL_PRINT_NAME (thisfun)))
! 	    {
! 	      error ("Not confirmed.");
! 	      /* NOTREACHED */
! 	    }
  	}
!       else if (!query ("Make selected stack frame return now? "))
! 	error ("Not confirmed.");
      }
  
!   /* FIXME: cagney/2003-01-18: Rather than pop each frame in turn,
!      this code should just go straight to the relevant frame and pop
!      that.  */
! 
!   /* Do the real work.  Pop until the specified frame is current.  We
!      use this method because the deprecated_selected_frame is not
!      valid after a frame_pop().  The pc comparison makes this work
!      even if the selected frame shares its fp with another frame.  */
  
!   /* FIXME: cagney/32003-03-12: This code should use frame_id_eq().
!      Unfortunatly, that function doesn't yet include the PC in any
!      frame ID comparison.  */
! 
!   while (selected_frame_addr != get_frame_base (frame = get_current_frame ())
! 	 || selected_frame_pc != get_frame_pc (frame))
!     frame_pop (get_current_frame ());
! 
!   /* Then pop that frame.  */
  
    frame_pop (get_current_frame ());
  
!   /* Compute the return value (if any) and store in the place
!      for return values.  */
! 
!   if (retval_exp)
!     set_return_value (return_value);
! 
!   /* If we are at the end of a call dummy now, pop the dummy frame too.  */
  
!   /* FIXME: cagney/2003-01-18: This is silly.  Instead of popping all
!      the frames except the dummy, and then, as an afterthought,
!      popping the dummy frame, this code should just pop through to the
!      dummy frame.  */
!   
    if (CALL_DUMMY_HAS_COMPLETED (read_pc(), read_sp (),
  				get_frame_base (get_current_frame ())))
      frame_pop (get_current_frame ());
  
    /* If interactive, print the frame that is now current.  */
- 
    if (from_tty)
      frame_command ("0", 1);
    else
--- 1818,1955 ----
  return_command (char *retval_exp, int from_tty)
  {
    struct symbol *thisfun;
    struct value *return_value = NULL;
+   const char *query_prefix = "";
  
!   /* FIXME: cagney/2003-10-20: Perform a minimal existance test on the
!      target.  If that fails, error out.  For the moment don't rely on
!      get_selected_frame as it's error message is the the singularly
!      obscure "No registers".  */
!   if (!target_has_registers)
      error ("No selected frame.");
!   thisfun = get_frame_function (get_selected_frame ());
  
+   /* Compute the return value.  If the computation triggers an error,
+      let it bail.  If the return type can't be handled, set
+      RETURN_VALUE to NULL, and QUERY_PREFIX to an informational
+      message.  */
    if (retval_exp)
      {
        struct type *return_type = NULL;
  
+       /* Compute the return value.  Should the computation fail, this
+          call throws an error.  */
        return_value = parse_and_eval (retval_exp);
  
!       /* Cast return value to the return type of the function.  Should
!          the cast fail, this call throws an error.  */
        if (thisfun != NULL)
  	return_type = TYPE_TARGET_TYPE (SYMBOL_TYPE (thisfun));
        if (return_type == NULL)
  	return_type = builtin_type_int;
        return_value = value_cast (return_type, return_value);
  
!       /* Make sure the value is fully evaluated.  It may live in the
!          stack frame we're about to pop.  */
        if (VALUE_LAZY (return_value))
  	value_fetch_lazy (return_value);
  
!       /* Check that this architecture can handle the function's return
!          type.  In the case of "struct convention", still do the
!          "return", just also warn the user.  */
!       if (gdbarch_return_value_p (current_gdbarch))
  	{
! 	  if (gdbarch_return_value (current_gdbarch, return_type,
! 				    NULL, NULL, NULL)
! 	      == RETURN_VALUE_REGISTER_CONVENTION)
! 	    return_value = NULL;
  	}
!       else
! 	{
! 	  /* NOTE: cagney/2003-10-20: The double check is to ensure
! 	     that the STORE_RETURN_VALUE call, further down, is not
! 	     applied to a struct or union return-value.  It wasn't
! 	     allowed previously, so don't start allowing it now.  An
! 	     ABI that uses "register convention" to return small
! 	     structures and should implement the "return_value"
! 	     architecture method.  */
! 	  if (using_struct_return (return_type, 0)
! 	      || TYPE_CODE (return_type) == TYPE_CODE_STRUCT
! 	      || TYPE_CODE (return_type) == TYPE_CODE_UNION)
! 	    return_value = NULL;
! 	}
!       if (return_value == NULL)
! 	query_prefix = "\
! The location at which to store the function's return value is unknown.\n";
!     }
! 
!   /* Does an interactive user really want to do this?  Include
!      information, such as how well GDB can handle the return value, in
!      the query message.  */
!   if (from_tty)
!     {
!       int confirmed;
!       if (thisfun == NULL)
! 	confirmed = query ("%sMake selected stack frame return now? ",
! 			   query_prefix);
!       else
! 	confirmed = query ("%sMake %s return now? ", query_prefix,
! 			   SYMBOL_PRINT_NAME (thisfun));
!       if (!confirmed)
! 	error ("Not confirmed");
      }
  
!   /* NOTE: cagney/2003-01-18: Is this silly?  Rather than pop each
!      frame in turn, should this code just go straight to the relevant
!      frame and pop that?  */
  
!   /* First discard all frames inner-to the selected frame (making the
!      selected frame current).  */
!   {
!     struct frame_id selected_id = get_frame_id (get_selected_frame ());
!     while (!frame_id_eq (selected_id, get_frame_id (get_current_frame ())))
!       {
! 	if (frame_id_inner (selected_id, get_frame_id (get_current_frame ())))
! 	  /* Caught in the safety net, oops!  We've gone way past the
!              selected frame.  */
! 	  error ("Problem while popping stack frames (corrupt stack?)");
! 	frame_pop (get_current_frame ());
!       }
!   }
  
+   /* Second discard the selected frame (which is now also the current
+      frame).  */
    frame_pop (get_current_frame ());
  
!   /* Store RETURN_VAUE in the just-returned register set.  */
!   if (return_value != NULL)
!     {
!       struct type *return_type = VALUE_TYPE (return_value);
!       if (!gdbarch_return_value_p (current_gdbarch))
! 	{
! 	  STORE_RETURN_VALUE (return_type, current_regcache,
! 			      VALUE_CONTENTS (return_value));
! 	}
!       else
! 	{
! 	  gdb_assert (gdbarch_return_value (current_gdbarch, return_type,
! 					    NULL, NULL, NULL)
! 		      == RETURN_VALUE_REGISTER_CONVENTION);
! 	  gdbarch_return_value (current_gdbarch, return_type, current_regcache,
! 				VALUE_CONTENTS (return_value), NULL);
! 	}
!     }
  
!   /* If we are at the end of a call dummy now, pop the dummy frame
!      too.  */
!   /* NOTE: cagney/2003-01-18: Is this silly?  Instead of popping all
!      the frames in sequence, should this code just pop the dummy frame
!      directly?  */
    if (CALL_DUMMY_HAS_COMPLETED (read_pc(), read_sp (),
  				get_frame_base (get_current_frame ())))
      frame_pop (get_current_frame ());
  
    /* If interactive, print the frame that is now current.  */
    if (from_tty)
      frame_command ("0", 1);
    else
Index: value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.53
diff -c -r1.53 value.h
*** value.h	2 Oct 2003 04:40:58 -0000	1.53
--- value.h	20 Oct 2003 22:40:35 -0000
***************
*** 425,432 ****
  
  extern int using_struct_return (struct type *value_type, int gcc_p);
  
- extern void set_return_value (struct value *val);
- 
  extern struct value *evaluate_expression (struct expression *exp);
  
  extern struct value *evaluate_type (struct expression *exp);
--- 425,430 ----
Index: values.c
===================================================================
RCS file: /cvs/src/src/gdb/values.c,v
retrieving revision 1.60
diff -c -r1.60 values.c
*** values.c	20 Oct 2003 15:38:02 -0000	1.60
--- values.c	20 Oct 2003 22:40:36 -0000
***************
*** 1306,1363 ****
  	  == RETURN_VALUE_STRUCT_CONVENTION);
  }
  
- /* Store VAL so it will be returned if a function returns now.
-    Does not verify that VAL's type matches what the current
-    function wants to return.  */
- 
- void
- set_return_value (struct value *val)
- {
-   struct type *type = check_typedef (VALUE_TYPE (val));
-   enum type_code code = TYPE_CODE (type);
- 
-   if (code == TYPE_CODE_ERROR)
-     error ("Function return type unknown.");
- 
-   if (gdbarch_return_value_p (current_gdbarch))
-     {
-       switch (gdbarch_return_value (current_gdbarch, type, NULL, NULL, NULL))
- 	{
- 	case RETURN_VALUE_REGISTER_CONVENTION:
- 	  /* Success.  The architecture can deal with it, write it to
-              the regcache.  */
- 	  gdbarch_return_value (current_gdbarch, type, current_regcache,
- 				VALUE_CONTENTS (val), NULL);
- 	  return;
- 	case RETURN_VALUE_STRUCT_CONVENTION:
- 	  /* Failure.  For the moment, assume that it is not possible
-              to find the location, on the stack, at which the "struct
-              return" value should be stored.  Only a warning because
-              an error aborts the "return" command leaving GDB in a
-              weird state.  */
- 	  warning ("Location of return value unknown");
- 	  return;
- 	}
-     }
- 
- 
-   if (code == TYPE_CODE_STRUCT
-       || code == TYPE_CODE_UNION)	/* FIXME, implement struct return.  */
-     /* FIXME: cagney/2003-10-20: This should be an internal-warning.
-        The problem is that while GDB's core supports "struct return"
-        using "register convention", many architectures haven't been
-        updated to implement the mechanisms needed to make it work.
-        It's a warning, and not an error, as otherwize it will jump out
-        of the "return" command leaving both GDB and the user in a very
-        confused state.  */
-     {
-       warning ("This architecture does not support specifying a struct or union return-value.");
-       return;
-     }
- 
-   STORE_RETURN_VALUE (type, current_regcache, VALUE_CONTENTS (val));
- }
- 
  void
  _initialize_values (void)
  {
--- 1306,1311 ----