[PATCH] Fix print of value type in a corner case of finish

[PATCH] Fix print of value type in a corner case of finish

[Antoine Tremblay]

When doing finish in a function, if gdb fails to return a value, gdb
also fails at printing the value type if this type is a struct.

For example :

(gdb) fin
....
Value returned has type: . Cannot determine contents

This patch fixes this by calling type_to_string to print the type
so that we can support these types.

This patch returns the following example output :

(gdb) fin
....
Value returned has type: struct test. Cannot determine contents

gdb/ChangeLog:
	* gdb/infcmd.c (print_return_value): use type_to_string to print type.
---
 gdb/infcmd.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9a1fb8d..3737b8f 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1607,10 +1607,16 @@ print_return_value (struct value *function, struct type *value_type)
     }
   else
     {
+      struct cleanup *oldchain;
+      char *type_name;
+
+      type_name = type_to_string (value_type);
+      oldchain = make_cleanup (xfree, type_name);
       ui_out_text (uiout, "Value returned has type: ");
-      ui_out_field_string (uiout, "return-type", TYPE_NAME (value_type));
+      ui_out_field_string (uiout, "return-type", type_name);
       ui_out_text (uiout, ".");
       ui_out_text (uiout, " Cannot determine contents\n");
+      do_cleanups (oldchain);
     }
 }
 
-- 
1.7.9.5

Re: [PATCH] Fix print of value type in a corner case of finish

[Pedro Alves]

It there a way to add a test?

Also please use "const char *" for type_name, so C++
doesn't complain.  :-)

Thanks,
Pedro Alves

On 02/26/2015 01:25 PM, Antoine Tremblay wrote:
> When doing finish in a function, if gdb fails to return a value, gdb
> also fails at printing the value type if this type is a struct.
> 
> For example :
> 
> (gdb) fin
> ....
> Value returned has type: . Cannot determine contents
> 
> This patch fixes this by calling type_to_string to print the type
> so that we can support these types.
> 
> This patch returns the following example output :
> 
> (gdb) fin
> ....
> Value returned has type: struct test. Cannot determine contents
> 
> gdb/ChangeLog:
> 	* gdb/infcmd.c (print_return_value): use type_to_string to print type.
> ---
>  gdb/infcmd.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 9a1fb8d..3737b8f 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1607,10 +1607,16 @@ print_return_value (struct value *function, struct type *value_type)
>      }
>    else
>      {
> +      struct cleanup *oldchain;
> +      char *type_name;
> +
> +      type_name = type_to_string (value_type);
> +      oldchain = make_cleanup (xfree, type_name);
>        ui_out_text (uiout, "Value returned has type: ");
> -      ui_out_field_string (uiout, "return-type", TYPE_NAME (value_type));
> +      ui_out_field_string (uiout, "return-type", type_name);
>        ui_out_text (uiout, ".");
>        ui_out_text (uiout, " Cannot determine contents\n");
> +      do_cleanups (oldchain);
>      }
>  }
>  
> 

Re: [PATCH] Fix print of value type in a corner case of finish

[Antoine Tremblay]

On 02/26/2015 08:35 AM, Pedro Alves wrote:
> It there a way to add a test?
I would need to add a test that's specific to an architecture that does 
not support the struct return value...
I'll see what I can figure out..

> Also please use "const char *" for type_name, so C++
> doesn't complain.  :-)
Humm not sure I understand here since type_to_string returns a char* 
that needs to be freed ?

Converting to const char* would make make_cleanup complain...

How is C++ complaining ?

> Thanks,
> Pedro Alves
>
> On 02/26/2015 01:25 PM, Antoine Tremblay wrote:
>> When doing finish in a function, if gdb fails to return a value, gdb
>> also fails at printing the value type if this type is a struct.
>>
>> For example :
>>
>> (gdb) fin
>> ....
>> Value returned has type: . Cannot determine contents
>>
>> This patch fixes this by calling type_to_string to print the type
>> so that we can support these types.
>>
>> This patch returns the following example output :
>>
>> (gdb) fin
>> ....
>> Value returned has type: struct test. Cannot determine contents
>>
>> gdb/ChangeLog:
>> 	* gdb/infcmd.c (print_return_value): use type_to_string to print type.
>> ---
>>   gdb/infcmd.c |    8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index 9a1fb8d..3737b8f 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -1607,10 +1607,16 @@ print_return_value (struct value *function, struct type *value_type)
>>       }
>>     else
>>       {
>> +      struct cleanup *oldchain;
>> +      char *type_name;
>> +
>> +      type_name = type_to_string (value_type);
>> +      oldchain = make_cleanup (xfree, type_name);
>>         ui_out_text (uiout, "Value returned has type: ");
>> -      ui_out_field_string (uiout, "return-type", TYPE_NAME (value_type));
>> +      ui_out_field_string (uiout, "return-type", type_name);
>>         ui_out_text (uiout, ".");
>>         ui_out_text (uiout, " Cannot determine contents\n");
>> +      do_cleanups (oldchain);
>>       }
>>   }
>>   
>>

