This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Allow struct 'return' on 32-bit sparc.
- From: Mark Kettenis <mark dot kettenis at xs4all dot nl>
- To: davem at davemloft dot net
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 5 Feb 2013 00:57:15 +0100 (CET)
- Subject: Re: [PATCH] Allow struct 'return' on 32-bit sparc.
- References: <20130201.161300.1158114789368969492.davem@davemloft.net>
> 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)
> }
>
>
> -/* 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().