diff mbox

[RFA] Implement show | set can-call-inferior-functions [on|off]

Message ID 20190423215826.9936-1-philippe.waroquiers@skynet.be
State New, archived
Headers

Commit Message

Philippe Waroquiers April 23, 2019, 9:58 p.m. UTC 
  Inferior function calls are powerful but might lead to undesired
results such as crashes when calling nested functions (frequently
used in particular in Ada).

This implements a GDB setting to disable calling inferior functions.

Note: the idea is that if/when the 'slash command' patch is pushed,
that this setting can be changed by using the shortcut /c.

2019-04-23  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

gdb/ChangeLog
	* NEWS: Mention the new set|show can-call-inferior-functions.
	* infcall.c (can_call_inferior_functions_p): New variable.
	(show_can_call_inferior_functions_p): New function.
	(call_function_by_hand_dummy): Throws an error if not
	can-call-inferior-functions.
	(_initialize_infcall): Call add_setshow_boolean_cmd for
	can-call-inferior-functions.

gdb/testsuite/ChangeLog
	* gdb.base/callexit.exp: Test can-call-inferior-functions off.

gdb/doc/ChangeLog
	* gdb.texinfo (Calling): Document the new
	set|show can-call-inferior-functions.
---
 gdb/NEWS                            | 11 +++++++++++
 gdb/doc/gdb.texinfo                 | 16 +++++++++++++++
 gdb/infcall.c                       | 30 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/callexit.exp |  7 +++++++
 4 files changed, 64 insertions(+)

Comments

Eli Zaretskii April 24, 2019, 6:53 a.m. UTC | #1 
> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Date: Tue, 23 Apr 2019 23:58:26 +0200
> 
> Inferior function calls are powerful but might lead to undesired
> results such as crashes when calling nested functions (frequently
> used in particular in Ada).
> 
> This implements a GDB setting to disable calling inferior functions.

Thanks, I think this is useful functionality.

However, I question the wisdom of erroring out when an attempt is made
to call inferior functions when that's disabled.  If this inferior
call is part of some script, the script will error out at that point,
which might not be what the user wants.  Erroring out is justified
when the inferior function was called interactively, though.  Can we
come up with something more useful for the scripting use case?  Like
perhaps 'call' should display a warning and do nothing, 'print' should
just display a warning, and otherwise the inferior would always return
zero?  Or maybe this should be subject to some additional knob?

> +set can-call-inferior-functions [on|off]
> +show can-call-inferior-functions
> +  Control whether GDB is allowed to do inferior function calls.
> +  Inferior function calls are e.g. needed to evaluate and print
> +  some expressions.  Such inferior function calls can have undesired
> +  side effects.  It is now possible to forbid such inferior function
> +  calls.
> +  By default, GDB is allowed to do inferior function calls.

I think this should tell what happens if the function is called when
inferior calls are not allowed.

> +@item set can-call-inferior-functions
> +@kindex set can-call-inferior-functions
> +@cindex disabling inferior function calls.

Index entries should not end with a period.

Also, I think an additional index entry here will be useful:

  @cindex inferior function calls, disabling

