[review] infcall: handle pass-by-reference arguments appropriately

[review] infcall: handle pass-by-reference arguments appropriately

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has uploaded a new change for review.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/141
......................................................................

infcall: handle pass-by-reference arguments appropriately

If an aggregate argument is implicitly pass-by-reference, allocate a
temporary object on the stack, initialize it via the copy constructor
(if exists) or trivially by memcpy'ing.  Pass the reference of the
temporary to the callee function.  After the callee returns, invoke
the destructor of the temporary.

gdb/ChangeLog:
2019-MM-DD  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* infcall.c (call_function_by_hand_dummy): Update the argument-
	passing section for call-by-value parameters.
	(struct destructor_info): New struct.
	(call_destructors): New auxiliary function.

Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
---
M gdb/infcall.c
1 file changed, 100 insertions(+), 3 deletions(-)



diff --git a/gdb/infcall.c b/gdb/infcall.c
index 288b9e0..701df4d 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -42,6 +42,7 @@
 #include "thread-fsm.h"
 #include <algorithm>
 #include "gdbsupport/scope-exit.h"
+#include <list>
 
 /* If we can't find a function's name from its address,
    we print this instead.  */
@@ -704,6 +705,33 @@
   return addr;
 }
 
+/* The data structure which keeps a destructor function and
+   its implicit 'this' parameter.  */
+
+struct destructor_info
+{
+  destructor_info (struct value *function, struct value *self)
+    : function (function), self (self) { }
+
+  struct value *function;
+  struct value *self;
+};
+
+
+/* Auxiliary function that takes a list of destructor functions
+   with their 'this' parameters, and invokes the functions.  */
+
+static void
+call_destructors (const std::list<destructor_info> &dtors_to_invoke,
+		  struct type *default_return_type)
+{
+  for (auto vals : dtors_to_invoke)
+    {
+      call_function_by_hand (vals.function, default_return_type,
+			     gdb::make_array_view (&(vals.self), 1));
+    }
+}
+
 /* See infcall.h.  */
 
 struct value *
@@ -983,6 +1011,12 @@
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
 
+  /* Coerce the arguments and handle pass-by-reference.
+     We want to remember the destruction required for pass-by-ref values.
+     For these, store the dtor function and the 'this' argument
+     in DTORS_TO_INVOKE.  */
+  std::list<destructor_info> dtors_to_invoke;
+
   for (int i = args.size () - 1; i >= 0; i--)
     {
       int prototyped;
@@ -1020,9 +1054,68 @@
       args[i] = value_arg_coerce (gdbarch, args[i],
 				  param_type, prototyped);
 
-      if (param_type != NULL
-	  && !(language_pass_by_reference (param_type).trivially_copyable))
-	args[i] = value_addr (args[i]);
+      if (param_type == NULL)
+	continue;
+
+      auto info = language_pass_by_reference (param_type);
+      if (!info.copy_constructible)
+	error (_("expression cannot be evaluated because the type '%s' "
+		 "is not copy constructible"), TYPE_NAME (param_type));
+
+      if (!info.destructible)
+	error (_("expression cannot be evaluated because the type '%s' "
+		 "is not destructible"), TYPE_NAME (param_type));
+
+      if (info.trivially_copyable)
+	continue;
+
+      /* This is a pass-by-ref value.  Check for error cases before
+	 pushing the temporary to the stack.  */
+
+      if (!info.trivially_copy_constructible && info.cctor_name == NULL)
+	error (_("evaluation of this expression requires a copy constructor"
+		 " for the type '%s'."), TYPE_NAME (param_type));
+
+      if (!info.trivially_destructible && info.dtor_name == NULL)
+	error (_("evaluation of this expression requires a destructor"
+		 " for the type '%s'."), TYPE_NAME (param_type));
+
+      /* Make a copy of the argument on the stack.  If the argument is
+	 trivially copy ctor'able, copy bit by bit.  Otherwise, call
+	 the copy ctor to initialize the clone.  */
+      CORE_ADDR addr = reserve_stack_space (param_type, sp);
+      struct value *clone
+	= value_from_contents_and_address (param_type, NULL, addr);
+      push_thread_stack_temporary (call_thread.get (), clone);
+      struct value *clone_ptr
+	= value_from_pointer (lookup_pointer_type (param_type), addr);
+
+      if (info.trivially_copy_constructible)
+	{
+	  int length = TYPE_LENGTH (param_type);
+	  write_memory (addr, value_contents (args[i]), length);
+	}
+      else
+	{
+	  struct value *copy_ctor
+	    = find_function_in_inferior (info.cctor_name, 0);
+	  struct value *cctor_args[2] = { clone_ptr, args[i] };
+	  call_function_by_hand (copy_ctor, default_return_type,
+				 gdb::make_array_view (cctor_args, 2));
+	}
+
+      /* If the argument has a destructor, remember it so that we
+	 invoke it after the infcall is complete.  */
+      if (!info.trivially_destructible)
+	{
+	  struct value *dtor
+	    = find_function_in_inferior (info.dtor_name, 0);
+	  /* Insert the dtor to the front of the list to call them
+	     in reverse order later.  */
+	  dtors_to_invoke.emplace_front (dtor, clone_ptr);
+	}
+
+      args[i] = clone_ptr;
     }
 
   /* Reserve space for the return structure to be written on the
@@ -1189,6 +1282,10 @@
 	    maybe_remove_breakpoints ();
 
 	    gdb_assert (retval != NULL);
+
+	    /* Destruct the pass-by-ref argument clones.  */
+	    call_destructors (dtors_to_invoke, default_return_type);
+
 	    return retval;
 	  }
 

