[review] infcall, c++: collect more pass-by-reference information

Message ID gerrit.1571406803000.Ic05bd98a962d07ec3c1ad041f709687eabda3bb9@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Oct. 18, 2019, 1:53 p.m. UTC
  Tankut Baris Aktemur has uploaded a new change for review.

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, 222 insertions(+), 51 deletions(-)
  

Comments

Simon Marchi (Code Review) Oct. 18, 2019, 2:08 p.m. UTC | #1
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.
  
Simon Marchi (Code Review) Oct. 29, 2019, 10:13 p.m. UTC | #2
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.
  
Simon Marchi (Code Review) Nov. 12, 2019, 3:22 p.m. UTC | #3
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);
  
Simon Marchi (Code Review) Dec. 9, 2019, 5:54 p.m. UTC | #4
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;
  
Simon Marchi (Code Review) Dec. 13, 2019, 9:33 p.m. UTC | #5
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.
  

Patch

diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 8b3cc89..d1f8a09 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,124 @@ 
   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 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
@@ -1238,16 +1357,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,22 +1378,46 @@ 
   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.  */
+  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 copy ctors?
+     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);
 	 fieldelem++)
@@ -1284,49 +1426,58 @@ 
 	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);
+	    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;
-		  }
+		/* It's unclear how to handle multiple copy ctors.
+		   Until we figure that out, enforce a single def.  */
+		gdb_assert (cctor_def == DOES_NOT_EXIST_IN_SOURCE);
+		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
@@ -1337,15 +1488,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 = 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;
+	 use the DWARF attribute.  */
+      info.trivially_copyable = is_pass_by_value;
+    }
+
   return info;
 }