Re: [PATCH,V2 2/5] CTF: set up debug info for function arguments

[Wei-min Pan <weimin.pan@oracle.com>]

On 3/2/2021 4:23 AM, Lancelot SIX wrote:
> Hi,
>
> I just have style related comments bellow.
>
> Le Mon, Mar 01, 2021 at 08:53:35PM -0500, Weimin Pan via Gdb-patches a écrit :
>> Added this support in read_func_kind_type after gcc started generating
>> CTF for function arguments.
>>
>> Expanded gdb.base/ctf-ptype.exp to test function arguments. Also fixed
>> some typos.
>>
>> ---
>>   gdb/ChangeLog                        |  4 ++++
>>   gdb/ctfread.c                        | 29 ++++++++++++++++++++++++++++-
>>   gdb/testsuite/ChangeLog              |  4 ++++
>>   gdb/testsuite/gdb.base/ctf-ptype.exp | 19 ++++++++++++++++---
>>   4 files changed, 52 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 88278d1..0a839ba 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,5 +1,9 @@
>>   2021-02-26  Weimin Pan  <weimin.pan@oracle.com>
>>   
>> +	* ctfread.c (read_func_kind_type): Set up function arguments.
>> +
>> +2021-02-26  Weimin Pan  <weimin.pan@oracle.com>
>> +
>>   	* ctfread.c (new_symbol): Set function address.
>>   	(read_func_kind_type): Remove incorrect type name setting.
>>   
>> diff --git a/gdb/ctfread.c b/gdb/ctfread.c
>> index 5a68d9c..75b098c 100644
>> --- a/gdb/ctfread.c
>> +++ b/gdb/ctfread.c
>> @@ -652,8 +652,9 @@ struct ctf_tid_and_type
>>   {
>>     struct objfile *of = ccp->of;
>>     ctf_dict_t *fp = ccp->fp;
>> -  struct type *type, *rettype;
>> +  struct type *type, *rettype, *atype;
>>     ctf_funcinfo_t cfi;
>> +  uint32_t argc;
>>   
>>     type = alloc_type (of);
>>   
>> @@ -663,6 +664,32 @@ struct ctf_tid_and_type
>>     TYPE_TARGET_TYPE (type) = rettype;
>>     set_type_align (type, ctf_type_align (fp, tid));
>>   
>> +  /* Set up function's arguments.  */
>> +  argc = cfi.ctc_argc;
>> +  type->set_num_fields (argc);
>> +  if (cfi.ctc_flags & CTF_FUNC_VARARG)
>> +    type->set_has_varargs (true);
>> +
>> +  if (argc != 0)
>> +    {
>> +      std::vector<ctf_id_t> argv (argc);
>> +      if (ctf_func_type_args (fp, tid, argc, argv.data ()) == CTF_ERR)
>> +	return NULL;
> Could be nullptr instead of NULL (even if I am not sure the standard
> enforces that).

nullptr of std::nullptr_t, which is implicitly converted to null pointer 
value of any pointer type,
seems more appropriate. Will do the necessary changes in other places too.

>
>> +
>> +      type->set_fields
>> +	((struct field *) TYPE_ZALLOC (type, argc * sizeof (struct field)));
>> +      struct type *void_type = objfile_type (of)->builtin_void;
>> +      /* If failed to find the argument type, fill it with void_type.  */
>> +      for (int iparam = 0; iparam < argc; iparam++)
>> +	{
>> +	  atype = get_tid_type (of, argv[iparam]);
>> +	  if (atype)
> An explicit comparison against nullptr should be done here. See
> https://urldefense.com/v3/__https://sourceware.org/gdb/wiki/Internals*20GDB-C-Coding-Standards*Comparison_With_NULL_And_Zero__;JSM!!GqivPVa7Brio!IXhIALJgGrsPK1dDbwNKp69KxLTd4Msy6ShNy9Euo18QeN2Iz4dEBg2rhM_3eUmx$

OK, will do.

Thank you for your comments.

> Thanks,
> Lancelot.
>
>> +	    type->field (iparam).set_type (atype);
>> +	  else
>> +	    type->field (iparam).set_type (void_type);
>> +	}
>> +    }
>> +
>>     return set_tid_type (of, tid, type);
>>   }
>>   
>> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
>> index 3d71354..9445362 100644
>> --- a/gdb/testsuite/ChangeLog
>> +++ b/gdb/testsuite/ChangeLog
>> @@ -1,5 +1,9 @@
>>   2021-02-26  Weimin Pan  <weimin.pan@oracle.com>
>>   
>> +	* gdb.base/ctf-ptype.exp: Add function tests and fix typos.
>> +
>> +2021-02-26  Weimin Pan  <weimin.pan@oracle.com>
>> +
>>   	* gdb.base/ctf-funcreturn.exp: New file.
>>   
>>   2021-01-29  Tom de Vries  <tdevries@suse.de>
>> diff --git a/gdb/testsuite/gdb.base/ctf-ptype.exp b/gdb/testsuite/gdb.base/ctf-ptype.exp
>> index ffe40f1..056f712 100644
>> --- a/gdb/testsuite/gdb.base/ctf-ptype.exp
>> +++ b/gdb/testsuite/gdb.base/ctf-ptype.exp
>> @@ -63,10 +63,10 @@ gdb_test "ptype struct t_struct" "type = struct t_struct \{.*\[\r\n\]    (unsign
>>   
>>   # Test the equivalence between '.' and '->' for struct member references.
>>   
>> -if [gdb_test "ptype v_t_struct_p.v_float_member"	"type = float"]<0 then {
>> +if [gdb_test "ptype v_struct1.v_float_member"	"type = float"]<0 then {
>>       return -1
>>   }
>> -if [gdb_test "ptype v_t_struct_p->v_float_member"	"type = float"]<0 then {
>> +if [gdb_test "ptype v_struct1->v_float_member"	"type = float"]<0 then {
>>       return -1
>>   }
>>   if [gdb_test "ptype v_t_struct_p.v_float_member"	"type = float"]<0 then {
>> @@ -211,7 +211,7 @@ gdb_test "ptype the_highest" \
>>   
>>   gdb_test "ptype the_highest.anonymous_level_1" \
>>            "type = struct \{.*\[\r\n\] *int b;.*\[\r\n\] *struct \{.*\[\r\n\] *int c;.*\[\r\n\] *\} anonymous_level_2;.*\[\r\n\]}.*" \
>> -         "ptype the_highest"
>> +         "ptype the_highest.anonymous_level_1"
>>   
>>   # Print the type of the identifier ID, and check the response:
>>   # - Expect to see PROTOTYPED as the type.  PROTOTYPED is not a regular
>> @@ -255,8 +255,21 @@ proc ptype_maybe_prototyped { id prototyped plain { overprototyped "NO-MATCH" }
>>       }
>>   }
>>   
>> +ptype_maybe_prototyped "func_type" "int (*)(int (*)(int, float), float)" \
>> +                                   "int (*)()"
>>   ptype_maybe_prototyped "old_fptr" "double (*)()" "double (*)()" \
>>                                     "double (*)(void)"
>> +ptype_maybe_prototyped "new_fptr" "double (*)()" "double (*)()"
>> +ptype_maybe_prototyped "fptr" "int (*)(int, float)" "int (*)()"
>> +ptype_maybe_prototyped "fptr2" "int *(*)(int (*)(int, float), float)" \
>> +                               "int *(*)()"
>> +ptype_maybe_prototyped "xptr" "int (*)(int (*)(), int (*)(), int)" \
>> +                              "int (*)()" \
>> +                              "int (*)(int (*)(void), int (*)(void), int)"
>> +ptype_maybe_prototyped "ffptr" "int (*(*)(char))(short int)" \
>> +                               "int (*(*)())()"
>> +ptype_maybe_prototyped "fffptr" "int (*(*(*)(char))(short int))(long int)" \
>> +                                "int (*(*(*)())())()"
>>   
>>   # Test printing type of string constants and array constants, but
>>   # requires a running process.  These call malloc, and can take a long
>> -- 
>> 1.8.3.1
>>