[review] infcall: handle pass-by-reference arguments appropriately

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/141
......................................................................


Patch Set 1: Code-Review+2

Thank you.  This looks reasonable to me.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
Gerrit-Change-Number: 141
Gerrit-PatchSet: 1
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 29 Oct 2019 19:54:20 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v2] infcall: handle pass-by-reference arguments appropriately

[Tankut Baris Aktemur (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/141
......................................................................

infcall: handle pass-by-reference arguments appropriately

If an aggregate argument is implicitly pass-by-reference, allocate a
temporary object on the stack, initialize it via the copy constructor
(if exists) or trivially by memcpy'ing.  Pass the reference of the
temporary to the callee function.  After the callee returns, invoke
the destructor of the temporary.

gdb/ChangeLog:
2019-MM-DD  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	PR gdb/25054
	* infcall.c (call_function_by_hand_dummy): Update the argument-
	passing section for call-by-value parameters.
	(struct destructor_info): New struct.
	(call_destructors): New auxiliary function.

Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
---
M gdb/infcall.c
1 file changed, 124 insertions(+), 3 deletions(-)



diff --git a/gdb/infcall.c b/gdb/infcall.c
index f3664d5..b6b617a 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -42,6 +42,7 @@
 #include "thread-fsm.h"
 #include <algorithm>
 #include "gdbsupport/scope-exit.h"
+#include <list>
 
 /* If we can't find a function's name from its address,
    we print this instead.  */
@@ -704,6 +705,33 @@
   return addr;
 }
 
+/* The data structure which keeps a destructor function and
+   its implicit 'this' parameter.  */
+
+struct destructor_info
+{
+  destructor_info (struct value *function, struct value *self)
+    : function (function), self (self) { }
+
+  struct value *function;
+  struct value *self;
+};
+
+
+/* Auxiliary function that takes a list of destructor functions
+   with their 'this' parameters, and invokes the functions.  */
+
+static void
+call_destructors (const std::list<destructor_info> &dtors_to_invoke,
+		  struct type *default_return_type)
+{
+  for (auto vals : dtors_to_invoke)
+    {
+      call_function_by_hand (vals.function, default_return_type,
+			     gdb::make_array_view (&(vals.self), 1));
+    }
+}
+
 /* See infcall.h.  */
 
 struct value *
@@ -983,6 +1011,12 @@
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
 
+  /* Coerce the arguments and handle pass-by-reference.
+     We want to remember the destruction required for pass-by-ref values.
+     For these, store the dtor function and the 'this' argument
+     in DTORS_TO_INVOKE.  */
+  std::list<destructor_info> dtors_to_invoke;
+
   for (int i = args.size () - 1; i >= 0; i--)
     {
       int prototyped;
@@ -1017,12 +1051,95 @@
       else
 	param_type = NULL;
 
+      value *original_arg = args[i];
       args[i] = value_arg_coerce (gdbarch, args[i],
 				  param_type, prototyped);
 
-      if (param_type != NULL
-	  && !(language_pass_by_reference (param_type).trivially_copyable))
-	args[i] = value_addr (args[i]);
+      if (param_type == NULL)
+	continue;
+
+      auto info = language_pass_by_reference (param_type);
+      if (!info.copy_constructible)
+	error (_("expression cannot be evaluated because the type '%s' "
+		 "is not copy constructible"), TYPE_NAME (param_type));
+
+      if (!info.destructible)
+	error (_("expression cannot be evaluated because the type '%s' "
+		 "is not destructible"), TYPE_NAME (param_type));
+
+      if (info.trivially_copyable)
+	continue;
+
+      /* Make a copy of the argument on the stack.  If the argument is
+	 trivially copy ctor'able, copy bit by bit.  Otherwise, call
+	 the copy ctor to initialize the clone.  */
+      CORE_ADDR addr = reserve_stack_space (param_type, sp);
+      value *clone
+	= value_from_contents_and_address (param_type, nullptr, addr);
+      push_thread_stack_temporary (call_thread.get (), clone);
+      value *clone_ptr
+	= value_from_pointer (lookup_pointer_type (param_type), addr);
+
+      if (info.trivially_copy_constructible)
+	{
+	  int length = TYPE_LENGTH (param_type);
+	  write_memory (addr, value_contents (args[i]), length);
+	}
+      else
+	{
+	  value *copy_ctor;
+	  value *cctor_args[2] = { clone_ptr, original_arg };
+	  find_overload_match (gdb::make_array_view (cctor_args, 2),
+			       TYPE_NAME (param_type), METHOD,
+			       &clone_ptr, nullptr, &copy_ctor, nullptr,
+			       nullptr, 0, EVAL_NORMAL);
+
+	  if (copy_ctor == nullptr)
+	    error (_("expression cannot be evaluated because a copy "
+		     "constructor for the type '%s' could not be found "
+		     "(maybe inlined?)"), TYPE_NAME (param_type));
+
+	  call_function_by_hand (copy_ctor, default_return_type,
+				 gdb::make_array_view (cctor_args, 2));
+	}
+
+      /* If the argument has a destructor, remember it so that we
+	 invoke it after the infcall is complete.  */
+      if (!info.trivially_destructible)
+	{
+	  /* Looking up the function via overload resolution does not
+	     work because the compiler (in particular, gcc) adds an
+	     artificial int parameter in some cases.  So we look up
+	     the function by using the "~" name.  This should be OK
+	     because there can be only one dtor definition.  */
+	  const char *dtor_name = nullptr;
+	  for (int fieldnum = 0;
+	       fieldnum < TYPE_NFN_FIELDS (param_type);
+	       fieldnum++)
+	    {
+	      fn_field *fn
+		= TYPE_FN_FIELDLIST1 (param_type, fieldnum);
+	      const char *field_name
+		= TYPE_FN_FIELDLIST_NAME (param_type, fieldnum);
+
+	      if (field_name[0] == '~')
+		dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, 0);
+	    }
+
+	  if (dtor_name == nullptr)
+	    error (_("expression cannot be evaluated because a destructor "
+		     "for the type '%s' could not be found "
+		     "(maybe inlined?)"), TYPE_NAME (param_type));
+
+	  value *dtor
+	    = find_function_in_inferior (dtor_name, 0);
+
+	  /* Insert the dtor to the front of the list to call them
+	     in reverse order later.  */
+	  dtors_to_invoke.emplace_front (dtor, clone_ptr);
+	}
+
+      args[i] = clone_ptr;
     }
 
   /* Reserve space for the return structure to be written on the
@@ -1189,6 +1306,10 @@
 	    maybe_remove_breakpoints ();
 
 	    gdb_assert (retval != NULL);
+
+	    /* Destruct the pass-by-ref argument clones.  */
+	    call_destructors (dtors_to_invoke, default_return_type);
+
 	    return retval;
 	  }
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
Gerrit-Change-Number: 141
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[review v2] infcall: handle pass-by-reference arguments appropriately

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/141
......................................................................


