[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