[PATCH 2/8] infcall, c++: allow more info to be computed for pass-by-reference values

Andrew Burgess andrew.burgess@embecosm.com
Wed May 22 20:17:00 GMT 2019


* Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> [2019-04-23 16:31:48 +0200]:

> In C++, call-by-value arguments that cannot be trivially copied are
> implicitly passed by reference.  When making an infcall, GDB needs to
> find out if an argument is pass-by-reference or not, so that the correct
> semantics can be followed.  This patch enriches the information computed
> by the language ops for pass-by-reference arguments.  Instead of a plain
> binary result, the computed information now includes whether the
> argument is
>   - copy constructible
>   - destructible
>   - trivially copyable
>   - trivially copy constructible
>   - trivially destructible
> and also
>   - the full name of the copy constructor
>   - the full name of the destructor
> in preparation for GDB's infcall mechanism to call the copy ctor and
> the destructor of a pass-by-ref argument appropriately.  This
> information is stored in a struct named 'language_pass_by_ref_info'.
> 
> gdb/ChangeLog:
> 
> 	* language.h (struct language_pass_by_ref_info): New struct.
> 	(struct language_defn)<la_pass_by_reference>: Change the signature
> 	to return a language_pass_by_ref_info instead of an int.
> 	(language_pass_by_reference): Ditto.
> 	(default_pass_by_reference): Ditto.
> 	Adjust the users listed below.
> 	* arch-utils.c (default_return_in_first_hidden_param_p):
> 	Update.
> 	* cp-abi.c (cp_pass_by_reference): Update.
> 	* cp-abi.h (struct cp_abi_ops)<pass_by_reference>: Update.
> 	* gnu-v3-abi.c (gnuv3_pass_by_reference): Update.
> 	* infcall.c (call_function_by_hand_dummy): Update.
> 	* language.c (language_pass_by_reference): Update.
> 	(default_pass_by_reference): Update.
> 	* tic6x-tdep.c (tic6x_return_value): Update.

I have a little feedback given below, but otherwise this looks fine.