Patch Set 2:

The revised patch uses overload resolution to pick the right copy constructor instance.  I think there is sufficient enough modification to justify a re-approval.  Could you please re-review?

Thank you.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
Gerrit-Change-Number: 141
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.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:59:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infcall: handle pass-by-reference arguments appropriately

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/141
......................................................................


Patch Set 2: Code-Review+2

FAOD this is still ok.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
Gerrit-Change-Number: 141
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.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:35:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[pushed] infcall: handle pass-by-reference arguments appropriately

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Tankut Baris Aktemur.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/141
......................................................................

infcall: handle pass-by-reference arguments appropriately

If an aggregate argument is implicitly pass-by-reference, allocate a
temporary object on the stack, initialize it via the copy constructor
(if exists) or trivially by memcpy'ing.  Pass the reference of the
temporary to the callee function.  After the callee returns, invoke
the destructor of the temporary.

gdb/ChangeLog:
2019-12-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	PR gdb/25054
	* infcall.c (call_function_by_hand_dummy): Update the argument-
	passing section for call-by-value parameters.
	(struct destructor_info): New struct.
	(call_destructors): New auxiliary function.

Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
---
M gdb/ChangeLog
M gdb/infcall.c
2 files changed, 132 insertions(+), 3 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 43b86f2..c2157e4 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2019-12-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
 