> +  add_setshow_boolean_cmd ("can-call-inferior-functions", no_class,
> +			   &can_call_inferior_functions_p, _("\
> +Set debugger's willingness to call inferior functions."), _("\
> +Show debugger's willingness to call inferior functions."), _("\
> +To call an inferior function, GDB has to temporarily modify the state\n\
> +of the inferior.  This has potentially undesired side effects.\n\
> +Also, having GDB calling nested functions is likely to be erroneous\n\
> +and may even crash the program being debugged.\n\
> +You can avoid such hazards by forbidding GDB to do inferior functions calls.\n\
> +The default is to allow inferior function calls."),

I think this doc string is too detailed, you in effect copied here
everything that we have in the manual, which is too much.  I suggest
to leave out the rationale for disabling the inferior calls, and
describe just the effects.  I do think this should say what happens
with calls when they are disallowed.

Thanks, the documentation parts are OK once the above nits are taken
care of.
Philippe Waroquiers April 24, 2019, 9:25 p.m. UTC | #2 
On Wed, 2019-04-24 at 09:53 +0300, Eli Zaretskii wrote:
> > From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> > Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> > Date: Tue, 23 Apr 2019 23:58:26 +0200
> > 
> > Inferior function calls are powerful but might lead to undesired
> > results such as crashes when calling nested functions (frequently
> > used in particular in Ada).
> > 
> > This implements a GDB setting to disable calling inferior functions.
> 
> Thanks, I think this is useful functionality.
> 
> However, I question the wisdom of erroring out when an attempt is made
> to call inferior functions when that's disabled.  If this inferior
> call is part of some script, the script will error out at that point,
> which might not be what the user wants.  Erroring out is justified
> when the inferior function was called interactively, though.  Can we
> come up with something more useful for the scripting use case?  Like
> perhaps 'call' should display a warning and do nothing, 'print' should
> just display a warning, and otherwise the inferior would always return
> zero?  Or maybe this should be subject to some additional knob?

Generally, returning 0 (or whatever value) can then later on 
cause problems in a script.
For example, evaluating some function calls sometimes imply to first
call malloc in the inferior.  When can-call-inferior-functions is off,
returning 0 (or whatever) from malloc will then cause further problems
(such as a SEGV).

So, it looks to me that just forbidding function calls when set to "off'
is on the safe side (which is the objective of this setting when debugging
things you do not want to see crash or impact with inferior function calls).
Then, the user can always decide to temporarily change the setting
if desired, once the error message is visible.


I will do the suggested doc changes for the v2 of the patch,

Thanks

Philippe

> 
> > +set can-call-inferior-functions [on|off]
> > +show can-call-inferior-functions
> > +  Control whether GDB is allowed to do inferior function calls.
> > +  Inferior function calls are e.g. needed to evaluate and print
> > +  some expressions.  Such inferior function calls can have undesired
> > +  side effects.  It is now possible to forbid such inferior function
> > +  calls.
> > +  By default, GDB is allowed to do inferior function calls.
> 
> I think this should tell what happens if the function is called when
> inferior calls are not allowed.
> 
> > +@item set can-call-inferior-functions
> > +@kindex set can-call-inferior-functions
> > +@cindex disabling inferior function calls.
> 
> Index entries should not end with a period.
> 
> Also, I think an additional index entry here will be useful:
> 
>   @cindex inferior function calls, disabling
> 
> > +  add_setshow_boolean_cmd ("can-call-inferior-functions", no_class,
> > +			   &can_call_inferior_functions_p, _("\
> > +Set debugger's willingness to call inferior functions."), _("\
> > +Show debugger's willingness to call inferior functions."), _("\
> > +To call an inferior function, GDB has to temporarily modify the state\n\
> > +of the inferior.  This has potentially undesired side effects.\n\
> > +Also, having GDB calling nested functions is likely to be erroneous\n\
> > +and may even crash the program being debugged.\n\
> > +You can avoid such hazards by forbidding GDB to do inferior functions calls.\n\
> > +The default is to allow inferior function calls."),
> 
> I think this doc string is too detailed, you in effect copied here
> everything that we have in the manual, which is too much.  I suggest
> to leave out the rationale for disabling the inferior calls, and
> describe just the effects.  I do think this should say what happens
> with calls when they are disallowed.
> 
> Thanks, the documentation parts are OK once the above nits are taken
> care of.
Eli Zaretskii April 25, 2019, 6:12 a.m. UTC | #3 
> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Cc: gdb-patches@sourceware.org
> Date: Wed, 24 Apr 2019 23:25:27 +0200
> 
> Generally, returning 0 (or whatever value) can then later on 
> cause problems in a script.
> For example, evaluating some function calls sometimes imply to first
> call malloc in the inferior.  When can-call-inferior-functions is off,
> returning 0 (or whatever) from malloc will then cause further problems
> (such as a SEGV).

Then perhaps a built-in variable to test whether inferior calls are
allowed would be of help?  Then script writers could look at that
variable and avoid calling inferior functions if they want the script
to continue running regardless, perhaps with reduced functionality.
WDYT?
Tom Tromey April 25, 2019, 1:15 p.m. UTC | #4 
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
>> Cc: gdb-patches@sourceware.org
>> Date: Wed, 24 Apr 2019 23:25:27 +0200
>> 
>> Generally, returning 0 (or whatever value) can then later on 
>> cause problems in a script.
>> For example, evaluating some function calls sometimes imply to first
>> call malloc in the inferior.  When can-call-inferior-functions is off,
>> returning 0 (or whatever) from malloc will then cause further problems
>> (such as a SEGV).

Eli> Then perhaps a built-in variable to test whether inferior calls are
Eli> allowed would be of help?  Then script writers could look at that
Eli> variable and avoid calling inferior functions if they want the script
Eli> to continue running regardless, perhaps with reduced functionality.
Eli> WDYT?

An error definitely seems like the correct thing to do here.  Other
choices will give silently mysterious behavior.

A convenience variable is fine, too, though I suspect it will be little
used.  I wouldn't require it for this patch to go in.

gdb could use better control over the handling of errors.  There's a
catch/throw patch sitting in bugzilla that would be good to resurrect.
Or, the ignore-errors command (implemented in Python) could be checked in.

Tom
Pedro Alves April 25, 2019, 5:16 p.m. UTC | #5 
Hi Philippe,

On 4/23/19 10:58 PM, Philippe Waroquiers wrote:

> +* New commands
> +
> +set can-call-inferior-functions [on|off]
> +show can-call-inferior-functions
> +  Control whether GDB is allowed to do inferior function calls.
> +  Inferior function calls are e.g. needed to evaluate and print
> +  some expressions.  Such inferior function calls can have undesired
> +  side effects.  It is now possible to forbid such inferior function
> +  calls.
> +  By default, GDB is allowed to do inferior function calls.
> +

GDB already has a set of knobs that prevent the user from doing
something that might not be safe or desirable:

 (gdb) set may-[TAB]
 may-insert-breakpoints       may-insert-tracepoints       may-write-memory             
 may-insert-fast-tracepoints  may-interrupt                may-write-registers          

See the "Observer Mode" node in the manual.

I think it'd make sense to call this new knob "may-xxx" too, so it shows
up alongside those above.  It's also more correct in the strict
English sense -- "can" denotes capability, while "may" denotes
permission, and here we're talking about permission, not some target
backend capability.

> diff --git a/gdb/testsuite/gdb.base/callexit.exp b/gdb/testsuite/gdb.base/callexit.exp
> index b6d9ae3f87..ac93125eb4 100644
> --- a/gdb/testsuite/gdb.base/callexit.exp
> +++ b/gdb/testsuite/gdb.base/callexit.exp
> @@ -37,6 +37,13 @@ if { ![runto_main] } {
>      return 0
>  }
>  
> +# Verify set can-call-inferior-functions behaviour.
> +gdb_test_no_output "set can-call-inferior-functions off"
> +gdb_test "call callexit()" \
> +    "Cannot call inferior function: can-call-inferior-functions is off." \
> +    "Inferior function call refused in off state"

Lowercase "Inferior".


On 4/23/19 10:58 PM, Philippe Waroquiers wrote:
> +@item set can-call-inferior-functions
> +@kindex set can-call-inferior-functions
> +@cindex disabling inferior function calls.
> +Set debugger's willingness to call inferior functions.

I think talking about "willingness" here is a bit weird.

I can tell that this was copied from "set can-use-hw-watchpoints".
In that case, if GDB is not willing to use HW watchpoints, it'll
still support watchpoints, it's just that they'll be slower.
"set can-use-hw-watchpoints" exists to inform GDB about
the target's actual capability wrt to hardware watchpoints
("can" vs "may").

This case is different.  It's about whether GDB allows
the _user_ to call functions.

I think modeling this on e.g., "set may-write-memory"'s documentation
would be better.  E.g.,:

This controls whether @value{GDBN} will attempt to call functions in
the program, such as with expressions in @code{print}.  It
defaults to @code{on}.

> +To call an inferior function, @value{GDBN} has to temporarily modify the state
> +of the inferior.  This has potentially undesired side effects.
> +Also, having @value{GDBN} calling nested functions is likely to be erroneous

"having @value{GDBN} call"

> +and may even crash the program being debugged.
> +You can avoid such hazards by forbidding  @value{GDBN} to do inferior

"forbidding @value{GDBN} from doing inferior ..."

> +functions calls.
> +The default is to allow inferior function calls.

I'm not really sure we should refer to infcalls as "inferior function calls"
in the manual.  Throughout, we tend to refer to those as "calling functions
in the program".  E.g.,:

~~~
@node Calling
@section Calling Program Functions
~~~

~~~
When you issue the @code{run} command, your program begins to execute
immediately.  @xref{Stopping, ,Stopping and Continuing}, for discussion
of how to arrange for your program to stop.  Once your program has
stopped, you may call functions in your program, using the @code{print}
or @code{call} commands.  @xref{Data, ,Examining Data}.
~~~

~~~
It is possible for the function you call via the @code{print} or
@code{call} command to generate a signal (e.g., if there's a bug in
~~~

~~~
@kindex dprintf-style call
Handle the output by calling a function in your program (normally
@code{printf}).
~~~

~~~
If you call a function interactively using @code{print} or @code{call},
~~~

So I'd rephrase the new texts throughout (docs, code, NEWS, error
output, etc.) in that vein.  E.g.:

"You can avoid such hazards by forbidding @value{GDBN} from calling
functions in the program being debugged."

I'd also drop the "inferior in the new knob, like:

 "set may-call-functions"

Thanks,
Pedro Alves
Pedro Alves April 25, 2019, 7:44 p.m. UTC | #6 
On 4/25/19 2:15 PM, Tom Tromey wrote:

> An error definitely seems like the correct thing to do here.  Other
> choices will give silently mysterious behavior.
> 
> A convenience variable is fine, too, though I suspect it will be little
> used.  I wouldn't require it for this patch to go in.

Agreed.  You could argue that it'd be better to instead come up with
a facility for CLI to inspect the value of any set/show option, though.
But you can already do that with Python's gdb.parameter, so I
guess it may not be that pressing to have a pure CLI mechanism.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/NEWS b/gdb/NEWS
index 5309a8f923..6288fed8af 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -27,6 +27,17 @@ 
      'array_indexes', 'symbols', 'unions', 'deref_refs', 'actual_objects',
      'static_members', 'max_elements', 'repeat_threshold', and 'format'.
 
+* New commands
+
+set can-call-inferior-functions [on|off]
+show can-call-inferior-functions
+  Control whether GDB is allowed to do inferior function calls.
+  Inferior function calls are e.g. needed to evaluate and print
+  some expressions.  Such inferior function calls can have undesired
+  side effects.  It is now possible to forbid such inferior function
+  calls.
+  By default, GDB is allowed to do inferior function calls.
+
 *** Changes in GDB 8.3
 
 * GDB and GDBserver now support access to additional registers on
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 0733e1acfd..38fd109369 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -18693,6 +18693,22 @@  the default C@t{++} exception handler and the inferior terminated.
 Show the current setting of stack unwinding in the functions called by
 @value{GDBN}.
 
+@item set can-call-inferior-functions
+@kindex set can-call-inferior-functions
+@cindex disabling inferior function calls.
+Set debugger's willingness to call inferior functions.
+To call an inferior function, @value{GDBN} has to temporarily modify the state
+of the inferior.  This has potentially undesired side effects.
+Also, having @value{GDBN} calling nested functions is likely to be erroneous
+and may even crash the program being debugged.
+You can avoid such hazards by forbidding  @value{GDBN} to do inferior
+functions calls.
+The default is to allow inferior function calls.
+
+@item show can-call-inferior-functions
+@kindex show can-call-inferior-functions
+Show debugger's willingness to call inferior functions.
+
 @end table
 
 @subsection Calling functions with no debug info
diff --git a/gdb/infcall.c b/gdb/infcall.c
index c102b301e0..a8965bd74a 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -55,6 +55,18 @@ 
    asynchronous inferior function call implementation, and that in
    turn means restructuring the code so that it is event driven.  */
 
+static int can_call_inferior_functions_p = 1;
+static void
+show_can_call_inferior_functions_p (struct ui_file *file, int from_tty,
+				    struct cmd_list_element *c,
+				    const char *value)
+{
+  fprintf_filtered (file,
+		    _("Debugger's willingness to call "
+		      "inferior functions is %s.\n"),
+		    value);
+}
+
 /* How you should pass arguments to a function depends on whether it
    was defined in K&R style or prototype style.  If you define a
    function using the K&R syntax that takes a `float' argument, then
@@ -708,6 +720,10 @@  call_function_by_hand_dummy (struct value *function,
   struct gdb_exception e;
   char name_buf[RAW_FUNCTION_ADDRESS_SIZE];
 
+  if (!can_call_inferior_functions_p)
+    error (_("Cannot call inferior function: "
+	     "can-call-inferior-functions is off."));
+
   if (!target_has_execution)
     noprocess ();
 
@@ -1359,6 +1375,20 @@  When the function is done executing, GDB will silently stop."),
 void
 _initialize_infcall (void)
 {
+  add_setshow_boolean_cmd ("can-call-inferior-functions", no_class,
+			   &can_call_inferior_functions_p, _("\
+Set debugger's willingness to call inferior functions."), _("\
+Show debugger's willingness to call inferior functions."), _("\
+To call an inferior function, GDB has to temporarily modify the state\n\
+of the inferior.  This has potentially undesired side effects.\n\
+Also, having GDB calling nested functions is likely to be erroneous\n\
+and may even crash the program being debugged.\n\
+You can avoid such hazards by forbidding GDB to do inferior functions calls.\n\
+The default is to allow inferior function calls."),
+			   NULL,
+			   show_can_call_inferior_functions_p,
+			   &setlist, &showlist);
+
   add_setshow_boolean_cmd ("coerce-float-to-double", class_obscure,
 			   &coerce_float_to_double_p, _("\
 Set coercion of floats to doubles when calling functions."), _("\
diff --git a/gdb/testsuite/gdb.base/callexit.exp b/gdb/testsuite/gdb.base/callexit.exp
index b6d9ae3f87..ac93125eb4 100644
--- a/gdb/testsuite/gdb.base/callexit.exp
+++ b/gdb/testsuite/gdb.base/callexit.exp
@@ -37,6 +37,13 @@  if { ![runto_main] } {
     return 0
 }
 
+# Verify set can-call-inferior-functions behaviour.
+gdb_test_no_output "set can-call-inferior-functions off"
+gdb_test "call callexit()" \
+    "Cannot call inferior function: can-call-inferior-functions is off." \
+    "Inferior function call refused in off state"
+gdb_test_no_output "set can-call-inferior-functions on"
+
 # Call function (causing the program to exit), and see if gdb handles
 # it properly.
 gdb_test "call callexit()" \