> 
> ---
>  gdb/arch-utils.c |  2 +-
>  gdb/cp-abi.c     |  6 ++++--
>  gdb/cp-abi.h     | 10 +++++----
>  gdb/gnu-v3-abi.c | 49 ++++++++++++++++++++++++++++++------------
>  gdb/infcall.c    |  3 ++-
>  gdb/language.c   | 27 +++++++++++++++++-------
>  gdb/language.h   | 55 +++++++++++++++++++++++++++++++++++++++---------
>  gdb/tic6x-tdep.c |  2 +-
>  8 files changed, 114 insertions(+), 40 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 43f5834b383..5ccabde45e3 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -858,7 +858,7 @@ default_return_in_first_hidden_param_p (struct gdbarch *gdbarch,
>    /* Usually, the return value's address is stored the in the "first hidden"
>       parameter if the return value should be passed by reference, as
>       specified in ABI.  */
> -  return language_pass_by_reference (type);
> +  return !(language_pass_by_reference (type).trivially_copyable);
>  }
>  
>  int default_insn_is_call (struct gdbarch *gdbarch, CORE_ADDR addr)
> diff --git a/gdb/cp-abi.c b/gdb/cp-abi.c
> index bbb74d42638..6503b4c160b 100644
> --- a/gdb/cp-abi.c
> +++ b/gdb/cp-abi.c
> @@ -220,11 +220,13 @@ cplus_typename_from_type_info (struct value *value)
>    return (*current_cp_abi.get_typename_from_type_info) (value);
>  }
>  
> -int
> +/* See cp-abi.h.  */
> +
> +struct language_pass_by_ref_info
>  cp_pass_by_reference (struct type *type)
>  {
>    if ((current_cp_abi.pass_by_reference) == NULL)
> -    return 0;
> +    return default_pass_by_reference (type);
>    return (*current_cp_abi.pass_by_reference) (type);
>  }
>  
> diff --git a/gdb/cp-abi.h b/gdb/cp-abi.h
> index 3cbf19cd511..0afba77aeb1 100644
> --- a/gdb/cp-abi.h
> +++ b/gdb/cp-abi.h
> @@ -207,9 +207,11 @@ extern std::string cplus_typename_from_type_info (struct value *value);
>  CORE_ADDR cplus_skip_trampoline (struct frame_info *frame,
>  				 CORE_ADDR stop_pc);
>  
> -/* Return non-zero if an argument of type TYPE should be passed by
> -   reference instead of value.  */
> -extern int cp_pass_by_reference (struct type *type);
> +/* Fill in INFO with information about whether TYPE should be
> +   passed (and returned) by reference at the language level.  */
> +
> +extern struct language_pass_by_ref_info cp_pass_by_reference
> +  (struct type *type);
>  
>  struct cp_abi_ops
>  {
> @@ -246,7 +248,7 @@ struct cp_abi_ops
>    struct type *(*get_type_from_type_info) (struct value *value);
>    std::string (*get_typename_from_type_info) (struct value *value);
>    CORE_ADDR (*skip_trampoline) (struct frame_info *, CORE_ADDR);
> -  int (*pass_by_reference) (struct type *type);
> +  struct language_pass_by_ref_info (*pass_by_reference) (struct type *type);
>  };
>  
>  
> diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
> index 6407c9beb82..b39e5a5c585 100644
> --- a/gdb/gnu-v3-abi.c
> +++ b/gdb/gnu-v3-abi.c
> @@ -1228,7 +1228,7 @@ gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)
>    return real_stop_pc;
>  }
>  
> -/* Return nonzero if a type should be passed by reference.
> +/* Return pass-by-reference information for the given TYPE.
>  
>     The rule in the v3 ABI document comes from section 3.1.1.  If the
>     type has a non-trivial copy constructor or destructor, then the
> @@ -1246,22 +1246,33 @@ gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)
>  
>     We don't do anything with the constructors or destructors,
>     but we have to get the argument passing right anyway.  */
> -static int
> +
> +static struct language_pass_by_ref_info
>  gnuv3_pass_by_reference (struct type *type)
>  {
>    int fieldnum, fieldelem;
>  
>    type = check_typedef (type);
>  
> +  /* Start with the default values.  */
> +  struct language_pass_by_ref_info info
> +    = default_pass_by_reference (type);
> +
> +  /* FIXME: Currently, this implementation only fills in the
> +     'trivially-copyable' field.  */
> +
>    /* We're only interested in things that can have methods.  */
>    if (TYPE_CODE (type) != TYPE_CODE_STRUCT
>        && TYPE_CODE (type) != TYPE_CODE_UNION)
> -    return 0;
> +    return info;
>  
>    /* A dynamic class has a non-trivial copy constructor.
>       See c++98 section 12.8 Copying class objects [class.copy].  */
>    if (gnuv3_dynamic_class (type))
> -    return 1;
> +    {
> +      info.trivially_copyable = false;
> +      return info;
> +    }
>  
>    for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
>      for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
> @@ -1278,7 +1289,10 @@ gnuv3_pass_by_reference (struct type *type)
>  
>  	/* If we've found a destructor, we must pass this by reference.  */
>  	if (name[0] == '~')
> -	  return 1;
> +	  {
> +	    info.trivially_copyable = false;
> +	    return info;
> +	  }
>  
>  	/* If the mangled name of this method doesn't indicate that it
>  	   is a constructor, we're not interested.
> @@ -1300,11 +1314,13 @@ gnuv3_pass_by_reference (struct type *type)
>  
>  	    if (TYPE_CODE (arg_type) == TYPE_CODE_REF)
>  	      {
> -		struct type *arg_target_type;
> -
> -	        arg_target_type = check_typedef (TYPE_TARGET_TYPE (arg_type));
> +		struct type *arg_target_type
> +		  = check_typedef (TYPE_TARGET_TYPE (arg_type));
>  		if (class_types_same_p (arg_target_type, type))
> -		  return 1;
> +		  {
> +		    info.trivially_copyable = false;
> +		    return info;
> +		  }
>  	      }
>  	  }
>        }
> @@ -1317,11 +1333,18 @@ gnuv3_pass_by_reference (struct type *type)
>       about recursive loops here, since we are only looking at members
>       of complete class type.  Also ignore any static members.  */
>    for (fieldnum = 0; fieldnum < TYPE_NFIELDS (type); fieldnum++)
> -    if (! field_is_static (&TYPE_FIELD (type, fieldnum))
> -        && gnuv3_pass_by_reference (TYPE_FIELD_TYPE (type, fieldnum)))
> -      return 1;
> +    if (!field_is_static (&TYPE_FIELD (type, fieldnum)))
> +      {
> +	struct language_pass_by_ref_info field_info
> +	  = gnuv3_pass_by_reference (TYPE_FIELD_TYPE (type, fieldnum));
> +	if (!field_info.trivially_copyable)
> +	  {
> +	    info.trivially_copyable = false;
> +	    return info;
> +	  }
> +      }
>  
> -  return 0;
> +  return info;
>  }
>  
>  static void
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index c102b301e00..058871c369b 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -970,7 +970,8 @@ call_function_by_hand_dummy (struct value *function,
>        args[i] = value_arg_coerce (gdbarch, args[i],
>  				  param_type, prototyped, &sp);
>  
> -      if (param_type != NULL && language_pass_by_reference (param_type))
> +      if (param_type != NULL
> +	  && !(language_pass_by_reference (param_type).trivially_copyable))
>  	args[i] = value_addr (args[i]);
>      }
>  
> diff --git a/gdb/language.c b/gdb/language.c
> index 954e4c200f0..09ed44e1803 100644
> --- a/gdb/language.c
> +++ b/gdb/language.c
> @@ -650,21 +650,32 @@ language_class_name_from_physname (const struct language_defn *lang,
>    return NULL;
>  }
>  
> -/* Return non-zero if TYPE should be passed (and returned) by
> -   reference at the language level.  */
> -int
> +/* Return information about whether TYPE should be passed
> +   (and returned) by reference at the language level.  */
> +
> +struct language_pass_by_ref_info
>  language_pass_by_reference (struct type *type)
>  {
>    return current_language->la_pass_by_reference (type);
>  }
>  
> -/* Return zero; by default, types are passed by value at the language
> -   level.  The target ABI may pass or return some structs by reference
> -   independent of this.  */
> -int
> +/* Return INFO whose 'trivially_copyable' is set to true; by default,
> +   types are passed by value at the language level.  The target ABI may
> +   pass or return some structs by reference independent of this.  */
> +

