* [review] infcall, c++: collect more pass-by-reference information
2019-10-18 13:53 [review] infcall, c++: collect more pass-by-reference information Tankut Baris Aktemur (Code Review)
@ 2019-10-18 14:08 ` Tankut Baris Aktemur (Code Review)
2019-10-29 22:13 ` Tom Tromey (Code Review)
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Tankut Baris Aktemur (Code Review) @ 2019-10-18 14:08 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Tankut Baris Aktemur has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137
......................................................................
Patch Set 1:
v2 of this patch: https://sourceware.org/ml/gdb-patches/2019-06/msg00461.html
with your comments addressed.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [review] infcall, c++: collect more pass-by-reference information
2019-10-18 13:53 [review] infcall, c++: collect more pass-by-reference information Tankut Baris Aktemur (Code Review)
2019-10-18 14:08 ` Tankut Baris Aktemur (Code Review)
@ 2019-10-29 22:13 ` Tom Tromey (Code Review)
2019-11-12 15:22 ` Tankut Baris Aktemur (Code Review)
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Tom Tromey (Code Review) @ 2019-10-29 22:13 UTC (permalink / raw)
To: Tankut Baris Aktemur, gdb-patches; +Cc: Andrew Burgess
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137
......................................................................
Patch Set 1: Code-Review+1
(3 comments)
Thank you. I appreciate what you've done here (in particular the comment
on the test case, but really all of it looks quite good).
There were some nits in this one so I am marking it +1.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137/1/gdb/gnu-v3-abi.c
File gdb/gnu-v3-abi.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137/1/gdb/gnu-v3-abi.c@1321
PS1, Line 1321:
1296 | is_copy_or_move_constructor_type (struct type *class_type,
| ...
1316 | then this is not a copy or move constructor, but just a
1317 | constructor. */
1318 | for (int i = 2; i < TYPE_NFIELDS (method_type); i++)
1319 | {
1320 | arg_type = TYPE_FIELD_TYPE (method_type, i);
1321 | /* FIXME taktemur/2019-04-23: As of this date, neither
1322 | clang++-7.0.0 nor g++-8.2.0 produce a DW_AT_default_value
1323 | attribute. GDB is also not set to read this attribute, yet.
1324 | Hence, we immediately return false if there are more than
1325 | 2 parameters. */
It would be good to ensure that there is a gcc bug report
for this, and then link to the bug from this comment.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137/1/gdb/gnu-v3-abi.c@1413
PS1, Line 1413:
1371 | gnuv3_pass_by_reference (struct type *type)
| ...
1404 | has_cc_attr = true;
1405 | is_pass_by_value = false;
1406 | }
1407 |
1408 | /* A dynamic class has a non-trivial copy constructor.
1409 | See c++98 section 12.8 Copying class objects [class.copy]. */
1410 | if (gnuv3_dynamic_class (type))
1411 | is_dynamic = true;
1412 |
1413 | /* FIXME taktemur/2019-04-23: What if there are multiple copy ctors?
I think it would be good to answer this before landing.
Maybe we need to do overload resolution?
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137/1/gdb/gnu-v3-abi.c@1495
PS1, Line 1495:
1371 | gnuv3_pass_by_reference (struct type *type)
| ...
1486 | about recursive loops here, since we are only looking at members
1487 | of complete class type. Also ignore any static members. */
1488 | for (fieldnum = 0; fieldnum < TYPE_NFIELDS (type); fieldnum++)
1489 | if (!field_is_static (&TYPE_FIELD (type, fieldnum)))
1490 | {
1491 | struct type *field_type = TYPE_FIELD_TYPE (type, fieldnum);
1492 |
1493 | /* For arrays, make the decision based on the element type. */
1494 | if (TYPE_CODE (field_type) == TYPE_CODE_ARRAY)
1495 | field_type = field_type->main_type->target_type;
I think it's more usual to use the TYPE_TARGET_TYPE
accessor here. Also normally one must call check_typedef
on the result.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
Gerrit-Change-Number: 137
Gerrit-PatchSet: 1
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 29 Oct 2019 22:13:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 9+ messages in thread
* [review] infcall, c++: collect more pass-by-reference information
2019-10-18 13:53 [review] infcall, c++: collect more pass-by-reference information Tankut Baris Aktemur (Code Review)
2019-10-18 14:08 ` Tankut Baris Aktemur (Code Review)
2019-10-29 22:13 ` Tom Tromey (Code Review)
@ 2019-11-12 15:22 ` Tankut Baris Aktemur (Code Review)
2019-12-09 17:45 ` [review v2] " Tankut Baris Aktemur (Code Review)
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Tankut Baris Aktemur (Code Review) @ 2019-11-12 15:22 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey, Andrew Burgess
Tankut Baris Aktemur has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137
......................................................................
Patch Set 1:
(1 comment)
| --- gdb/gnu-v3-abi.c
| +++ gdb/gnu-v3-abi.c
| @@ -1272,9 +1409,14 @@ gnuv3_pass_by_reference (struct type *type)
| 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 copy ctors?
PS1, Line 1413:
Thanks for the suggestion. I think overload resolution will work. It
will thus increase the coverage of this series, and also simplify the
definition of the `language_pass_by_ref_info` struct.
There is one problem, though. GDB picks the move constructor as a
result of overload resolution, instead of the copy constructor. I
detected that there is a bug in GDB's ranking mechanism. I'll propose
a patch to fix it. For the moment I'm keeping this series on hold.
| + E.g.:
| + class C {
| + public:
| + C (C &c) { ... }
| + C (const C &c) { ... }
| + };
| + */
| for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
| for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
Gerrit-Change-Number: 137
Gerrit-PatchSet: 1
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 12 Nov 2019 15:22:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 9+ messages in thread
* [review v2] infcall, c++: collect more pass-by-reference information
2019-10-18 13:53 [review] infcall, c++: collect more pass-by-reference information Tankut Baris Aktemur (Code Review)
` (2 preceding siblings ...)
2019-11-12 15:22 ` Tankut Baris Aktemur (Code Review)
@ 2019-12-09 17:45 ` Tankut Baris Aktemur (Code Review)
2019-12-09 17:55 ` Tankut Baris Aktemur (Code Review)
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Tankut Baris Aktemur (Code Review) @ 2019-12-09 17:45 UTC (permalink / raw)
To: Andrew Burgess, Tom Tromey, gdb-patches
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137
......................................................................
infcall, c++: collect more pass-by-reference information
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:
2019-MM-DD Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gnu-v3-abi.c (enum definition_style): New enum type.
(get_def_style): New function.
(is_user_provided_def): New function.
(is_implicit_def): New function.
(is_copy_or_move_constructor_type): New function.
(is_copy_constructor_type): New function.
(is_move_constructor_type): New function.
(gnuv3_pass_by_reference): Collect language_pass_by_ref_info
for a given type.
Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
---
M gdb/gnu-v3-abi.c
1 file changed, 236 insertions(+), 51 deletions(-)
diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 35197e5..33be218 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"
@@ -1230,6 +1231,127 @@
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 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 aktemur/2019-10-31: 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.
+ GCC bug link:
+ https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42959
+ */
+ 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
@@ -1238,16 +1360,15 @@
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)
@@ -1260,21 +1381,39 @@
struct language_pass_by_ref_info info
= default_pass_by_reference (type);
- /* FIXME: Currently, this implementation only fills in the
- 'trivially-copyable' field to preserve GDB's existing behavior. */
+ 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.
+ Note: GCC does not produce this attribute as of version 9.2.1.
+ Bug link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92418 */
+ 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;
for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
@@ -1284,49 +1423,75 @@
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.
+ There should be at most one dtor definition. */
+ gdb_assert (dtor_def == DOES_NOT_EXIST_IN_SOURCE);
+ dtor_def = get_def_style (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;
- }
+ /* There may be more than one cctors. E.g.: one that
+ take a const parameter and another that takes a
+ non-const parameter. Such as:
+
+ class K {
+ K (const K &k)...
+ K (K &k)...
+ };
+
+ It is sufficient for the type to be non-trivial
+ even only one of the cctors is explicit.
+ Therefore, update the cctor_def value in the
+ implicit -> explicit direction, not backwards. */
+
+ if (is_implicit_def (cctor_def))
+ cctor_def = get_def_style (fn, fieldelem);
+ }
+ else if (is_move_constructor_type (type, fieldtype))
+ {
+ /* Again, there may be multiple move ctors. Update the
+ mctor_def value if we found an explicit def and the
+ existing one is not explicit. Otherwise retain the
+ existing value. */
+ if (is_implicit_def (mctor_def))
+ 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
@@ -1337,15 +1502,35 @@
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 = check_typedef (TYPE_TARGET_TYPE (field_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;
+ use the DWARF attribute. */
+ info.trivially_copyable = is_pass_by_value;
+ }
+
return info;
}
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
Gerrit-Change-Number: 137
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset
^ permalink raw reply [flat|nested] 9+ messages in thread
* [review v2] infcall, c++: collect more pass-by-reference information
2019-10-18 13:53 [review] infcall, c++: collect more pass-by-reference information Tankut Baris Aktemur (Code Review)
` (3 preceding siblings ...)
2019-12-09 17:45 ` [review v2] " Tankut Baris Aktemur (Code Review)
@ 2019-12-09 17:55 ` Tankut Baris Aktemur (Code Review)
2019-12-13 21:33 ` Tom Tromey (Code Review)
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Tankut Baris Aktemur (Code Review) @ 2019-12-09 17:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey, Andrew Burgess
Tankut Baris Aktemur has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137
......................................................................
Patch Set 2:
(3 comments)
| --- gdb/gnu-v3-abi.c
| +++ gdb/gnu-v3-abi.c
| @@ -1233,0 +1312,23 @@ is_copy_or_move_constructor_type (struct type *class_type,
| + if (!(class_types_same_p (target, class_type)))
| + return false;
| +
| + /* ...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. */
PS1, Line 1325:
Added: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42959
(The ticket was opened by you :)
| + return false;
| + }
| +
| + return true;
| +}
| +
| +/* Return true if METHOD_TYPE is a copy ctor type for CLASS_TYPE. */
| +
| +static bool
...
| @@ -1272,9 +1409,14 @@ gnuv3_pass_by_reference (struct type *type)
| 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 copy ctors?
PS1, Line 1413:
Following the recent overload resolution fixes, this now works. The
`language_pass_by_ref_info` that was defined at
https://gnutoolchain-gerrit.osci.io/r/c/binutils-
gdb/+/136/2/gdb/language.h#146
is also simplified -- no need to carry the copy-ctor and dtor function
names anymore.
The call to overload resolution happens inside infrun.c, as part of
this patch:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/141
| + E.g.:
| + class C {
| + public:
| + C (C &c) { ... }
| + C (const C &c) { ... }
| + };
| + */
| for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
| for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
...
| @@ -1335,7 +1486,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)))
| {
| + 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;
PS1, Line 1495:
Done
| +
| 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;
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
Gerrit-Change-Number: 137
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Mon, 09 Dec 2019 17:54:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Comment-In-Reply-To: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 9+ messages in thread
* [review v2] infcall, c++: collect more pass-by-reference information
2019-10-18 13:53 [review] infcall, c++: collect more pass-by-reference information Tankut Baris Aktemur (Code Review)
` (4 preceding siblings ...)
2019-12-09 17:55 ` Tankut Baris Aktemur (Code Review)
@ 2019-12-13 21:33 ` Tom Tromey (Code Review)
2019-12-20 16:47 ` [pushed] " Sourceware to Gerrit sync (Code Review)
2019-12-20 16:53 ` Sourceware to Gerrit sync (Code Review)
7 siblings, 0 replies; 9+ messages in thread
From: Tom Tromey (Code Review) @ 2019-12-13 21:33 UTC (permalink / raw)
To: Tankut Baris Aktemur, gdb-patches; +Cc: Andrew Burgess
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137
......................................................................
Patch Set 2: Code-Review+2
Thanks for the update. This is ok.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
Gerrit-Change-Number: 137
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Fri, 13 Dec 2019 21:33:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pushed] infcall, c++: collect more pass-by-reference information
2019-10-18 13:53 [review] infcall, c++: collect more pass-by-reference information Tankut Baris Aktemur (Code Review)
` (5 preceding siblings ...)
2019-12-13 21:33 ` Tom Tromey (Code Review)
@ 2019-12-20 16:47 ` Sourceware to Gerrit sync (Code Review)
2019-12-20 16:53 ` Sourceware to Gerrit sync (Code Review)
7 siblings, 0 replies; 9+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-12-20 16:47 UTC (permalink / raw)
To: Tankut Baris Aktemur, Andrew Burgess, Tom Tromey, gdb-patches
The original change was created by Tankut Baris Aktemur.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137
......................................................................
infcall, c++: collect more pass-by-reference information
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:
2019-12-20 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gnu-v3-abi.c (enum definition_style): New enum type.
(get_def_style): New function.
(is_user_provided_def): New function.
(is_implicit_def): New function.
(is_copy_or_move_constructor_type): New function.
(is_copy_constructor_type): New function.
(is_move_constructor_type): New function.
(gnuv3_pass_by_reference): Collect language_pass_by_ref_info
for a given type.
Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
---
M gdb/ChangeLog
M gdb/gnu-v3-abi.c
2 files changed, 248 insertions(+), 51 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3e88754..43b86f2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,17 @@
2019-12-20 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
+ * gnu-v3-abi.c (enum definition_style): New enum type.
+ (get_def_style): New function.
+ (is_user_provided_def): New function.
+ (is_implicit_def): New function.
+ (is_copy_or_move_constructor_type): New function.
+ (is_copy_constructor_type): New function.
+ (is_move_constructor_type): New function.
+ (gnuv3_pass_by_reference): Collect language_pass_by_ref_info
+ for a given type.
+
+2019-12-20 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
+
* 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.
diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 35197e5..33be218 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"
@@ -1230,6 +1231,127 @@
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 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 aktemur/2019-10-31: 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.
+ GCC bug link:
+ https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42959
+ */
+ 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
@@ -1238,16 +1360,15 @@
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)
@@ -1260,21 +1381,39 @@
struct language_pass_by_ref_info info
= default_pass_by_reference (type);
- /* FIXME: Currently, this implementation only fills in the
- 'trivially-copyable' field to preserve GDB's existing behavior. */
+ 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.
+ Note: GCC does not produce this attribute as of version 9.2.1.
+ Bug link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92418 */
+ 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;
for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
@@ -1284,49 +1423,75 @@
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.
+ There should be at most one dtor definition. */
+ gdb_assert (dtor_def == DOES_NOT_EXIST_IN_SOURCE);
+ dtor_def = get_def_style (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;
- }
+ /* There may be more than one cctors. E.g.: one that
+ take a const parameter and another that takes a
+ non-const parameter. Such as:
+
+ class K {
+ K (const K &k)...
+ K (K &k)...
+ };
+
+ It is sufficient for the type to be non-trivial
+ even only one of the cctors is explicit.
+ Therefore, update the cctor_def value in the
+ implicit -> explicit direction, not backwards. */
+
+ if (is_implicit_def (cctor_def))
+ cctor_def = get_def_style (fn, fieldelem);
+ }
+ else if (is_move_constructor_type (type, fieldtype))
+ {
+ /* Again, there may be multiple move ctors. Update the
+ mctor_def value if we found an explicit def and the
+ existing one is not explicit. Otherwise retain the
+ existing value. */
+ if (is_implicit_def (mctor_def))
+ 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
@@ -1337,15 +1502,35 @@
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 = check_typedef (TYPE_TARGET_TYPE (field_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;
+ use the DWARF attribute. */
+ info.trivially_copyable = is_pass_by_value;
+ }
+
return info;
}
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
Gerrit-Change-Number: 137
Gerrit-PatchSet: 3
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pushed] infcall, c++: collect more pass-by-reference information
2019-10-18 13:53 [review] infcall, c++: collect more pass-by-reference information Tankut Baris Aktemur (Code Review)
` (6 preceding siblings ...)
2019-12-20 16:47 ` [pushed] " Sourceware to Gerrit sync (Code Review)
@ 2019-12-20 16:53 ` Sourceware to Gerrit sync (Code Review)
7 siblings, 0 replies; 9+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-12-20 16:53 UTC (permalink / raw)
To: Tankut Baris Aktemur, gdb-patches; +Cc: Tom Tromey, Andrew Burgess
Sourceware to Gerrit sync has submitted this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137
......................................................................
infcall, c++: collect more pass-by-reference information
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:
2019-12-20 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gnu-v3-abi.c (enum definition_style): New enum type.
(get_def_style): New function.
(is_user_provided_def): New function.
(is_implicit_def): New function.
(is_copy_or_move_constructor_type): New function.
(is_copy_constructor_type): New function.
(is_move_constructor_type): New function.
(gnuv3_pass_by_reference): Collect language_pass_by_ref_info
for a given type.
Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
---
M gdb/ChangeLog
M gdb/gnu-v3-abi.c
2 files changed, 248 insertions(+), 51 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3e88754..43b86f2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,17 @@
2019-12-20 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
+ * gnu-v3-abi.c (enum definition_style): New enum type.
+ (get_def_style): New function.
+ (is_user_provided_def): New function.
+ (is_implicit_def): New function.
+ (is_copy_or_move_constructor_type): New function.
+ (is_copy_constructor_type): New function.
+ (is_move_constructor_type): New function.
+ (gnuv3_pass_by_reference): Collect language_pass_by_ref_info
+ for a given type.
+
+2019-12-20 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
+
* 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.
diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 35197e5..33be218 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"
@@ -1230,6 +1231,127 @@
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 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 aktemur/2019-10-31: 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.
+ GCC bug link:
+ https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42959
+ */
+ 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
@@ -1238,16 +1360,15 @@
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)
@@ -1260,21 +1381,39 @@
struct language_pass_by_ref_info info
= default_pass_by_reference (type);
- /* FIXME: Currently, this implementation only fills in the
- 'trivially-copyable' field to preserve GDB's existing behavior. */
+ 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.
+ Note: GCC does not produce this attribute as of version 9.2.1.
+ Bug link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92418 */
+ 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;
for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
@@ -1284,49 +1423,75 @@
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.
+ There should be at most one dtor definition. */
+ gdb_assert (dtor_def == DOES_NOT_EXIST_IN_SOURCE);
+ dtor_def = get_def_style (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;
- }
+ /* There may be more than one cctors. E.g.: one that
+ take a const parameter and another that takes a
+ non-const parameter. Such as:
+
+ class K {
+ K (const K &k)...
+ K (K &k)...
+ };
+
+ It is sufficient for the type to be non-trivial
+ even only one of the cctors is explicit.
+ Therefore, update the cctor_def value in the
+ implicit -> explicit direction, not backwards. */
+
+ if (is_implicit_def (cctor_def))
+ cctor_def = get_def_style (fn, fieldelem);
+ }
+ else if (is_move_constructor_type (type, fieldtype))
+ {
+ /* Again, there may be multiple move ctors. Update the
+ mctor_def value if we found an explicit def and the
+ existing one is not explicit. Otherwise retain the
+ existing value. */
+ if (is_implicit_def (mctor_def))
+ 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
@@ -1337,15 +1502,35 @@
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 = check_typedef (TYPE_TARGET_TYPE (field_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;
+ use the DWARF attribute. */
+ info.trivially_copyable = is_pass_by_value;
+ }
+
return info;
}
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9
Gerrit-Change-Number: 137
Gerrit-PatchSet: 3
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged
^ permalink raw reply [flat|nested] 9+ messages in thread