Re: [RFA] Convert observers to C++

[Pedro Alves <palves at redhat dot com>]

On 11/05/2017 01:30 AM, Tom Tromey wrote:
> This converts observers from using a special source-generating script
> to be plain C++.  This is the second version of this patch.  This one
> takes advavantage of C++11 by using std::function and variadic
> templates.
> 
> This patch will require a clean build, because observer.h now appears
> in the source directory.  If this seems inconvenient, I could choose a
> different name for the new hearer, like "gdb_observers.h".
> 
> Regression tested on the buildbot.
> 

Thanks!

Some comments below.  Most mild, but one I think points at a bug.

> diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
> deleted file mode 100644
> index 606ddfe536..0000000000
> --- a/gdb/doc/observer.texi
> +++ /dev/null
> @@ -1,313 +0,0 @@
> -@c -*-texinfo-*-
> -
> -@c This file is part of the GDB manual.
> -@c
> -@c Copyright (C) 2003-2017 Free Software Foundation, Inc.
> -@c
> -@c See the file gdbint.texinfo for copying conditions.
> -@c
> -@c Also, the @deftypefun lines from this file are processed into a
> -@c header file during the GDB build process.  Permission is granted
> -@c to redistribute and/or modify those lines under the terms of the
> -@c GNU General Public License as published by the Free Software
> -@c Foundation; either version 3 of the License, or (at your option)
> -@c any later version.
> -
> -@node GDB Observers
> -@appendix @value{GDBN} Currently available observers
> -
> -@section Implementation rationale
> -@cindex observers implementation rationale
> -
> -An @dfn{observer} is an entity which is interested in being notified
> -when GDB reaches certain states, or certain events occur in GDB.
> -The entity being observed is called the @dfn{subject}.  To receive
> -notifications, the observer attaches a callback to the subject.
> -One subject can have several observers.
> -
> -@file{observer.c} implements an internal generic low-level event
> -notification mechanism.  This generic event notification mechanism is
> -then re-used to implement the exported high-level notification
> -management routines for all possible notifications.
> -
> -The current implementation of the generic observer provides support
> -for contextual data.  This contextual data is given to the subject
> -when attaching the callback.  In return, the subject will provide
> -this contextual data back to the observer as a parameter of the
> -callback.
> -
> -Note that the current support for the contextual data is only partial,
> -as it lacks a mechanism that would deallocate this data when the
> -callback is detached.  This is not a problem so far, as this contextual
> -data is only used internally to hold a function pointer.  Later on, if
> -a certain observer needs to provide support for user-level contextual
> -data, then the generic notification mechanism will need to be
> -enhanced to allow the observer to provide a routine to deallocate the
> -data when attaching the callback.
> -
> -The observer implementation is also currently not reentrant.
> -In particular, it is therefore not possible to call the attach
> -or detach routines during a notification.
> -
> -@section Debugging
> -Observer notifications can be traced using the command @samp{set debug
> -observer 1} (@pxref{Debugging Output, , Optional messages about
> -internal happenings, gdb, Debugging with @var{GDBN}}).
> -
> -@section @code{normal_stop} Notifications
> -@cindex @code{normal_stop} observer
> -@cindex notification about inferior execution stop
> -
> -@value{GDBN} notifies all @code{normal_stop} observers when the
> -inferior execution has just stopped, the associated messages and
> -annotations have been printed, and the control is about to be returned
> -to the user.
> -
> -Note that the @code{normal_stop} notification is not emitted when
> -the execution stops due to a breakpoint, and this breakpoint has
> -a condition that is not met.  If the breakpoint has any associated
> -commands list, the commands are executed after the notification
> -is emitted.

Is all the documentation in this file preserved?  I couldn't find
where this paragraph was moved to, for example.



