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

[Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>]

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).

> +
> +      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://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_NULL_And_Zero

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
> 

-- 
Lancelot SIX