+	PR gdb/25054
+	* infcall.c (call_function_by_hand_dummy): Update the argument-
+	passing section for call-by-value parameters.
+	(struct destructor_info): New struct.
+	(call_destructors): New auxiliary function.
+
+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.
diff --git a/gdb/infcall.c b/gdb/infcall.c
index f3664d5..b6b617a 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -42,6 +42,7 @@
 #include "thread-fsm.h"
 #include <algorithm>
 #include "gdbsupport/scope-exit.h"
+#include <list>
 
 /* If we can't find a function's name from its address,
    we print this instead.  */
@@ -704,6 +705,33 @@
   return addr;
 }
 
+/* The data structure which keeps a destructor function and
+   its implicit 'this' parameter.  */
+
+struct destructor_info
+{
+  destructor_info (struct value *function, struct value *self)
+    : function (function), self (self) { }
+
+  struct value *function;
+  struct value *self;
+};
+
+
+/* Auxiliary function that takes a list of destructor functions
+   with their 'this' parameters, and invokes the functions.  */
+
+static void
+call_destructors (const std::list<destructor_info> &dtors_to_invoke,
+		  struct type *default_return_type)
+{
+  for (auto vals : dtors_to_invoke)
+    {
+      call_function_by_hand (vals.function, default_return_type,
+			     gdb::make_array_view (&(vals.self), 1));
+    }
+}
+
 /* See infcall.h.  */
 
 struct value *
@@ -983,6 +1011,12 @@
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
 