> +namespace gdb_observers
> +{
> +unsigned int observer_debug;
> +
> +observer<struct bpstats *, int> normal_stop ("normal_stop");

You don't need to duplicate the observer types here.
This should work:

  decltype (normal_stop) normal_stop ("normal_stop");

And then, you could even do:

 #define DEFINE_OBSERVER(OBSERVER_NAME) \
   decltype (OBSERVER_NAME) OBSERVER_NAME (#OBSERVER_NAME)

 DEFINE_OBSERVER (normal_stop);
 DEFINE_OBSERVER (end_stepping_range);
 ...

> +observer<enum gdb_signal> signal_received ("signal_received");
> +observer<> end_stepping_range ("end_stepping_range");
> +observer<enum gdb_signal> signal_exited ("signal_exited");
> +observer<int> exited ("exited");
> +observer<> no_history ("no_history");
> +observer<> sync_execution_done ("sync_execution_done");
> +observer<> command_error ("command_error");
> +observer<struct target_ops *> target_changed ("target_changed");
> +observer<> executable_changed ("executable_changed");
> +observer<struct target_ops *, int> inferior_created ("inferior_created");
> +observer<struct inferior *, int, const char *, const char *>
> +    record_changed ("record_changed");
> +observer<struct so_list *> solib_loaded ("solib_loaded");
> +observer<struct so_list *> solib_unloaded ("solib_unloaded");
> +observer<struct objfile *> new_objfile ("new_objfile");
> +observer<struct objfile *> free_objfile ("free_objfile");
> +observer<struct thread_info *> new_thread ("new_thread");
> +observer<struct thread_info *, int> thread_exit ("thread_exit");
> +observer<ptid_t> thread_stop_requested ("thread_stop_requested");
> +observer<ptid_t> target_resumed ("target_resumed");
> +observer<> about_to_proceed ("about_to_proceed");
> +observer<struct breakpoint *> breakpoint_created ("breakpoint_created");
> +observer<struct breakpoint *> breakpoint_deleted ("breakpoint_deleted");
> +observer<struct breakpoint *> breakpoint_modified ("breakpoint_modified");
> +observer<int, int> traceframe_changed ("traceframe_changed");
> +observer<struct gdbarch *> architecture_changed ("architecture_changed");
> +observer<ptid_t, ptid_t> thread_ptid_changed ("thread_ptid_changed");
> +observer<struct inferior *> inferior_added ("inferior_added");
> +observer<struct inferior *> inferior_appeared ("inferior_appeared");
> +observer<struct inferior *> inferior_exit ("inferior_exit");
> +observer<struct inferior *> inferior_removed ("inferior_removed");
> +observer<struct inferior *, CORE_ADDR, ssize_t, const bfd_byte *>
> +    memory_changed ("memory_changed");
> +observer<const char *> before_prompt ("before_prompt");
> +observer<> gdb_datadir_changed ("gdb_datadir_changed");
> +observer<const char *, const char *>
> +    command_param_changed ("command_param_changed");
> +observer<const struct trace_state_variable *> tsv_created ("tsv_created");
> +observer<const struct trace_state_variable *> tsv_deleted ("tsv_deleted");
> +observer<const struct trace_state_variable *> tsv_modified ("tsv_modified");
> +observer<ptid_t, CORE_ADDR> inferior_call_pre ("inferior_call_pre");
> +observer<ptid_t, CORE_ADDR> inferior_call_post ("inferior_call_post");
> +observer<struct frame_info *, int> register_changed ("register_changed");
> +observer<user_selected_what>
> +    user_selected_context_changed ("user_selected_context_changed");
> +
> +#ifdef GDB_SELF_TEST
> +

It'd be nicer if the self tests were under "namespace selftests",
IMHO.

OOC, is there some guideline you were following for preferring
"namespace gdb_observers" over, say, "namespace gdb::observers" ?
Just curiosity, don't feel the need to change anything.

> +// This observer is used for internal testing.
> +static observer<int> test_notification ("test_notification");
> +

You're going to hate me, but it'd be nice IMO if you
switched to use /**/ consistently throughout, per GCC/GDB
convention.

>  
> -void
> -observer_test_third_notification_function (int arg)
> +static void
> +show_observer_debug (struct ui_file *file, int from_tty,
> +		     struct cmd_list_element *c, const char *value)
>  {
> -  observer_test_third_observer++;
> +  fprintf_filtered (file, _("Observer debugging is %s.\n"), value);
>  }
>  
>  void
>  _initialize_observer (void)
>  {
>    add_setshow_zuinteger_cmd ("observer", class_maintenance,
> -			     &observer_debug, _("\
> +			     &gdb_observers::observer_debug, _("\
>  Set observer debugging."), _("\
>  Show observer debugging."), _("\
>  When non-zero, observer debugging is enabled."),
>  			     NULL,
>  			     show_observer_debug,
>  			     &setdebuglist, &showdebuglist);
> -}
>  
> -#include "observer.inc"
> +#if GDB_SELF_TEST
> +  selftests::register_test ("gdb_observers",
> +			    gdb_observers::observer_self_tests);
> +#endif
> +}
> diff --git a/gdb/observer.h b/gdb/observer.h
> new file mode 100644
> index 0000000000..b40f6334ad
> --- /dev/null
> +++ b/gdb/observer.h
> @@ -0,0 +1,283 @@
> +/* Observers
> +
> +   Copyright (C) 2016, 2017 Free Software Foundation, Inc.

Use year range: 2016-2017.

> +template<typename... T>
> +class observer
> +{
> +public:
> +
> +  typedef std::function<void(T...)> func_type;
> +
> +  /* The return type of attach, which can be passed to detach to
> +     remove a listener.  */
> +  typedef size_t token_type;
> +
> +  /* A "null" value of token_type that can be used as a
> +     sentinel.  */
> +  static const token_type null_token;
> +
> +  explicit observer (const char *name) : m_name (name)
> +  {
> +  }
> +
> +  DISABLE_COPY_AND_ASSIGN (observer);
> +
> +  token_type attach (const func_type &f)
> +  {
> +    m_observers.push_back (f);
> +    return m_observers.size () - 1;
> +  }
> +
> +  void detach (const token_type &f)
> +  {
> +    m_observers.erase (m_observers.begin () + f);

Hmm, this looks incorrect.  "attach" returns an index
as token.  And then detach uses "std::vector::erase()",
which removes the element at the index and then shifts
the rest of the elements left to fill in the space
left over by the removed element(s).  So that invalidates
all token after the element erased.

> +  }
> +
> +  void notify (T... args) const
> +  {
> +    if (observer_debug)
> +      fprintf_unfiltered (gdb_stdlog, "observer %s notify() called\n",
> +			  m_name);
> +    for (auto &&func : m_observers)
> +      func (args...);
> +  }
> +
> +private:
> +
> +  std::vector<func_type> m_observers;
> +  const char *m_name;
> +};
> +
> +template<typename... T>
> +const typename observer<T...>::token_type observer<T...>::null_token
> +    = (size_t) -1;
> +

This could be constexpr and then defined in-class, right?

> +/* The inferior has stopped for real.  The bs argument describes the
> +   breakpoints were are stopped at, if any.  Second argument
> +   print_frame non-zero means display the location where the
> +   inferior has stopped.  */
> +extern observer<struct bpstats *, int> normal_stop;
> +
> +/* The inferior was stopped by a signal.  */
> +extern observer<enum gdb_signal> signal_received;
> +
> +/* We are done with a step/next/si/ni command.  */
> +extern observer<> end_stepping_range;
> +
> +/* The inferior was terminated by a signal.  */
> +extern observer<enum gdb_signal> signal_exited;
> +
> +/* The inferior program is finished.  */
> +extern observer<int> exited;
> +
> +/* Reverse execution: target ran out of history info.  */
> +extern observer<> no_history;
> +
> +/* A synchronous command finished.  */
> +extern observer<> sync_execution_done;
> +
> +/* An error was caught while executing a command.  */
> +extern observer<> command_error;
> +
> +/* The target's register contents have changed.  */
> +extern observer<struct target_ops *> target_changed;
> +
> +/* The executable being debugged by GDB has changed: The user
> +   decided to debug a different program, or the program he was
> +   debugging has been modified since being loaded by the debugger
> +   (by being recompiled, for instance).  */
> +extern observer<> executable_changed;
> +
> +/* gdb has just connected to an inferior.  For 'run', gdb calls this
> +   observer while the inferior is still stopped at the entry-point
> +   instruction.  For 'attach' and 'core', gdb calls this observer
> +   immediately after connecting to the inferior, and before any
> +   information on the inferior has been printed.  */
> +extern observer<struct target_ops *, int> inferior_created;
> +
> +/* The status of process record for inferior inferior in gdb has
> +   changed.  The process record is started if started is true, and
> +   the process record is stopped if started is false.
> +
> +   When started is true, method indicates the short name of the
> +   method used for recording.  If the method supports multiple
> +   formats, format indicates which one is being used, otherwise it
> +   is NULL.  When started is false, they are both NULL.  */
> +extern observer<struct inferior *, int, const char *, const char *>
> +    record_changed;
> +
> +/* The shared library specified by solib has been loaded.  Note that
> +   when gdb calls this observer, the library's symbols probably
> +   haven't been loaded yet.  */
> +extern observer<struct so_list *> solib_loaded;
> +
> +/* The shared library specified by solib has been unloaded.  Note
> +   that when gdb calls this observer, the library's symbols have not
> +   been unloaded yet, and thus are still available.  */
> +extern observer<struct so_list *> solib_unloaded;
> +
> +/* The symbol file specified by objfile has been loaded.  Called
> +   with objfile equal to NULL to indicate previously loaded symbol
> +   table data has now been invalidated.  */
> +extern observer<struct objfile *> new_objfile;
> +
> +/* The object file specified by objfile is about to be freed.  */
> +extern observer<struct objfile *> free_objfile;
> +
> +/* The thread specified by t has been created.  */
> +extern observer<struct thread_info *> new_thread;
> +
> +/* The thread specified by t has exited.  The silent argument
> +   indicates that gdb is removing the thread from its tables without
> +   wanting to notify the user about it.  */
> +extern observer<struct thread_info *, int> thread_exit;
> +
> +/* An explicit stop request was issued to ptid.  If ptid equals
> +   minus_one_ptid, the request applied to all threads.  If
> +   ptid_is_pid(ptid) returns true, the request applied to all
> +   threads of the process pointed at by ptid.  Otherwise, the
> +   request applied to the single thread pointed at by ptid.  */
> +extern observer<ptid_t> thread_stop_requested;
> +
> +/* The target was resumed.  The ptid parameter specifies which
> +   thread was resume, and may be RESUME_ALL if all threads are
> +   resumed.  */
> +extern observer<ptid_t> target_resumed;
> +
> +/* The target is about to be proceeded.  */
> +extern observer<> about_to_proceed;
> +
> +/* A new breakpoint b has been created.  */
> +extern observer<struct breakpoint *> breakpoint_created;
> +
> +/* A breakpoint has been destroyed.  The argument b is the
> +   pointer to the destroyed breakpoint.  */
> +extern observer<struct breakpoint *> breakpoint_deleted;
> +
> +/* A breakpoint has been modified in some way.  The argument b
> +   is the modified breakpoint.  */
> +extern observer<struct breakpoint *> breakpoint_modified;
> +
> +/* The trace frame is changed to tfnum (e.g., by using the 'tfind'
> +   command).  If tfnum is negative, it means gdb resumes live
> +   debugging.  The number of the tracepoint associated with this
> +   traceframe is tpnum.  */
> +extern observer<int, int> traceframe_changed;
> +
> +/* The current architecture has changed.  The argument newarch is a
> +   pointer to the new architecture.  */
> +extern observer<struct gdbarch *> architecture_changed;
> +
> +/* The thread's ptid has changed.  The old_ptid parameter specifies
> +   the old value, and new_ptid specifies the new value.  */
> +extern observer<ptid_t, ptid_t> thread_ptid_changed;
> +
> +/* The inferior inf has been added to the list of inferiors.  At
> +   this point, it might not be associated with any process.  */
> +extern observer<struct inferior *> inferior_added;
> +
> +/* The inferior identified by inf has been attached to a
> +   process.  */
> +extern observer<struct inferior *> inferior_appeared;
> +
> +/* Either the inferior associated with inf has been detached from
> +   the process, or the process has exited.  */
> +extern observer<struct inferior *> inferior_exit;
> +
> +/* The inferior inf has been removed from the list of inferiors.
> +   This method is called immediately before freeing inf.  */
> +extern observer<struct inferior *> inferior_removed;
> +
> +/* Bytes from data to data + len have been written to the inferior
> +   at addr.  */
> +extern observer<struct inferior *, CORE_ADDR, ssize_t, const bfd_byte *>
> +    memory_changed;
> +
> +/* Called before a top-level prompt is displayed.  current_prompt is
> +   the current top-level prompt.  */
> +extern observer<const char *> before_prompt;
> +
> +/* Variable gdb_datadir has been set.  The value may not necessarily
> +   change.  */
> +extern observer<> gdb_datadir_changed;
> +
> +/* The parameter of some 'set' commands in console are changed.
> +   This method is called after a command 'set param value'.  param
> +   is the parameter of 'set' command, and value is the value of
> +   changed parameter.  */
> +extern observer<const char *, const char *> command_param_changed;
> +
> +/* The new trace state variable tsv is created.  */
> +extern observer<const struct trace_state_variable *> tsv_created;
> +
> +/* The trace state variable tsv is deleted.  If tsv is NULL, all
> +   trace state variables are deleted.  */
> +extern observer<const struct trace_state_variable *> tsv_deleted;
> +
> +/* The trace state value tsv is modified.  */
> +extern observer<const struct trace_state_variable *> tsv_modified;
> +
> +/* An inferior function at address is about to be called in thread
> +   thread.  */
> +extern observer<ptid_t, CORE_ADDR> inferior_call_pre;
> +
> +/* The inferior function at address has just been called.  This
> +   observer is called even if the inferior exits during the call.
> +   thread is the thread in which the function was called, which may
> +   be different from the current thread.  */
> +extern observer<ptid_t, CORE_ADDR> inferior_call_post;
> +
> +/* A register in the inferior has been modified by the gdb user.  */
> +extern observer<struct frame_info *, int> register_changed;
> +
> +/* The user-selected inferior, thread and/or frame has changed.  The
> +   user_select_what flag specifies if the inferior, thread and/or
> +   frame has changed.  */
> +extern observer<user_selected_what> user_selected_context_changed;

(I wonder about splitting core/lib from uses of the core.
I.e., defining the struct observer core mechanism in one
header, and then define the actual observers in a separate header.
I'm basically pondering about e.g., using observers under common/
or in gdbserver/.  Like e.g., "common/observer.h" and then
"gdb/observers.h" and "gdbserver/observers.h", etc.  But we can cross
that bridge when we get to it, too.)

Thanks,
Pedro Alves