Re: [PATCH] Fix print of value type in a corner case of finish

[Pedro Alves]

On 02/26/2015 02:11 PM, Antoine Tremblay wrote:
> On 02/26/2015 08:35 AM, Pedro Alves wrote:
>> It there a way to add a test?
> I would need to add a test that's specific to an architecture that does 
> not support the struct return value...

Hmm, AFAICS, it's not that the architecture doesn't support
the struct return value, or some kind of memory read error or
some such.  It's that the ABI returns the object using
struct return convention / RETURN_VALUE_STRUCT_CONVENTION:

/* Return the value of the result of a function at the end of a 'finish'
   command/BP.  */

struct value *
get_return_value (struct value *function, struct type *value_type)
{

...
  switch (gdbarch_return_value (gdbarch, function, value_type,
  				NULL, NULL, NULL))
    {
...
      break;
    case RETURN_VALUE_STRUCT_CONVENTION:
      value = NULL;
      break;

I think that we make the test return a big enough
structure, we'll hit RETURN_VALUE_STRUCT_CONVENTION on most
architectures (but not x86...)?

For archs that don't do RETURN_VALUE_STRUCT_CONVENTION, the
test could cope, by also passing if GDB manages to extract
the real value.  In sum, PASS if real value, or at least type.
FAIL otherwise.

But, ... I'm actually very much surprised we don't test this
already.  So I did

$ grep -rn "\"finish\"" testsuite/

and noticed:
...
gdb.base/structs.exp:259:# "finish") and correctly extract/store any corresponding
gdb.base/structs.exp:276:# is in three parts: test "return"; test "finish"; check that the two
gdb.base/structs.exp:299:    # consistency between this and the "finish" case.
gdb.base/structs.exp:404:    # Check that a "finish" works.
gdb.base/structs.exp:427:    gdb_test_multiple "finish" "${test}" {
gdb.base/structs.exp:467:    # Since "finish" works in more cases than "return" (see
gdb.base/structs.exp:470:    # known implies that the "finish" value is known (but not the
...

And lo:

    set test "finish foo<n>; ${tests}"
    set finish_value_known 1
    gdb_test_multiple "finish" "${test}" {
	-re "Value returned is .*${gdb_prompt} $" {
	    pass "${test}"
	}
	-re "Cannot determine contents.*${gdb_prompt} $" {
	    # Expected bad value.  For the moment this is ok.
	    set finish_value_known 0
	    pass "${test}"
	}
    }


So the regex here is too lax and missed this bug.  Could you
tweak it to make sure some type was output?

> 
>> Also please use "const char *" for type_name, so C++
>> doesn't complain.  :-)
> Humm not sure I understand here since type_to_string returns a char* 
> that needs to be freed ?
> 
> Converting to const char* would make make_cleanup complain...
> 
> How is C++ complaining ?

Oh, nevermind, that was a thinko on my part, sorry.


Thanks,
Pedro Alves

Re: [PATCH] Fix print of value type in a corner case of finish

[Antoine Tremblay]

On 02/26/2015 10:18 AM, Pedro Alves wrote:
> I think that we make the test return a big enough
> structure, we'll hit RETURN_VALUE_STRUCT_CONVENTION on most
> architectures (but not x86...)?
Yes that's what led me to believe I needed something arch specific but 
indeed it may not totally be..
> So the regex here is too lax and missed this bug.  Could you
> tweak it to make sure some type was output?
Wow I had totally missed that , Thanks!!

Patch v2 coming up in a few secs...

In that patch I check that I actually get the right struct type :)
(I tested the patch by simulating the condition and it works fine)

Regards,

Antoine