+  /* Coerce the arguments and handle pass-by-reference.
+     We want to remember the destruction required for pass-by-ref values.
+     For these, store the dtor function and the 'this' argument
+     in DTORS_TO_INVOKE.  */
+  std::list<destructor_info> dtors_to_invoke;
+
   for (int i = args.size () - 1; i >= 0; i--)
     {
       int prototyped;
@@ -1017,12 +1051,95 @@
       else
 	param_type = NULL;
 
+      value *original_arg = args[i];
       args[i] = value_arg_coerce (gdbarch, args[i],
 				  param_type, prototyped);
 
-      if (param_type != NULL
-	  && !(language_pass_by_reference (param_type).trivially_copyable))
-	args[i] = value_addr (args[i]);
+      if (param_type == NULL)
+	continue;
+
+      auto info = language_pass_by_reference (param_type);
+      if (!info.copy_constructible)
+	error (_("expression cannot be evaluated because the type '%s' "
+		 "is not copy constructible"), TYPE_NAME (param_type));
+
+      if (!info.destructible)
+	error (_("expression cannot be evaluated because the type '%s' "
+		 "is not destructible"), TYPE_NAME (param_type));
+
+      if (info.trivially_copyable)
+	continue;
+
+      /* Make a copy of the argument on the stack.  If the argument is
+	 trivially copy ctor'able, copy bit by bit.  Otherwise, call
+	 the copy ctor to initialize the clone.  */
+      CORE_ADDR addr = reserve_stack_space (param_type, sp);
+      value *clone
+	= value_from_contents_and_address (param_type, nullptr, addr);
+      push_thread_stack_temporary (call_thread.get (), clone);
+      value *clone_ptr
+	= value_from_pointer (lookup_pointer_type (param_type), addr);
+
+      if (info.trivially_copy_constructible)
+	{
+	  int length = TYPE_LENGTH (param_type);
+	  write_memory (addr, value_contents (args[i]), length);
+	}
+      else
+	{
+	  value *copy_ctor;
+	  value *cctor_args[2] = { clone_ptr, original_arg };
+	  find_overload_match (gdb::make_array_view (cctor_args, 2),
+			       TYPE_NAME (param_type), METHOD,
+			       &clone_ptr, nullptr, &copy_ctor, nullptr,
+			       nullptr, 0, EVAL_NORMAL);
+
+	  if (copy_ctor == nullptr)
+	    error (_("expression cannot be evaluated because a copy "
+		     "constructor for the type '%s' could not be found "
+		     "(maybe inlined?)"), TYPE_NAME (param_type));
+
+	  call_function_by_hand (copy_ctor, default_return_type,
+				 gdb::make_array_view (cctor_args, 2));
+	}
+
+      /* If the argument has a destructor, remember it so that we
+	 invoke it after the infcall is complete.  */
+      if (!info.trivially_destructible)
+	{
+	  /* Looking up the function via overload resolution does not
+	     work because the compiler (in particular, gcc) adds an
+	     artificial int parameter in some cases.  So we look up
+	     the function by using the "~" name.  This should be OK
+	     because there can be only one dtor definition.  */
+	  const char *dtor_name = nullptr;
+	  for (int fieldnum = 0;
+	       fieldnum < TYPE_NFN_FIELDS (param_type);
+	       fieldnum++)
+	    {
+	      fn_field *fn
+		= TYPE_FN_FIELDLIST1 (param_type, fieldnum);
+	      const char *field_name
+		= TYPE_FN_FIELDLIST_NAME (param_type, fieldnum);
+
+	      if (field_name[0] == '~')
+		dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, 0);
+	    }
+
+	  if (dtor_name == nullptr)
+	    error (_("expression cannot be evaluated because a destructor "
+		     "for the type '%s' could not be found "
+		     "(maybe inlined?)"), TYPE_NAME (param_type));
+
+	  value *dtor
+	    = find_function_in_inferior (dtor_name, 0);
+
+	  /* Insert the dtor to the front of the list to call them
+	     in reverse order later.  */
+	  dtors_to_invoke.emplace_front (dtor, clone_ptr);
+	}
+
+      args[i] = clone_ptr;
     }
 
   /* Reserve space for the return structure to be written on the
@@ -1189,6 +1306,10 @@
 	    maybe_remove_breakpoints ();
 
 	    gdb_assert (retval != NULL);
+
+	    /* Destruct the pass-by-ref argument clones.  */
+	    call_destructors (dtors_to_invoke, default_return_type);
+
 	    return retval;
 	  }
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
Gerrit-Change-Number: 141
Gerrit-PatchSet: 3
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[pushed] infcall: handle pass-by-reference arguments appropriately

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/141
......................................................................

