[3/8] infcall, c++: collect more pass-by-reference information
Commit Message
Walk through a given type to collect information about whether the type
is copy constructible, destructible, trivially copyable, trivially copy
constructible, trivially destructible. The previous algorithm returned
only a boolean result about whether the type is trivially copyable.
This patch computes more info. Additionally, it utilizes DWARF attributes
that were previously not taken into account; namely,
DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.
gdb/ChangeLog:
* gnu-v3-abi.c (enum definition_style): New.
(get_def_style): New.
(is_user_provided_def): New.
(is_implicit_def): New.
(is_copy_or_move_constructor_type): New.
(is_copy_constructor_type): New.
(is_move_constructor_type): New.
(gnuv3_pass_by_reference): Update.
---
gdb/gnu-v3-abi.c | 259 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 208 insertions(+), 51 deletions(-)
Comments
* Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> [2019-04-23 16:31:49 +0200]:
> Walk through a given type to collect information about whether the type
> is copy constructible, destructible, trivially copyable, trivially copy
> constructible, trivially destructible. The previous algorithm returned
> only a boolean result about whether the type is trivially copyable.
> This patch computes more info. Additionally, it utilizes DWARF attributes
> that were previously not taken into account; namely,
> DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.
>
> gdb/ChangeLog:
>
> * gnu-v3-abi.c (enum definition_style): New.
> (get_def_style): New.
> (is_user_provided_def): New.
> (is_implicit_def): New.
> (is_copy_or_move_constructor_type): New.
> (is_copy_constructor_type): New.
> (is_move_constructor_type): New.
> (gnuv3_pass_by_reference): Update.
I'm basically happy with this, a few formatting issues and questions
about comments below.
>
> ---
> gdb/gnu-v3-abi.c | 259 +++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 208 insertions(+), 51 deletions(-)
>
> diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
> index b39e5a5c585..9069305626f 100644
> --- a/gdb/gnu-v3-abi.c
> +++ b/gdb/gnu-v3-abi.c
> @@ -23,6 +23,7 @@
> #include "cp-abi.h"
> #include "cp-support.h"
> #include "demangle.h"
> +#include "dwarf2.h"
> #include "objfiles.h"
> #include "valprint.h"
> #include "c-lang.h"
> @@ -1228,6 +1229,122 @@ gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)
> return real_stop_pc;
> }
>
> +/* A member function is in one these states. */
> +
> +enum definition_style
> +{
> + DOES_NOT_EXIST_IN_SOURCE,
> + DEFAULTED_INSIDE,
> + DEFAULTED_OUTSIDE,
> + DELETED,
> + EXPLICIT,
> +};
> +
> +/* Return how the given field is defined. */
> +
> +static definition_style
> +get_def_style (struct fn_field *fn, int fieldelem)
> +{
> + if (TYPE_FN_FIELD_DELETED (fn, fieldelem))
> + return DELETED;
> +
> + if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))
> + return DOES_NOT_EXIST_IN_SOURCE;
> +
> + switch (TYPE_FN_FIELD_DEFAULTED (fn, fieldelem))
> + {
> + case DW_DEFAULTED_no:
> + return EXPLICIT;
> + case DW_DEFAULTED_in_class:
> + return DEFAULTED_INSIDE;
> + case DW_DEFAULTED_out_of_class:
> + return DEFAULTED_OUTSIDE;
> + default:
> + break;
> + }
> +
> + return EXPLICIT;
> +}
> +
> +/* Helper functions to determine whether the given definition style
> + denotes that the definition is user-provided or implicit.
> + Being defaulted outside the class decl counts as an explicit
> + user-definition, while being defaulted inside is implicit. */
> +
> +static bool
> +is_user_provided_def (definition_style def)
> +{
> + return def == EXPLICIT || def == DEFAULTED_OUTSIDE;
> +}
> +
> +static bool
> +is_implicit_def (definition_style def)
> +{
> + return def == DOES_NOT_EXIST_IN_SOURCE || def == DEFAULTED_INSIDE;
> +}
> +
> +/* Helper function to decide if METHOD_TYPE is a copy/move
> + constructor type for CLASS_TYPE. EXPECTED is the expected
> + type code for the "right-hand-side" argument.
> + This function is supposed to be used by the IS_COPY_CONSTRUCTOR_TYPE
> + and IS_MOVE_CONSTRUCTOR_TYPE functions below. Normally, you should
> + not need to call this directly. */
> +
> +static bool
> +is_copy_or_move_constructor_type (struct type *class_type,
> + struct type *method_type,
> + type_code expected)
> +{
> + /* The method should take at least two arguments... */
> + if (TYPE_NFIELDS (method_type) < 2)
> + return false;
> +
> + /* ...and the second argument should be the same as the class
> + type, with the expected type code... */
> + struct type *arg_type = TYPE_FIELD_TYPE (method_type, 1);
> +
> + if (TYPE_CODE (arg_type) != expected)
> + return false;
> +
> + struct type *target = check_typedef (TYPE_TARGET_TYPE (arg_type));
> + if (!(class_types_same_p (target, class_type)))
> + return false;
> +
> + /* ...and the rest of the arguments should have default values. */
This comment threw me for a while I read this to mean that I couldn't
legally write something like:
struct A
{
int a;
A (const A &other, int arg);
};
which is daft, because obviously I can. Then I realised that that
isn't a copy constructor, it's just a constructor that takes an 'A&',
doh!
But, I wonder if the comment could be made clearer to help those of us
a bit slower on the uptake, maybe something like:
/* ...and if any of the remaining arguments don't have a default value
then this is not a copy or move constructor, but just a constructor. */
> + for (int i = 2; i < TYPE_NFIELDS (method_type); i++)
> + {
> + arg_type = TYPE_FIELD_TYPE (method_type, i);
> + /* FIXME taktemur/2019-04-23: As of this date, neither
> + clang++-7.0.0 nor g++-8.2.0 produce a DW_AT_default_value
> + attribute. GDB is also not set to read this attribute, yet.
> + Hence, we immediately return false if there are more than
> + 2 parameters. */
> + return false;
> + }
> +
> + return true;
> +}
> +
> +/* Return true if METHOD_TYPE is a copy ctor type for CLASS_TYPE. */
> +
> +static bool
> +is_copy_constructor_type (struct type *class_type,
> + struct type *method_type)
> +{
> + return is_copy_or_move_constructor_type (class_type, method_type,
> + TYPE_CODE_REF);
> +}
> +
> +/* Return true if METHOD_TYPE is a move ctor type for CLASS_TYPE. */
> +
> +static bool
> +is_move_constructor_type (struct type *class_type,
> + struct type *method_type)
> +{
> + return is_copy_or_move_constructor_type (class_type, method_type,
> + TYPE_CODE_RVALUE_REF);
> +}
> +
> /* Return pass-by-reference information for the given TYPE.
>
> The rule in the v3 ABI document comes from section 3.1.1. If the
> @@ -1236,16 +1353,15 @@ gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)
> is one or perform the copy itself otherwise), pass the address of
> the copy, and then destroy the temporary (if necessary).
>
> - For return values with non-trivial copy constructors or
> + For return values with non-trivial copy/move constructors or
> destructors, space will be allocated in the caller, and a pointer
> will be passed as the first argument (preceding "this").
>
> We don't have a bulletproof mechanism for determining whether a
> - constructor or destructor is trivial. For GCC and DWARF2 debug
> - information, we can check the artificial flag.
> -
> - We don't do anything with the constructors or destructors,
> - but we have to get the argument passing right anyway. */
> + constructor or destructor is trivial. For GCC and DWARF5 debug
> + information, we can check the calling_convention attribute,
> + the 'artificial' flag, the 'defaulted' attribute, and the
> + 'deleted' attribute. */
>
> static struct language_pass_by_ref_info
> gnuv3_pass_by_reference (struct type *type)
> @@ -1258,22 +1374,39 @@ gnuv3_pass_by_reference (struct type *type)
> struct language_pass_by_ref_info info
> = default_pass_by_reference (type);
>
> - /* FIXME: Currently, this implementation only fills in the
> - 'trivially-copyable' field. */
> + bool has_cc_attr = false;
> + bool is_pass_by_value = false;
> + bool is_dynamic = false;
> + definition_style cctor_def = DOES_NOT_EXIST_IN_SOURCE;
> + definition_style dtor_def = DOES_NOT_EXIST_IN_SOURCE;
> + definition_style mctor_def = DOES_NOT_EXIST_IN_SOURCE;
>
> /* We're only interested in things that can have methods. */
> if (TYPE_CODE (type) != TYPE_CODE_STRUCT
> && TYPE_CODE (type) != TYPE_CODE_UNION)
> return info;
>
> + /* The compiler may have emitted the calling convention attribute. */
> + if (TYPE_CPLUS_CALLING_CONVENTION (type) == DW_CC_pass_by_value)
> + {
> + has_cc_attr = true;
> + is_pass_by_value = true;
> + /* Do not return immediately. We have to find out if this type
> + is copy_constructible and destructible. */
> + }
> +
> + if (TYPE_CPLUS_CALLING_CONVENTION (type) == DW_CC_pass_by_reference)
> + {
> + has_cc_attr = true;
> + is_pass_by_value = false;
> + }
> +
> /* 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))
> - {
> - info.trivially_copyable = false;
> - return info;
> - }
> + is_dynamic = true;
>
> + /* FIXME taktemur/2019-04-23: What if there are multiple cctors? */
Can such a situation arise? If you know how it could but don't know
how to handle it then can we expand the comment. If you don't think
such a situation could arise then lets delete this comment and add an
assertion below.
> for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
> for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
> fieldelem++)
> @@ -1282,49 +1415,53 @@ gnuv3_pass_by_reference (struct type *type)
> const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);
> struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);
>
> - /* If this function is marked as artificial, it is compiler-generated,
> - and we assume it is trivial. */
> - if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))
> - continue;
> -
> - /* If we've found a destructor, we must pass this by reference. */
> if (name[0] == '~')
> {
> - info.trivially_copyable = false;
> - return info;
> + /* We've found a destructor. */
> + dtor_def = get_def_style (fn, fieldelem);
> + info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
Maybe we should have an error or at least a warning if
'info.dtor_name' is not nullptr before this assignment - this would
indicate multiple destructors, which seems weird, right?
> }
> -
> - /* If the mangled name of this method doesn't indicate that it
> - is a constructor, we're not interested.
> -
> - FIXME drow/2007-09-23: We could do this using the name of
> - the method and the name of the class instead of dealing
> - with the mangled name. We don't have a convenient function
> - to strip off both leading scope qualifiers and trailing
> - template arguments yet. */
> - if (!is_constructor_name (TYPE_FN_FIELD_PHYSNAME (fn, fieldelem))
> - && !TYPE_FN_FIELD_CONSTRUCTOR (fn, fieldelem))
> - continue;
> -
> - /* If this method takes two arguments, and the second argument is
> - a reference to this class, then it is a copy constructor. */
> - if (TYPE_NFIELDS (fieldtype) == 2)
> + else if (is_constructor_name (TYPE_FN_FIELD_PHYSNAME (fn, fieldelem))
> + || TYPE_FN_FIELD_CONSTRUCTOR (fn, fieldelem))
> {
> - struct type *arg_type = TYPE_FIELD_TYPE (fieldtype, 1);
> -
> - if (TYPE_CODE (arg_type) == TYPE_CODE_REF)
> + /* FIXME drow/2007-09-23: We could do this using the name of
> + the method and the name of the class instead of dealing
> + with the mangled name. We don't have a convenient function
> + to strip off both leading scope qualifiers and trailing
> + template arguments yet. */
> + if (is_copy_constructor_type (type, fieldtype))
> {
> - struct type *arg_target_type
> - = check_typedef (TYPE_TARGET_TYPE (arg_type));
> - if (class_types_same_p (arg_target_type, type))
> - {
> - info.trivially_copyable = false;
> - return info;
> - }
> + cctor_def = get_def_style (fn, fieldelem);
> + info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
This would be where we assert that we only have one cctor I think...
> }
> + else if (is_move_constructor_type (type, fieldtype))
> + mctor_def = get_def_style (fn, fieldelem);
> }
> }
>
> + bool cctor_implicitly_deleted
> + = mctor_def != DOES_NOT_EXIST_IN_SOURCE
> + && cctor_def == DOES_NOT_EXIST_IN_SOURCE;
I think this should be parenthesised like this:
bool cctor_implicitly_deleted
= (mctor_def != DOES_NOT_EXIST_IN_SOURCE
&& cctor_def == DOES_NOT_EXIST_IN_SOURCE);
My reference is:
https://www.gnu.org/prep/standards/standards.html#Formatting
> +
> + bool cctor_explicitly_deleted = cctor_def == DELETED;
> +
> + if (cctor_implicitly_deleted || cctor_explicitly_deleted)
> + info.copy_constructible = false;
> +
> + if (dtor_def == DELETED)
> + info.destructible = false;
> +
> + info.trivially_destructible = is_implicit_def (dtor_def);
> +
> + info.trivially_copy_constructible
> + = is_implicit_def (cctor_def)
> + && !is_dynamic;
Again extra ( ... ) needed I think.
> +
> + info.trivially_copyable
> + = info.trivially_copy_constructible
> + && info.trivially_destructible
> + && !is_user_provided_def (mctor_def);
And again.
> +
> /* Even if all the constructors and destructors were artificial, one
> of them may have invoked a non-artificial constructor or
> destructor in a base class. If any base class needs to be passed
> @@ -1335,15 +1472,35 @@ gnuv3_pass_by_reference (struct type *type)
> for (fieldnum = 0; fieldnum < TYPE_NFIELDS (type); fieldnum++)
> if (!field_is_static (&TYPE_FIELD (type, fieldnum)))
> {
> + struct type *field_type = TYPE_FIELD_TYPE (type, fieldnum);
> +
> + /* For arrays, make the decision based on the element type. */
> + if (TYPE_CODE (field_type) == TYPE_CODE_ARRAY)
> + field_type = field_type->main_type->target_type;
> +
> struct language_pass_by_ref_info field_info
> - = gnuv3_pass_by_reference (TYPE_FIELD_TYPE (type, fieldnum));
> + = gnuv3_pass_by_reference (field_type);
> +
> + if (!field_info.copy_constructible)
> + info.copy_constructible = false;
> + if (!field_info.destructible)
> + info.destructible = false;
> if (!field_info.trivially_copyable)
> - {
> - info.trivially_copyable = false;
> - return info;
> - }
> + info.trivially_copyable = false;
> + if (!field_info.trivially_copy_constructible)
> + info.trivially_copy_constructible = false;
> + if (!field_info.trivially_destructible)
> + info.trivially_destructible = false;
> }
>
> + /* Consistency check. */
> + if (has_cc_attr && info.trivially_copyable != is_pass_by_value)
> + {
> + /* DWARF CC attribute is not the same as the inferred value;
> + using the DWARF attribute. */
> + info.trivially_copyable = is_pass_by_value;
> + }
> +
> return info;
> }
>
> --
> 2.21.0
>
Thanks,
Andrew
> > Walk through a given type to collect information about whether the type
> > is copy constructible, destructible, trivially copyable, trivially copy
> > constructible, trivially destructible. The previous algorithm returned
> > only a boolean result about whether the type is trivially copyable.
> > This patch computes more info. Additionally, it utilizes DWARF attributes
> > that were previously not taken into account; namely,
> > DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.
>
> I'm basically happy with this, a few formatting issues and questions
> about comments below.
>
Thank you.
> >
> > + /* FIXME taktemur/2019-04-23: What if there are multiple cctors? */
>
> Can such a situation arise? If you know how it could but don't know
> how to handle it then can we expand the comment. If you don't think
> such a situation could arise then lets delete this comment and add an
> assertion below.
>
Such a situation can arise when there is a copy ctor that takes a
non-const &T and another that takes a const &T. I'm planning to
add the example below to the comment:
/* FIXME taktemur/2019-04-23: What if there are multiple copy ctors?
E.g.:
class C {
public:
C (C &c) { ... }
C (const C &c) { ... }
};
*/
The correct version shall be selected based on the type of the argument,
but I don't know how to express that in GDB.
> > for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
> > for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
> > fieldelem++)
> > @@ -1282,49 +1415,53 @@ gnuv3_pass_by_reference (struct type *type)
> > const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);
> > struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);
> >
> > - /* If this function is marked as artificial, it is compiler-generated,
> > - and we assume it is trivial. */
> > - if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))
> > - continue;
> > -
> > - /* If we've found a destructor, we must pass this by reference. */
> > if (name[0] == '~')
> > {
> > - info.trivially_copyable = false;
> > - return info;
> > + /* We've found a destructor. */
> > + dtor_def = get_def_style (fn, fieldelem);
> > + info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
>
> Maybe we should have an error or at least a warning if
> 'info.dtor_name' is not nullptr before this assignment - this would
> indicate multiple destructors, which seems weird, right?
>
I believe the 'info.dtor_name' field can be nullptr even if a destructor
definition exists, if the destructor is inlined and hence its code does
not exist in the object file. (Such a case also requires special treatment
and is handled at the client side, in gdb/infcall.c in part 7/8 of this patch.)
So, I thought I should gdb_assert on 'dtor_def == DOES_NOT_EXIST_IN_SOURCE'
instead. Is this OK?
> > + if (is_copy_constructor_type (type, fieldtype))
> > {
> > - struct type *arg_target_type
> > - = check_typedef (TYPE_TARGET_TYPE (arg_type));
> > - if (class_types_same_p (arg_target_type, type))
> > - {
> > - info.trivially_copyable = false;
> > - return info;
> > - }
> > + cctor_def = get_def_style (fn, fieldelem);
> > + info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
>
> This would be where we assert that we only have one cctor I think...
>
Similarly, I'm planning to assert 'cctor_def == DOES_NOT_EXIST_IN_SOURCE'.
> > + bool cctor_implicitly_deleted
> > + = mctor_def != DOES_NOT_EXIST_IN_SOURCE
> > + && cctor_def == DOES_NOT_EXIST_IN_SOURCE;
>
> I think this should be parenthesised like this:
>
> bool cctor_implicitly_deleted
> = (mctor_def != DOES_NOT_EXIST_IN_SOURCE
> && cctor_def == DOES_NOT_EXIST_IN_SOURCE);
>
> My reference is:
> https://www.gnu.org/prep/standards/standards.html#Formatting
>
Thanks for the pointer. I'll address these formatting issues in the next update.
>
> Thanks,
> Andrew
Regards,
-Baris
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
* Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> [2019-05-31 13:55:58 +0000]:
> > > Walk through a given type to collect information about whether the type
> > > is copy constructible, destructible, trivially copyable, trivially copy
> > > constructible, trivially destructible. The previous algorithm returned
> > > only a boolean result about whether the type is trivially copyable.
> > > This patch computes more info. Additionally, it utilizes DWARF attributes
> > > that were previously not taken into account; namely,
> > > DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.
> >
> > I'm basically happy with this, a few formatting issues and questions
> > about comments below.
> >
>
> Thank you.
>
> > >
> > > + /* FIXME taktemur/2019-04-23: What if there are multiple cctors? */
> >
> > Can such a situation arise? If you know how it could but don't know
> > how to handle it then can we expand the comment. If you don't think
> > such a situation could arise then lets delete this comment and add an
> > assertion below.
> >
>
> Such a situation can arise when there is a copy ctor that takes a
> non-const &T and another that takes a const &T. I'm planning to
> add the example below to the comment:
>
> /* FIXME taktemur/2019-04-23: What if there are multiple copy ctors?
> E.g.:
> class C {
> public:
> C (C &c) { ... }
> C (const C &c) { ... }
> };
> */
That would be great, I find information like this in comments very
helpful so thank you.
>
> The correct version shall be selected based on the type of the argument,
> but I don't know how to express that in GDB.
>
>
> > > for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
> > > for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
> > > fieldelem++)
> > > @@ -1282,49 +1415,53 @@ gnuv3_pass_by_reference (struct type *type)
> > > const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);
> > > struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);
> > >
> > > - /* If this function is marked as artificial, it is compiler-generated,
> > > - and we assume it is trivial. */
> > > - if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))
> > > - continue;
> > > -
> > > - /* If we've found a destructor, we must pass this by reference. */
> > > if (name[0] == '~')
> > > {
> > > - info.trivially_copyable = false;
> > > - return info;
> > > + /* We've found a destructor. */
> > > + dtor_def = get_def_style (fn, fieldelem);
> > > + info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
> >
> > Maybe we should have an error or at least a warning if
> > 'info.dtor_name' is not nullptr before this assignment - this would
> > indicate multiple destructors, which seems weird, right?
> >
>
> I believe the 'info.dtor_name' field can be nullptr even if a destructor
> definition exists, if the destructor is inlined and hence its code does
> not exist in the object file. (Such a case also requires special treatment
> and is handled at the client side, in gdb/infcall.c in part 7/8 of this patch.)
> So, I thought I should gdb_assert on 'dtor_def == DOES_NOT_EXIST_IN_SOURCE'
> instead. Is this OK?
>
> > > + if (is_copy_constructor_type (type, fieldtype))
> > > {
> > > - struct type *arg_target_type
> > > - = check_typedef (TYPE_TARGET_TYPE (arg_type));
> > > - if (class_types_same_p (arg_target_type, type))
> > > - {
> > > - info.trivially_copyable = false;
> > > - return info;
> > > - }
> > > + cctor_def = get_def_style (fn, fieldelem);
> > > + info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
> >
> > This would be where we assert that we only have one cctor I think...
> >
>
> Similarly, I'm planning to assert 'cctor_def == DOES_NOT_EXIST_IN_SOURCE'.
>
OK, that sounds reasonable.
If nobody else reviews the remaining patches in this series then I
will get around to them soon I hope. I've been crazy busy the last
couple of weeks and am still trying to catch up on everything.
Thanks,
Andrew
> > > + bool cctor_implicitly_deleted
> > > + = mctor_def != DOES_NOT_EXIST_IN_SOURCE
> > > + && cctor_def == DOES_NOT_EXIST_IN_SOURCE;
> >
> > I think this should be parenthesised like this:
> >
> > bool cctor_implicitly_deleted
> > = (mctor_def != DOES_NOT_EXIST_IN_SOURCE
> > && cctor_def == DOES_NOT_EXIST_IN_SOURCE);
> >
> > My reference is:
> > https://www.gnu.org/prep/standards/standards.html#Formatting
> >
>
> Thanks for the pointer. I'll address these formatting issues in the next update.
>
> >
> > Thanks,
> > Andrew
>
> Regards,
> -Baris
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>
* On Sunday, June 23, 2019 9:24 PM, Andrew Burgess wrote:
> * Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> [2019-05-31 13:55:58 +0000]:
>
> > > > Walk through a given type to collect information about whether the type
> > > > is copy constructible, destructible, trivially copyable, trivially copy
> > > > constructible, trivially destructible. The previous algorithm returned
> > > > only a boolean result about whether the type is trivially copyable.
> > > > This patch computes more info. Additionally, it utilizes DWARF attributes
> > > > that were previously not taken into account; namely,
> > > > DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.
> > >
> > > I'm basically happy with this, a few formatting issues and questions
> > > about comments below.
> > >
> >
> > Thank you.
> >
> > > >
> > > > + /* FIXME taktemur/2019-04-23: What if there are multiple cctors? */
> > >
> > > Can such a situation arise? If you know how it could but don't know
> > > how to handle it then can we expand the comment. If you don't think
> > > such a situation could arise then lets delete this comment and add an
> > > assertion below.
> > >
> >
> > Such a situation can arise when there is a copy ctor that takes a
> > non-const &T and another that takes a const &T. I'm planning to
> > add the example below to the comment:
> >
> > /* FIXME taktemur/2019-04-23: What if there are multiple copy ctors?
> > E.g.:
> > class C {
> > public:
> > C (C &c) { ... }
> > C (const C &c) { ... }
> > };
> > */
>
> That would be great, I find information like this in comments very
> helpful so thank you.
>
> >
> > The correct version shall be selected based on the type of the argument,
> > but I don't know how to express that in GDB.
> >
> >
> > > > for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
> > > > for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
> > > > fieldelem++)
> > > > @@ -1282,49 +1415,53 @@ gnuv3_pass_by_reference (struct type *type)
> > > > const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);
> > > > struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);
> > > >
> > > > - /* If this function is marked as artificial, it is compiler-generated,
> > > > - and we assume it is trivial. */
> > > > - if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))
> > > > - continue;
> > > > -
> > > > - /* If we've found a destructor, we must pass this by reference. */
> > > > if (name[0] == '~')
> > > > {
> > > > - info.trivially_copyable = false;
> > > > - return info;
> > > > + /* We've found a destructor. */
> > > > + dtor_def = get_def_style (fn, fieldelem);
> > > > + info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
> > >
> > > Maybe we should have an error or at least a warning if
> > > 'info.dtor_name' is not nullptr before this assignment - this would
> > > indicate multiple destructors, which seems weird, right?
> > >
> >
> > I believe the 'info.dtor_name' field can be nullptr even if a destructor
> > definition exists, if the destructor is inlined and hence its code does
> > not exist in the object file. (Such a case also requires special treatment
> > and is handled at the client side, in gdb/infcall.c in part 7/8 of this patch.)
> > So, I thought I should gdb_assert on 'dtor_def == DOES_NOT_EXIST_IN_SOURCE'
> > instead. Is this OK?
> >
> > > > + if (is_copy_constructor_type (type, fieldtype))
> > > > {
> > > > - struct type *arg_target_type
> > > > - = check_typedef (TYPE_TARGET_TYPE (arg_type));
> > > > - if (class_types_same_p (arg_target_type, type))
> > > > - {
> > > > - info.trivially_copyable = false;
> > > > - return info;
> > > > - }
> > > > + cctor_def = get_def_style (fn, fieldelem);
> > > > + info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
> > >
> > > This would be where we assert that we only have one cctor I think...
> > >
> >
> > Similarly, I'm planning to assert 'cctor_def == DOES_NOT_EXIST_IN_SOURCE'.
> >
>
> OK, that sounds reasonable.
>
> If nobody else reviews the remaining patches in this series then I
> will get around to them soon I hope. I've been crazy busy the last
> couple of weeks and am still trying to catch up on everything.
>
> Thanks,
> Andrew
>
Thank you! The remaining patches have not received a review so far.
I'll submit today a v2 of the series in which your comments have been addressed
in patches 1-3.
-Baris
> > > > + bool cctor_implicitly_deleted
> > > > + = mctor_def != DOES_NOT_EXIST_IN_SOURCE
> > > > + && cctor_def == DOES_NOT_EXIST_IN_SOURCE;
> > >
> > > I think this should be parenthesised like this:
> > >
> > > bool cctor_implicitly_deleted
> > > = (mctor_def != DOES_NOT_EXIST_IN_SOURCE
> > > && cctor_def == DOES_NOT_EXIST_IN_SOURCE);
> > >
> > > My reference is:
> > > https://www.gnu.org/prep/standards/standards.html#Formatting
> > >
> >
> > Thanks for the pointer. I'll address these formatting issues in the next update.
> >
> > >
> > > Thanks,
> > > Andrew
> >
> > Regards,
> > -Baris
> >
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
@@ -23,6 +23,7 @@
#include "cp-abi.h"
#include "cp-support.h"
#include "demangle.h"
+#include "dwarf2.h"
#include "objfiles.h"
#include "valprint.h"
#include "c-lang.h"
@@ -1228,6 +1229,122 @@ gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)
return real_stop_pc;
}
+/* A member function is in one these states. */
+
+enum definition_style
+{
+ DOES_NOT_EXIST_IN_SOURCE,
+ DEFAULTED_INSIDE,
+ DEFAULTED_OUTSIDE,
+ DELETED,
+ EXPLICIT,
+};
+
+/* Return how the given field is defined. */
+
+static definition_style
+get_def_style (struct fn_field *fn, int fieldelem)
+{
+ if (TYPE_FN_FIELD_DELETED (fn, fieldelem))
+ return DELETED;
+
+ if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))
+ return DOES_NOT_EXIST_IN_SOURCE;
+
+ switch (TYPE_FN_FIELD_DEFAULTED (fn, fieldelem))
+ {
+ case DW_DEFAULTED_no:
+ return EXPLICIT;
+ case DW_DEFAULTED_in_class:
+ return DEFAULTED_INSIDE;
+ case DW_DEFAULTED_out_of_class:
+ return DEFAULTED_OUTSIDE;
+ default:
+ break;
+ }
+
+ return EXPLICIT;
+}
+
+/* Helper functions to determine whether the given definition style
+ denotes that the definition is user-provided or implicit.
+ Being defaulted outside the class decl counts as an explicit
+ user-definition, while being defaulted inside is implicit. */
+
+static bool
+is_user_provided_def (definition_style def)
+{
+ return def == EXPLICIT || def == DEFAULTED_OUTSIDE;
+}
+
+static bool
+is_implicit_def (definition_style def)
+{
+ return def == DOES_NOT_EXIST_IN_SOURCE || def == DEFAULTED_INSIDE;
+}
+
+/* Helper function to decide if METHOD_TYPE is a copy/move
+ constructor type for CLASS_TYPE. EXPECTED is the expected
+ type code for the "right-hand-side" argument.
+ This function is supposed to be used by the IS_COPY_CONSTRUCTOR_TYPE
+ and IS_MOVE_CONSTRUCTOR_TYPE functions below. Normally, you should
+ not need to call this directly. */
+
+static bool
+is_copy_or_move_constructor_type (struct type *class_type,
+ struct type *method_type,
+ type_code expected)
+{
+ /* The method should take at least two arguments... */
+ if (TYPE_NFIELDS (method_type) < 2)
+ return false;
+
+ /* ...and the second argument should be the same as the class
+ type, with the expected type code... */
+ struct type *arg_type = TYPE_FIELD_TYPE (method_type, 1);
+
+ if (TYPE_CODE (arg_type) != expected)
+ return false;
+
+ struct type *target = check_typedef (TYPE_TARGET_TYPE (arg_type));
+ if (!(class_types_same_p (target, class_type)))
+ return false;
+
+ /* ...and the rest of the arguments should have default values. */
+ for (int i = 2; i < TYPE_NFIELDS (method_type); i++)
+ {
+ arg_type = TYPE_FIELD_TYPE (method_type, i);
+ /* FIXME taktemur/2019-04-23: As of this date, neither
+ clang++-7.0.0 nor g++-8.2.0 produce a DW_AT_default_value
+ attribute. GDB is also not set to read this attribute, yet.
+ Hence, we immediately return false if there are more than
+ 2 parameters. */
+ return false;
+ }
+
+ return true;
+}
+
+/* Return true if METHOD_TYPE is a copy ctor type for CLASS_TYPE. */
+
+static bool
+is_copy_constructor_type (struct type *class_type,
+ struct type *method_type)
+{
+ return is_copy_or_move_constructor_type (class_type, method_type,
+ TYPE_CODE_REF);
+}
+
+/* Return true if METHOD_TYPE is a move ctor type for CLASS_TYPE. */
+
+static bool
+is_move_constructor_type (struct type *class_type,
+ struct type *method_type)
+{
+ return is_copy_or_move_constructor_type (class_type, method_type,
+ TYPE_CODE_RVALUE_REF);
+}
+
/* Return pass-by-reference information for the given TYPE.
The rule in the v3 ABI document comes from section 3.1.1. If the
@@ -1236,16 +1353,15 @@ gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)
is one or perform the copy itself otherwise), pass the address of
the copy, and then destroy the temporary (if necessary).
- For return values with non-trivial copy constructors or
+ For return values with non-trivial copy/move constructors or
destructors, space will be allocated in the caller, and a pointer
will be passed as the first argument (preceding "this").
We don't have a bulletproof mechanism for determining whether a
- constructor or destructor is trivial. For GCC and DWARF2 debug
- information, we can check the artificial flag.
-
- We don't do anything with the constructors or destructors,
- but we have to get the argument passing right anyway. */
+ constructor or destructor is trivial. For GCC and DWARF5 debug
+ information, we can check the calling_convention attribute,
+ the 'artificial' flag, the 'defaulted' attribute, and the
+ 'deleted' attribute. */
static struct language_pass_by_ref_info
gnuv3_pass_by_reference (struct type *type)
@@ -1258,22 +1374,39 @@ gnuv3_pass_by_reference (struct type *type)
struct language_pass_by_ref_info info
= default_pass_by_reference (type);
- /* FIXME: Currently, this implementation only fills in the
- 'trivially-copyable' field. */
+ bool has_cc_attr = false;
+ bool is_pass_by_value = false;
+ bool is_dynamic = false;
+ definition_style cctor_def = DOES_NOT_EXIST_IN_SOURCE;
+ definition_style dtor_def = DOES_NOT_EXIST_IN_SOURCE;
+ definition_style mctor_def = DOES_NOT_EXIST_IN_SOURCE;
/* We're only interested in things that can have methods. */
if (TYPE_CODE (type) != TYPE_CODE_STRUCT
&& TYPE_CODE (type) != TYPE_CODE_UNION)
return info;
+ /* The compiler may have emitted the calling convention attribute. */
+ if (TYPE_CPLUS_CALLING_CONVENTION (type) == DW_CC_pass_by_value)
+ {
+ has_cc_attr = true;
+ is_pass_by_value = true;
+ /* Do not return immediately. We have to find out if this type
+ is copy_constructible and destructible. */
+ }
+
+ if (TYPE_CPLUS_CALLING_CONVENTION (type) == DW_CC_pass_by_reference)
+ {
+ has_cc_attr = true;
+ is_pass_by_value = false;
+ }
+
/* 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))
- {
- info.trivially_copyable = false;
- return info;
- }
+ is_dynamic = true;
+ /* FIXME taktemur/2019-04-23: What if there are multiple cctors? */
for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
fieldelem++)
@@ -1282,49 +1415,53 @@ gnuv3_pass_by_reference (struct type *type)
const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);
struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);
- /* If this function is marked as artificial, it is compiler-generated,
- and we assume it is trivial. */
- if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))
- continue;
-
- /* If we've found a destructor, we must pass this by reference. */
if (name[0] == '~')
{
- info.trivially_copyable = false;
- return info;
+ /* We've found a destructor. */
+ dtor_def = get_def_style (fn, fieldelem);
+ info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
}
-
- /* If the mangled name of this method doesn't indicate that it
- is a constructor, we're not interested.
-
- FIXME drow/2007-09-23: We could do this using the name of
- the method and the name of the class instead of dealing
- with the mangled name. We don't have a convenient function
- to strip off both leading scope qualifiers and trailing
- template arguments yet. */
- if (!is_constructor_name (TYPE_FN_FIELD_PHYSNAME (fn, fieldelem))
- && !TYPE_FN_FIELD_CONSTRUCTOR (fn, fieldelem))
- continue;
-
- /* If this method takes two arguments, and the second argument is
- a reference to this class, then it is a copy constructor. */
- if (TYPE_NFIELDS (fieldtype) == 2)
+ else if (is_constructor_name (TYPE_FN_FIELD_PHYSNAME (fn, fieldelem))
+ || TYPE_FN_FIELD_CONSTRUCTOR (fn, fieldelem))
{
- struct type *arg_type = TYPE_FIELD_TYPE (fieldtype, 1);
-
- if (TYPE_CODE (arg_type) == TYPE_CODE_REF)
+ /* FIXME drow/2007-09-23: We could do this using the name of
+ the method and the name of the class instead of dealing
+ with the mangled name. We don't have a convenient function
+ to strip off both leading scope qualifiers and trailing
+ template arguments yet. */
+ if (is_copy_constructor_type (type, fieldtype))
{
- struct type *arg_target_type
- = check_typedef (TYPE_TARGET_TYPE (arg_type));
- if (class_types_same_p (arg_target_type, type))
- {
- info.trivially_copyable = false;
- return info;
- }
+ cctor_def = get_def_style (fn, fieldelem);
+ info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
}
+ else if (is_move_constructor_type (type, fieldtype))
+ mctor_def = get_def_style (fn, fieldelem);
}
}
+ bool cctor_implicitly_deleted
+ = mctor_def != DOES_NOT_EXIST_IN_SOURCE
+ && cctor_def == DOES_NOT_EXIST_IN_SOURCE;
+
+ bool cctor_explicitly_deleted = cctor_def == DELETED;
+
+ if (cctor_implicitly_deleted || cctor_explicitly_deleted)
+ info.copy_constructible = false;
+
+ if (dtor_def == DELETED)
+ info.destructible = false;
+
+ info.trivially_destructible = is_implicit_def (dtor_def);
+
+ info.trivially_copy_constructible
+ = is_implicit_def (cctor_def)
+ && !is_dynamic;
+
+ info.trivially_copyable
+ = info.trivially_copy_constructible
+ && info.trivially_destructible
+ && !is_user_provided_def (mctor_def);
+
/* Even if all the constructors and destructors were artificial, one
of them may have invoked a non-artificial constructor or
destructor in a base class. If any base class needs to be passed
@@ -1335,15 +1472,35 @@ gnuv3_pass_by_reference (struct type *type)
for (fieldnum = 0; fieldnum < TYPE_NFIELDS (type); fieldnum++)
if (!field_is_static (&TYPE_FIELD (type, fieldnum)))
{
+ struct type *field_type = TYPE_FIELD_TYPE (type, fieldnum);
+
+ /* For arrays, make the decision based on the element type. */
+ if (TYPE_CODE (field_type) == TYPE_CODE_ARRAY)
+ field_type = field_type->main_type->target_type;
+
struct language_pass_by_ref_info field_info
- = gnuv3_pass_by_reference (TYPE_FIELD_TYPE (type, fieldnum));
+ = gnuv3_pass_by_reference (field_type);
+
+ if (!field_info.copy_constructible)
+ info.copy_constructible = false;
+ if (!field_info.destructible)
+ info.destructible = false;
if (!field_info.trivially_copyable)
- {
- info.trivially_copyable = false;
- return info;
- }
+ info.trivially_copyable = false;
+ if (!field_info.trivially_copy_constructible)
+ info.trivially_copy_constructible = false;
+ if (!field_info.trivially_destructible)
+ info.trivially_destructible = false;
}
+ /* Consistency check. */
+ if (has_cc_attr && info.trivially_copyable != is_pass_by_value)
+ {
+ /* DWARF CC attribute is not the same as the inferred value;
+ using the DWARF attribute. */
+ info.trivially_copyable = is_pass_by_value;
+ }
+
return info;
}