This comment mentions how 'trivially_copyable' is initialised, but
doesn't really explain any of the other defaults.

Maybe the default field values should be moved out of this function
and into 'struct language_pass_by_reference', and this comment should
just explain that this returns a default struct and languages should
update it as appropriate.

> +struct language_pass_by_ref_info
>  default_pass_by_reference (struct type *type)
>  {
> -  return 0;
> +  struct language_pass_by_ref_info info;
> +  info.copy_constructible = true;
> +  info.destructible = true;
> +  info.trivially_copyable = true;
> +  info.trivially_copy_constructible = true;
> +  info.trivially_destructible = true;
> +  info.cctor_name = NULL;
> +  info.dtor_name = NULL;
> +
> +  return info;
>  }
>  
>  /* Return the default string containing the list of characters
> diff --git a/gdb/language.h b/gdb/language.h
> index 3e0bc9d0d46..40d63dfe8a2 100644
> --- a/gdb/language.h
> +++ b/gdb/language.h
> @@ -128,6 +128,40 @@ struct language_arch_info
>    struct type *bool_type_default;
>  };
>  
> +/* Pass-by-reference information for a call-by-value argument.  */
> +
> +struct language_pass_by_ref_info
> +{
> +  /* Name of the copy ctor function.  Expected to be fully-qualified.  */
> +
> +  const char *cctor_name;

I think the blank lines are not needed between the comments and the
fields, see:
  https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Document_every_global.2BAC8-static_entity

> +
> +  /* Name of the destructor function.  Expected to be fully-qualified.  */
> +
> +  const char *dtor_name;
> +
> +  /* Is the type of the argument in question copy-constructible?  */
> +
> +  bool copy_constructible;

This comment doesn't seem very helpful.  Could you expand on what true
or false value would mean here?  I have the same question about all of
the fields below.

Also I think it might be worth inlining the default value here rather
than in 'default_pass_by_reference', so:

  bool copy_constructible = true;

then you can extend the comment to explain why that default was
selected.  I think doing this for all the fields in this struct would
be a good change.

> +
> +  /* Is the type of the argument in question destructible?  */
> +
> +  bool destructible;
> +
> +  /* Is the argument in question trivially copyable?
> +     That is, is it pass-by-value?  */
> +
> +  bool trivially_copyable;
> +
> +  /* Is the argument in question trivially copy constructible?  */
> +
> +  bool trivially_copy_constructible;
> +
> +  /* Is the argument in question trivially destructible?  */
> +
> +  bool trivially_destructible;
> +};
> +
>  /* Structure tying together assorted information about a language.  */
>  
>  struct language_defn
> @@ -356,9 +390,10 @@ struct language_defn
>                                    struct ui_file *stream,
>                                    const struct value_print_options *options);
>  
> -    /* Return non-zero if TYPE should be passed (and returned) by
> -       reference at the language level.  */
> -    int (*la_pass_by_reference) (struct type *type);
> +    /* Return information about whether TYPE should be passed
> +       (and returned) by reference at the language level.  */
> +    struct language_pass_by_ref_info (*la_pass_by_reference)
> +      (struct type *type);
>  
>      /* Obtain a string from the inferior, storing it in a newly allocated
>         buffer in BUFFER, which should be freed by the caller.  If the
> @@ -612,14 +647,14 @@ extern void default_print_array_index (struct value *index_value,
>                                         struct ui_file *stream,
>  				       const struct value_print_options *options);
>  
> -/* Return non-zero if TYPE should be passed (and returned) by
> -   reference at the language level.  */
> -int language_pass_by_reference (struct type *type);
> +/* Return information about whether TYPE should be passed
> +   (and returned) by reference at the language level.  */
> +struct language_pass_by_ref_info language_pass_by_reference (struct type *type);
>  
> -/* Return zero; by default, types are passed by value at the language
> -   level.  The target ABI may pass or return some structs by reference
> -   independent of this.  */
> -int default_pass_by_reference (struct type *type);
> +/* Return INFO whose 'trivially_copyable' is set to true; by default,
> +   types are passed by value at the language level.  The target ABI may
> +   pass or return some structs by reference independent of this.  */
> +struct language_pass_by_ref_info default_pass_by_reference (struct type *type);
>  
>  /* The default implementation of la_print_typedef.  */
>  void default_print_typedef (struct type *type, struct symbol *new_symbol,
> diff --git a/gdb/tic6x-tdep.c b/gdb/tic6x-tdep.c
> index 61dc676de3b..73149f492f5 100644
> --- a/gdb/tic6x-tdep.c
> +++ b/gdb/tic6x-tdep.c
> @@ -793,7 +793,7 @@ tic6x_return_value (struct gdbarch *gdbarch, struct value *function,
>        if (type != NULL)
>  	{
>  	  type = check_typedef (type);
> -	  if (language_pass_by_reference (type))
> +	  if (!(language_pass_by_reference (type).trivially_copyable))
>  	    return RETURN_VALUE_STRUCT_CONVENTION;
>  	}
>      }
> -- 
> 2.21.0
> 

Thanks,
Andrew



More information about the Gdb-patches mailing list