Re: [patch] Fix `return' of long/long-long results with no debuginfo

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

> Date: Wed, 11 Feb 2009 22:46:47 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>

Sorry Jan, are you really just ignoring my remarks and actually making
this diff more unacceptable to me?

> On Wed, 11 Feb 2009 22:22:41 +0100, Mark Kettenis wrote:
> > And I have a (somewhat) vague recollection that there are debug formats or
> > compilers where we cant distinguish between functions without a return type
> > and functions with "unknown" return type.
> 
> As `struct type' for a function type GDB debuginfo readers always create by
> make_function_type and this function crashes on NULL `type' we can assume
> TYPE_TARGET_TYPE is never NULL.
> 
> For DWARF it is clear both according to the standard
> 3.3.2 Subroutine and Entry Point Return Types
> If the subroutine or entry point is a function that returns a value, then its
> debugging information entry has a DW_AT_type attribute [...]
> 
> and according to the code:
> dwarf2read.c:die_type():
>       /* A missing DW_AT_type represents a void type.  */
> 
> 
> Made the condition in the patch more explicit now.  Removed the original
> TYPE_CODE_VOID code emulating the K&R behavior to more follow the modern
> ANSI/ISO C compilers refuse `return value;' in a void-returning function
> without having typeless/`int'-defaulted functions permitted.
> 
> 
> > From: Daniel Jacobowitz <drow@false.org>
> > > Everything we pick will be wrong some of the time, but IMO "long" is
> > > maximally useful.  "long long" on 32-bit platforms is going to pick
> > > up garbage from the next register for int or void * returns.
> 
> Garbage would be seen by caller of a `long long' returning callee where GDB
> would return something smaller (like `int').  As GDB writes (not "reads") the
> register it just will write zero to one excessive but unused register.  This
> register must be callee-saved (not caller-saved) by the ABI supporting `long
> long' return type and GDB already asserts the return type is
> RETURN_VALUE_REGISTER_CONVENTION.
> 
> If GDB would return only the `long' size there would be no way how to do the
> `return' command on a debuginfo-less function natively returning `long long'.
> 
> 
> > The right solution of course is to distribute libraries with debug
> > sumbols.  But in cases where that is impossible, would it be an idea
> > to use the type of the return value expression given by the user
> > instead of int as a fallback?
> 
> Any return value gets retyped to `long long' which is intepreted by the caller
> even as lower-sized types (not guaranteed by ABI but at lest checked now by
> new gdb.base/return-nodebug.exp).
> 
> 
> > In the case at hand, "return -1" will still fail, but it would be
> > possible to do return (void *)-1 to do what the user wanted.
> 
> `return -1' is checked by gdb.base/return-nodebug.exp and it works.
> 
> 
> Thanks,
> Jan
> 
> 
> gdb/
> 2009-02-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* stack.c (return_command): Default to the builtin_long_long or at
> 	least builtin_long types now.  Assert TYPE_TARGET_TYPE of a typed
> 	function is non-NULL.  Error on return value for void returning
> 	functions.
> 
> gdb/testsuite/
> 2009-02-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/return-nodebug.exp, gdb.base/return-nodebug.c: New.
> 	* gdb.base/return2.exp (try to return value in a void function): New.
> 
> --- gdb/stack.c	11 Feb 2009 16:07:28 -0000	1.185
> +++ gdb/stack.c	11 Feb 2009 21:39:57 -0000
> @@ -1805,9 +1805,23 @@ return_command (char *retval_exp, int fr
>        /* 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 (get_frame_arch (thisframe))->builtin_int;
> +      	{
> +	  return_type = TYPE_TARGET_TYPE (SYMBOL_TYPE (thisfun));
> +	  gdb_assert (return_type != NULL);
> +	  if (TYPE_CODE (return_type) == TYPE_CODE_VOID)
> +	    error (_("Function %s does not return any value"),
> +		   SYMBOL_PRINT_NAME (thisfun));
> +	}
> +      else
> +        {
> +	  return_type
> +	    = builtin_type (get_frame_arch (thisframe))->builtin_long_long;
> +
> +	  /* Hopefull each ABI returns at least `long' type in register.  */
> +	  if (using_struct_return (NULL, return_type))
> +	    return_type
> +	      = builtin_type (get_frame_arch (thisframe))->builtin_long;
> +	}
>        CHECK_TYPEDEF (return_type);
>        return_value = value_cast (return_type, return_value);
>  
> @@ -1816,14 +1830,7 @@ return_command (char *retval_exp, int fr
>        if (value_lazy (return_value))
>  	value_fetch_lazy (return_value);
>  
> -      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
> -           return expression so that, even when the expression result
> -           is discarded, side effects such as "return i++" still
> -           occur.  */
> -	return_value = NULL;
> -      else if (thisfun != NULL
> +      if (thisfun != NULL
>  	       && using_struct_return (SYMBOL_TYPE (thisfun), return_type))
>  	{
>  	  query_prefix = "\
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/return-nodebug.c	11 Feb 2009 21:40:00 -0000
> @@ -0,0 +1,40 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2009 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +
> +static TYPE
> +func (void)
> +{
> +  return (TYPE) -1;
> +}
> +
> +static void
> +marker (void)
> +{
> +}
> +
> +int
> +main (void)
> +{
> +  printf ("result=" FORMAT "\n", CAST func ());
> +
> +  /* Cannot `next' with no debug info.  */
> +  marker ();
> +
> +  return 0;
> +}
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/return-nodebug.exp	11 Feb 2009 21:40:00 -0000
> @@ -0,0 +1,52 @@
> +# Copyright (C) 2009 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +proc do_test {typenospace} {
> +    global pf_prefix
> +    set old_prefix $pf_prefix
> +    lappend pf_prefix "$typenospace:"
> +
> +    if {[runto "func"]} {
> +	# Test return from a function with no debug info (symbol; still it may
> +	# have a minimal-symbol) if it does not crash.
> +	gdb_test "return -1" "#0 .* main \\(.*"			\
> +		 "return from function with no debug info"	\
> +		 "Make selected stack frame return now\\? \\(y or n\\) " "y"
> +
> +	# And if it returned the full width of the result.
> +	gdb_test "adv marker" "\r\nresult=-1\r\n.* in marker \\(.*" \
> +		 "full width of the returned result"
> +    }
> +
> +    set pf_prefix $old_prefix
> +}
> +
> +foreach case {{{char}      %d (int)}	\
> +	      {{short}     %d (int)}	\
> +	      {{int}       %d}		\
> +	      {{long}      %ld}		\
> +	      {{long long} %lld}} {
> +    set type [lindex $case 0]
> +    set format [lindex $case 1]
> +    set cast [lindex $case 2]
> +
> +    set typeesc [string map {{ } {\ }} $type]
> +    set typenospace [string map {{ } -} $type]
> +
> +    if {[prepare_for_testing return-nodebug.exp "return-nodebug-$typenospace" "return-nodebug.c" \
> +	 [list "additional_flags=-DFORMAT=\"$format\" -DTYPE=$typeesc -DCAST=$cast"]] == 0} {
> +	do_test $typenospace
> +    }
> +}
> --- gdb/testsuite/gdb.base/return2.exp	3 Jan 2009 05:58:03 -0000	1.11
> +++ gdb/testsuite/gdb.base/return2.exp	11 Feb 2009 21:40:00 -0000
> @@ -70,6 +70,8 @@ proc return_void { } {
>  	    "set break on void_func"
>      gdb_test "continue" "Breakpoint.* void_func.*" \
>  	    "continue to void_func"
> +    gdb_test "return 23" "Function void_func does not return any value" \
> +	     "try to return value in a void function"
>      send_gdb "return \n"
>      gdb_expect {
>  	-re "Make void_func return now.*y or n. $" {
>