infcall: handle pass-by-reference arguments appropriately

If an aggregate argument is implicitly pass-by-reference, allocate a
temporary object on the stack, initialize it via the copy constructor
(if exists) or trivially by memcpy'ing.  Pass the reference of the
temporary to the callee function.  After the callee returns, invoke
the destructor of the temporary.

gdb/ChangeLog:
2019-12-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	PR gdb/25054
	* infcall.c (call_function_by_hand_dummy): Update the argument-
	passing section for call-by-value parameters.
	(struct destructor_info): New struct.
	(call_destructors): New auxiliary function.

Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
---
M gdb/ChangeLog
M gdb/infcall.c
2 files changed, 132 insertions(+), 3 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 43b86f2..c2157e4 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2019-12-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
 
+	PR gdb/25054
+	* infcall.c (call_function_by_hand_dummy): Update the argument-
+	passing section for call-by-value parameters.
+	(struct destructor_info): New struct.
+	(call_destructors): New auxiliary function.
+
+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.
diff --git a/gdb/infcall.c b/gdb/infcall.c
index f3664d5..b6b617a 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -42,6 +42,7 @@
 #include "thread-fsm.h"
 #include <algorithm>
 #include "gdbsupport/scope-exit.h"
+#include <list>
 
 /* If we can't find a function's name from its address,
    we print this instead.  */
@@ -704,6 +705,33 @@
   return addr;
 }
 
+/* The data structure which keeps a destructor function and
+   its implicit 'this' parameter.  */
+
+struct destructor_info
+{
+  destructor_info (struct value *function, struct value *self)
+    : function (function), self (self) { }
+
+  struct value *function;
+  struct value *self;
+};
+
+
+/* Auxiliary function that takes a list of destructor functions
+   with their 'this' parameters, and invokes the functions.  */
+
+static void
+call_destructors (const std::list<destructor_info> &dtors_to_invoke,
+		  struct type *default_return_type)
+{
+  for (auto vals : dtors_to_invoke)
+    {
+      call_function_by_hand (vals.function, default_return_type,
+			     gdb::make_array_view (&(vals.self), 1));
+    }
+}
+
 /* See infcall.h.  */
 
 struct value *
@@ -983,6 +1011,12 @@
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
 
