[review] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[review] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[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/+/139
......................................................................

infcall: move assertions in 'call_function_by_hand_dummy' to an earlier spot

This is a refactoring that performs type assertions on the callee
function at the beginning of 'call_function_by_hand_dummy' rather than
at a later point.

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

	* infcall.c (call_function_by_hand_dummy): Refactor.

Change-Id: I411ac083ac6a9ee6eb93c4b82393a81a4fc927be
---
M gdb/infcall.c
1 file changed, 21 insertions(+), 20 deletions(-)



diff --git a/gdb/infcall.c b/gdb/infcall.c
index a342310..629a83c 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -782,6 +782,27 @@
   if (!gdbarch_push_dummy_call_p (gdbarch))
     error (_("This target does not support function calls."));
 
+  /* Find the function type and do a sanity check.  */
+  type *ftype;
+  type *values_type;
+  CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
+
+  if (values_type == NULL)
+    values_type = default_return_type;
+  if (values_type == NULL)
+    {
+      const char *name = get_function_name (funaddr,
+					    name_buf, sizeof (name_buf));
+      error (_("'%s' has unknown return type; "
+	       "cast the call to its declared return type"),
+	     name);
+    }
+
+  values_type = check_typedef (values_type);
+
+  if (args.size () < TYPE_NFIELDS (ftype))
+    error (_("Too few arguments in function call."));
+
   /* A holder for the inferior status.
      This is only needed while we're preparing the inferior function call.  */
   infcall_control_state_up inf_status (save_infcall_control_state ());
@@ -887,23 +908,6 @@
       }
   }
 
-  type *ftype;
-  type *values_type;
-  CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
-
-  if (values_type == NULL)
-    values_type = default_return_type;
-  if (values_type == NULL)
-    {
-      const char *name = get_function_name (funaddr,
-					    name_buf, sizeof (name_buf));
-      error (_("'%s' has unknown return type; "
-	       "cast the call to its declared return type"),
-	     name);
-    }
-
-  values_type = check_typedef (values_type);
-
   /* Are we returning a value using a structure return?  */
 
   if (gdbarch_return_in_first_hidden_param_p (gdbarch, values_type))
@@ -981,9 +985,6 @@
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
 
-  if (args.size () < TYPE_NFIELDS (ftype))
-    error (_("Too few arguments in function call."));
-
   for (int i = args.size () - 1; i >= 0; i--)
     {
       int prototyped;

[review] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

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


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/139/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/139/1//COMMIT_MSG@12 
PS1, Line 12: 
Can you make it explicit here why this is a good idea?

[review] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[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/+/139
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/139/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/139/1//COMMIT_MSG@12 
PS1, Line 12: 
> Can you make it explicit here why this is a good idea?
How about this? "... so that (1) the checks are grouped together at the beginning of the function for improved readability, and (2) we don't have to align and push things on the stack only to find out later that the function call is illegal."

[review] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

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


Patch Set 1: Code-Review+1

I'd make it clear, in the cover letter/commit message, the reason for refactoring. As for the explanation, it sounds good to me.

I agree we should check all the easy things before doing any further processing towards a function call. This looks like a small optimization of sorts.

[review] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

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


Patch Set 1:

> Patch Set 1: Code-Review+1
> 
> I'd make it clear, in the cover letter/commit message, the reason for refactoring. As for the explanation, it sounds good to me.
> 
> I agree we should check all the easy things before doing any further processing towards a function call. This looks like a small optimization of sorts.

Agreed, please put this in the commit message.

[review v2] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[Tankut Baris Aktemur (Code Review)]

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

infcall: move assertions in 'call_function_by_hand_dummy' to an earlier spot

This is a refactoring that performs type assertions on the callee
function at the beginning of 'call_function_by_hand_dummy' rather than
at a later point so that

- the checks are grouped together at the beginning of the function for
improved readability, and

- we don't have to align and push things on the stack only to find out
later that the function call is illegal.

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

	* infcall.c (call_function_by_hand_dummy): Refactor.

Change-Id: I411ac083ac6a9ee6eb93c4b82393a81a4fc927be
---
M gdb/ChangeLog
M gdb/infcall.c
2 files changed, 25 insertions(+), 20 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a70f3e1..fb15f2b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2019-10-22  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
+
+	* infcall.c (call_function_by_hand_dummy): Refactor.
+
 2019-10-21  Ali Tamur  <tamu@google.com>
 
 	* dwarf2read.c (dir_index): Change type.
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 726f14d..0d8d5b2 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -746,6 +746,27 @@
   if (!gdbarch_push_dummy_call_p (gdbarch))
     error (_("This target does not support function calls."));
 
+  /* Find the function type and do a sanity check.  */
+  type *ftype;
+  type *values_type;
+  CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
+
+  if (values_type == NULL)
+    values_type = default_return_type;
+  if (values_type == NULL)
+    {
+      const char *name = get_function_name (funaddr,
+					    name_buf, sizeof (name_buf));
+      error (_("'%s' has unknown return type; "
+	       "cast the call to its declared return type"),
+	     name);
+    }
+
+  values_type = check_typedef (values_type);
+
+  if (args.size () < TYPE_NFIELDS (ftype))
+    error (_("Too few arguments in function call."));
+
   /* A holder for the inferior status.
      This is only needed while we're preparing the inferior function call.  */
   infcall_control_state_up inf_status (save_infcall_control_state ());
@@ -851,23 +872,6 @@
       }
   }
 
-  type *ftype;
-  type *values_type;
-  CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
-
-  if (values_type == NULL)
-    values_type = default_return_type;
-  if (values_type == NULL)
-    {
-      const char *name = get_function_name (funaddr,
-					    name_buf, sizeof (name_buf));
-      error (_("'%s' has unknown return type; "
-	       "cast the call to its declared return type"),
-	     name);
-    }
-
-  values_type = check_typedef (values_type);
-
   /* Are we returning a value using a structure return?  */
 
   if (gdbarch_return_in_first_hidden_param_p (gdbarch, values_type))
@@ -945,9 +949,6 @@
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
 
-  if (args.size () < TYPE_NFIELDS (ftype))
-    error (_("Too few arguments in function call."));
-
   for (int i = args.size () - 1; i >= 0; i--)
     {
       int prototyped;

[review v2] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[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/+/139
......................................................................


Patch Set 2:

> Agreed, please put this in the commit message.

I updated the commit message and also included the ChangeLog changes.  Thanks.

[review v2] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

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


Patch Set 2: Code-Review+1

LGTM

[review v2] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

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


Patch Set 2: Code-Review+2

Thanks, LGTM too.

I've seen that you are already in the process of getting write access, this is ok to push once you have it.

[pushed] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[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/+/139
......................................................................

infcall: move assertions in 'call_function_by_hand_dummy' to an earlier spot

This is a refactoring that performs type assertions on the callee
function at the beginning of 'call_function_by_hand_dummy' rather than
at a later point so that

- the checks are grouped together at the beginning of the function for
improved readability, and

- we don't have to align and push things on the stack only to find out
later that the function call is illegal.

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

	* infcall.c (call_function_by_hand_dummy): Refactor.

Change-Id: I411ac083ac6a9ee6eb93c4b82393a81a4fc927be
---
M gdb/ChangeLog
M gdb/infcall.c
2 files changed, 25 insertions(+), 20 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 53f50e2..6fafc44 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2019-10-23  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
 
+	* infcall.c (call_function_by_hand_dummy): Refactor.
+
+2019-10-23  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
+
 	* MAINTAINERS (Write After Approval): Add Tankut Baris Aktemur.
 
 2019-10-23  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 726f14d..0d8d5b2 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -746,6 +746,27 @@
   if (!gdbarch_push_dummy_call_p (gdbarch))
     error (_("This target does not support function calls."));
 
+  /* Find the function type and do a sanity check.  */
+  type *ftype;
+  type *values_type;
+  CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
+
+  if (values_type == NULL)
+    values_type = default_return_type;
+  if (values_type == NULL)
+    {
+      const char *name = get_function_name (funaddr,
+					    name_buf, sizeof (name_buf));
+      error (_("'%s' has unknown return type; "
+	       "cast the call to its declared return type"),
+	     name);
+    }
+
+  values_type = check_typedef (values_type);
+
+  if (args.size () < TYPE_NFIELDS (ftype))
+    error (_("Too few arguments in function call."));
+
   /* A holder for the inferior status.
      This is only needed while we're preparing the inferior function call.  */
   infcall_control_state_up inf_status (save_infcall_control_state ());
@@ -851,23 +872,6 @@
       }
   }
 
-  type *ftype;
-  type *values_type;
-  CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
-
-  if (values_type == NULL)
-    values_type = default_return_type;
-  if (values_type == NULL)
-    {
-      const char *name = get_function_name (funaddr,
-					    name_buf, sizeof (name_buf));
-      error (_("'%s' has unknown return type; "
-	       "cast the call to its declared return type"),
-	     name);
-    }
-
-  values_type = check_typedef (values_type);
-
   /* Are we returning a value using a structure return?  */
 
   if (gdbarch_return_in_first_hidden_param_p (gdbarch, values_type))
@@ -945,9 +949,6 @@
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
 
-  if (args.size () < TYPE_NFIELDS (ftype))
-    error (_("Too few arguments in function call."));
-
   for (int i = args.size () - 1; i >= 0; i--)
     {
       int prototyped;

[pushed] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...

[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/+/139
......................................................................

infcall: move assertions in 'call_function_by_hand_dummy' to an earlier spot

This is a refactoring that performs type assertions on the callee
function at the beginning of 'call_function_by_hand_dummy' rather than
at a later point so that

- the checks are grouped together at the beginning of the function for
improved readability, and

- we don't have to align and push things on the stack only to find out
later that the function call is illegal.

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

	* infcall.c (call_function_by_hand_dummy): Refactor.

Change-Id: I411ac083ac6a9ee6eb93c4b82393a81a4fc927be
---
M gdb/ChangeLog
M gdb/infcall.c
2 files changed, 25 insertions(+), 20 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 53f50e2..6fafc44 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2019-10-23  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
 
+	* infcall.c (call_function_by_hand_dummy): Refactor.
+
+2019-10-23  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
+
 	* MAINTAINERS (Write After Approval): Add Tankut Baris Aktemur.
 
 2019-10-23  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 726f14d..0d8d5b2 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -746,6 +746,27 @@
   if (!gdbarch_push_dummy_call_p (gdbarch))
     error (_("This target does not support function calls."));
 
+  /* Find the function type and do a sanity check.  */
+  type *ftype;
+  type *values_type;
+  CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
+
+  if (values_type == NULL)
+    values_type = default_return_type;
+  if (values_type == NULL)
+    {
+      const char *name = get_function_name (funaddr,
+					    name_buf, sizeof (name_buf));
+      error (_("'%s' has unknown return type; "
+	       "cast the call to its declared return type"),
+	     name);
+    }
+
+  values_type = check_typedef (values_type);
+
+  if (args.size () < TYPE_NFIELDS (ftype))
+    error (_("Too few arguments in function call."));
+
   /* A holder for the inferior status.
      This is only needed while we're preparing the inferior function call.  */
   infcall_control_state_up inf_status (save_infcall_control_state ());
@@ -851,23 +872,6 @@
       }
   }
 
-  type *ftype;
-  type *values_type;
-  CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
-
-  if (values_type == NULL)
-    values_type = default_return_type;
-  if (values_type == NULL)
-    {
-      const char *name = get_function_name (funaddr,
-					    name_buf, sizeof (name_buf));
-      error (_("'%s' has unknown return type; "
-	       "cast the call to its declared return type"),
-	     name);
-    }
-
-  values_type = check_typedef (values_type);
-
   /* Are we returning a value using a structure return?  */
 
   if (gdbarch_return_in_first_hidden_param_p (gdbarch, values_type))
@@ -945,9 +949,6 @@
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
 
-  if (args.size () < TYPE_NFIELDS (ftype))
-    error (_("Too few arguments in function call."));
-
   for (int i = args.size () - 1; i >= 0; i--)
     {
       int prototyped;