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

login
register
mail settings
Submitter Simon Marchi (Code Review)
Date Oct. 18, 2019, 1:53 p.m.
Message ID <gerrit.1571406803000.I411ac083ac6a9ee6eb93c4b82393a81a4fc927be@gnutoolchain-gerrit.osci.io>
Download mbox | patch
Permalink /patch/35138/
State New
Headers show

Comments

Simon Marchi (Code Review) - Oct. 18, 2019, 1:53 p.m.
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(-)
Simon Marchi (Code Review) - Oct. 18, 2019, 3:10 p.m.
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?
Simon Marchi (Code Review) - Oct. 21, 2019, 7:36 a.m.
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."
Simon Marchi (Code Review) - Oct. 21, 2019, 6:55 p.m.
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.
Simon Marchi (Code Review) - Oct. 21, 2019, 7 p.m.
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.
Simon Marchi (Code Review) - Oct. 22, 2019, 7:19 a.m.
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.
Simon Marchi (Code Review) - Oct. 22, 2019, 8:57 a.m.
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
Simon Marchi (Code Review) - Oct. 22, 2019, 4:27 p.m.
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.

Patch

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;