[review] infcall: move assertions in 'call_function_by_hand_dummy' to an earli...
Commit Message
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(-)
Comments
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?
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."
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 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.
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.
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 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.
@@ -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;