+  /* Coerce the arguments and handle pass-by-reference.
+     We want to remember the destruction required for pass-by-ref values.
+     For these, store the dtor function and the 'this' argument
+     in DTORS_TO_INVOKE.  */
+  std::list<destructor_info> dtors_to_invoke;
+
   for (int i = args.size () - 1; i >= 0; i--)
     {
       int prototyped;
@@ -1017,12 +1051,95 @@
       else
 	param_type = NULL;
 
+      value *original_arg = args[i];
       args[i] = value_arg_coerce (gdbarch, args[i],
 				  param_type, prototyped);
 
-      if (param_type != NULL
-	  && !(language_pass_by_reference (param_type).trivially_copyable))
-	args[i] = value_addr (args[i]);
+      if (param_type == NULL)
+	continue;
+
+      auto info = language_pass_by_reference (param_type);
+      if (!info.copy_constructible)
+	error (_("expression cannot be evaluated because the type '%s' "
+		 "is not copy constructible"), TYPE_NAME (param_type));
+
+      if (!info.destructible)
+	error (_("expression cannot be evaluated because the type '%s' "
+		 "is not destructible"), TYPE_NAME (param_type));
+
+      if (info.trivially_copyable)
+	continue;
+
+      /* Make a copy of the argument on the stack.  If the argument is
+	 trivially copy ctor'able, copy bit by bit.  Otherwise, call
+	 the copy ctor to initialize the clone.  */
+      CORE_ADDR addr = reserve_stack_space (param_type, sp);
+      value *clone
+	= value_from_contents_and_address (param_type, nullptr, addr);
+      push_thread_stack_temporary (call_thread.get (), clone);
+      value *clone_ptr
+	= value_from_pointer (lookup_pointer_type (param_type), addr);
+
+      if (info.trivially_copy_constructible)
+	{
+	  int length = TYPE_LENGTH (param_type);
+	  write_memory (addr, value_contents (args[i]), length);
+	}
+      else
+	{
+	  value *copy_ctor;
+	  value *cctor_args[2] = { clone_ptr, original_arg };
+	  find_overload_match (gdb::make_array_view (cctor_args, 2),
+			       TYPE_NAME (param_type), METHOD,
+			       &clone_ptr, nullptr, &copy_ctor, nullptr,
+			       nullptr, 0, EVAL_NORMAL);
+
+	  if (copy_ctor == nullptr)
+	    error (_("expression cannot be evaluated because a copy "
+		     "constructor for the type '%s' could not be found "
+		     "(maybe inlined?)"), TYPE_NAME (param_type));
+
+	  call_function_by_hand (copy_ctor, default_return_type,
+				 gdb::make_array_view (cctor_args, 2));
+	}
+
+      /* If the argument has a destructor, remember it so that we
+	 invoke it after the infcall is complete.  */
+      if (!info.trivially_destructible)
+	{
+	  /* Looking up the function via overload resolution does not
+	     work because the compiler (in particular, gcc) adds an
+	     artificial int parameter in some cases.  So we look up
+	     the function by using the "~" name.  This should be OK
+	     because there can be only one dtor definition.  */
+	  const char *dtor_name = nullptr;
+	  for (int fieldnum = 0;
+	       fieldnum < TYPE_NFN_FIELDS (param_type);
+	       fieldnum++)
+	    {
+	      fn_field *fn
+		= TYPE_FN_FIELDLIST1 (param_type, fieldnum);
+	      const char *field_name
+		= TYPE_FN_FIELDLIST_NAME (param_type, fieldnum);
+
+	      if (field_name[0] == '~')
+		dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, 0);
+	    }
+
+	  if (dtor_name == nullptr)
+	    error (_("expression cannot be evaluated because a destructor "
+		     "for the type '%s' could not be found "
+		     "(maybe inlined?)"), TYPE_NAME (param_type));
+
+	  value *dtor
+	    = find_function_in_inferior (dtor_name, 0);
+
+	  /* Insert the dtor to the front of the list to call them
+	     in reverse order later.  */
+	  dtors_to_invoke.emplace_front (dtor, clone_ptr);
+	}
+
+      args[i] = clone_ptr;
     }
 
   /* Reserve space for the return structure to be written on the
@@ -1189,6 +1306,10 @@
 	    maybe_remove_breakpoints ();
 
 	    gdb_assert (retval != NULL);
+
+	    /* Destruct the pass-by-ref argument clones.  */
+	    call_destructors (dtors_to_invoke, default_return_type);
+
 	    return retval;
 	  }
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I18fa5d0df814dfa0defe9e862a88a6dbf1d99d01
Gerrit-Change-Number: 141
Gerrit-PatchSet: 3
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged