Re: [PATCH v6] Events when inferior is modified

[Pedro Alves <palves at redhat dot com>]

I see many new functions without intro comments.  Please add an intro comment
to each new function, describing the contract of the function.  If the function
is just implementing some hook, then have the function comment say that
"Inplements method foo for bar", instead of duplicating comments about
parameters and return types.

Please add comments to new enums (inferior_call_kind), and their values, etc. too.

> @@ -627,6 +628,8 @@ call_function_by_hand (struct value *function, int
> nargs, struct value **args)
>         target_values_type = values_type;
>       }
>
> +  observer_notify_inferior_call_pre ();
> +
>     /* Determine the location of the breakpoint (and possibly other
>        stuff) that the called function will return to.  The SPARC, for a
>        function returning a structure or union, needs to make space for
> @@ -860,6 +863,8 @@ call_function_by_hand (struct value *function, int
> nargs, struct value **args)
>       e = run_inferior_call (tp, real_pc);
>     }
>
> +  observer_notify_inferior_call_post ();
> +
>     /* Rethrow an error if we got one trying to run the inferior.  */
>
>     if (e.reason < 0)

I see an issue with these events.  On the call_pre case, the
caller can infer which thread the infcall was being run on
from the currently selected thread.

But, not so from the call_post.  The inferior may have crashed,
exited, or stopped for some breakpoint in a different thread
at that point.  So the user of that event doesn't really know
which function call finished.

GDB only does one function call at a time currently, but
that's subject to change.  It'd be best to design with that
in mind already.

Also, the documentation leaves this issue unspecified:

> +@item events.inferior_call_post
> +Emits @code{gdb.InferiorCallPostEvent} which indicates that a function in
> +the inferior has returned.

I, as I reader, immediately wonder what happens if the function
doesn't get to return, but inferior exits, etc.  E.g., "is the
event called in that scenario as well?".

> +# Test register changed event
> +gdb_test_no_output {set $old_sp = $sp}
> +gdb_test {set $sp = 0} ".*event type: register-changed.*
> +.*frame: .*
> +.*num: .*"
> +gdb_test {set $sp = 1} ".*event type: register-changed.*
> +.*frame: .*
> +.*num: .*"
> +gdb_test {set $sp = $old_sp} ".*event type: register-changed.*
> +.*frame: .*
> +.*num: .*"

Please write explicit \r\n instead.  (or use e.g.,
gdb_test_sequence).


> +set test "continue to breakpoint 5"
> +gdb_test_multiple "continue" $test {
> +     -re "event type: memory-changed" {
> +	fail $test
> +    }
> +    -re ".*event type: continue.*
> +.*event type: stop.*
> +.*stop reason: breakpoint.*
> +.*all threads stopped.*$gdb_prompt $" {
> +	pass $test
> +    }
> +}
> +
> +gdb_test_no_output "delete 5"

Please avoid hard coding breakpoint numbers in tests.
You can instead extract the number with expect_out
when you create the breakpoint, and store it in a
tcl variable.  Alternatively, use $bpnum.

> +gdb_test_multiple {up} $test {
> +    -re "event type: register-changed" {
> +	fail $test
> +    }

Please always match $gdb_prompt $, otherwise you're
leaving the prompt in the expect buffer, which confuses
the next gdb_test_multiple/gdb_test call.

While at it, please make sure the test passes with "make check-read1".

> +    -re "#1.*in first.*\r\n.*do_nothing.*\r\n$gdb_prompt $" {
> +	pass $test
> +    }
> +}

> +  ** gdb.events.inferior_call_pre Function call is about to be made.
> +  ** gdb.events.inferior_call_post Function call has just been made.
> +  ** gdb.events.memory_changed A memory location has been altered.
> +  ** gdb.events.register_changed A register has been altered.

Maybe it's just me, but I find this hard to read.  I'd expect some kind
of separator between the event name and its description.  Like e.g.,:

  ** gdb.events.register_changed - A register has been altered.
or:
  ** gdb.events.register_changed: A register has been altered.
or:
  ** gdb.events.register_changed
       A register has been altered.


Thanks,
Pedro Alves