Re: [PATCH] Allow struct 'return' on 32-bit sparc.

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> Date: Fri, 01 Feb 2013 16:13:00 -0500 (EST)
> From: David Miller <davem@davemloft.net>
> 
> If gdb cannot write the return value, it pops the stackframe only.
> What this means is that the return value we end up with is going to
> be essentially random.

It does warn you though.

> For example, if the calling convention is like 32-bit sparc, a fixed
> stack frame slot is used for these return values.  Whatever happened
> to be there before the function call is the value that we will end up
> with.
> 
> Taking a step back I went to look at why gdb can't override the return
> value in this situation in the first place.
> 
> Unlike other targets, we can actually properly override the return
> value on 32-bit sparc because it is placed in a fixed location offset
> from the stack pointer.
> 
> On other targets, the function is passed the memory area address as
> an argument and returns that address as a return value.  When gdb
> pops the stack for the return command, this address return value
> won't be set and therefore gdb doesn't have access to the location.
> 
> It looks like the various RETURN_VALUE_* cases were split up
> specifically with this exact distinction in mind.

Yup.  Not sure why I never finished it though.

> So I changed it such that return_command will perform a successful
> return value override not just for RETURN_VALUE_REGISTER_CONVENTION,
> but for RETURN_VALUE_ABI_PRESERVES_ADDRESS as well.
> 
> Then I converted the one target using RETURN_VALUE_ABI_PRESERVES_ADDRESS
> to support writing a value in this case.
> 
> Any objections?

The sparc-tdep.c bits are correct as far as I can see.

I think it would be better if your new function would still have
"struct_return" in its name.  My suggestion would be
"struct_return_convention".

Also,

> diff --git a/gdb/stack.c b/gdb/stack.c
> index c8a6a7e..17950a2 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -2274,6 +2274,7 @@ down_command (char *count_exp, int from_tty)
>  void
>  return_command (char *retval_exp, int from_tty)
>  {
> +  enum return_value_convention rv_conv;
>    struct frame_info *thisframe;
>    struct gdbarch *gdbarch;
>    struct symbol *thisfun;
> @@ -2327,6 +2328,7 @@ return_command (char *retval_exp, int from_tty)
>        if (thisfun != NULL)
>  	function = read_var_value (thisfun, thisframe);
>  
> +      rv_conv = RETURN_VALUE_REGISTER_CONVENTION;
>        if (TYPE_CODE (return_type) == TYPE_CODE_VOID)
>  	/* If the return-type is "void", don't try to find the
>             return-value's location.  However, do still evaluate the
> @@ -2334,14 +2336,18 @@ return_command (char *retval_exp, int from_tty)
>             is discarded, side effects such as "return i++" still
>             occur.  */
>  	return_value = NULL;
> -      else if (thisfun != NULL
> -	       && using_struct_return (gdbarch, function, return_type))
> +      else if (thisfun != NULL)
>  	{
> -	  query_prefix = "The location at which to store the "
> -	    "function's return value is unknown.\n"
> -	    "If you continue, the return value "
> -	    "that you specified will be ignored.\n";
> -	  return_value = NULL;
> +	  rv_conv = convention_for_return (gdbarch, function, return_type);
> +	  if (rv_conv == RETURN_VALUE_STRUCT_CONVENTION
> +	      || rv_conv == RETURN_VALUE_ABI_RETURNS_ADDRESS)
> +	    {
> +	      query_prefix = "The location at which to store the "
> +		"function's return value is unknown.\n"
> +		"If you continue, the return value "
> +		"that you specified will be ignored.\n";
> +	      return_value = NULL;
> +	    }
>  	}
>      }
>  
> @@ -2371,9 +2377,8 @@ return_command (char *retval_exp, int from_tty)
>        struct type *return_type = value_type (return_value);
>        struct gdbarch *gdbarch = get_regcache_arch (get_current_regcache ());
>  
> -      gdb_assert (gdbarch_return_value (gdbarch, function, return_type, NULL,
> -					NULL, NULL)
> -		  == RETURN_VALUE_REGISTER_CONVENTION);
> +      gdb_assert (rv_conv != RETURN_VALUE_STRUCT_CONVENTION
> +		  && rv_conv != RETURN_VALUE_ABI_RETURNS_ADDRESS);
>        gdbarch_return_value (gdbarch, function, return_type,
>  			    get_current_regcache (), NULL /*read*/,
>  			    value_contents (return_value) /*write*/);
> diff --git a/gdb/value.c b/gdb/value.c
> index dbf1c37..07b7dbf 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -3323,13 +3323,12 @@ coerce_array (struct value *arg)
>  }
>  \f
>  
> -/* Return true if the function returning the specified type is using
> -   the convention of returning structures in memory (passing in the
> -   address as a hidden first parameter).  */
> +/* Return the return value convention that will be used for the
> +   specified type.  */
>  
> -int
> -using_struct_return (struct gdbarch *gdbarch,
> -		     struct value *function, struct type *value_type)
> +enum return_value_convention
> +convention_for_return (struct gdbarch *gdbarch,
> +		       struct value *function, struct type *value_type)
>  {
>    enum type_code code = TYPE_CODE (value_type);
>  
> @@ -3339,11 +3338,22 @@ using_struct_return (struct gdbarch *gdbarch,
>    if (code == TYPE_CODE_VOID)
>      /* A void return value is never in memory.  See also corresponding
>         code in "print_return_value".  */
> -    return 0;
> +    return RETURN_VALUE_REGISTER_CONVENTION;

Return RETURN_VALUE_REGISTER_CONVENTION if there is nothing to return
seems to be a bit odd.  But since convention_for_return() is never
called with code being TYPE_CODE (there is an explicit check for that
in stack.c:return_command()) you could simply leave the check in
using_struct_return().