[PATCH v3] gdb: Do autoload before notifying Python side in new_objfile event
Michael Weghorn
m.weghorn@posteo.de
Thu Apr 22 15:46:57 GMT 2021
On 20/04/2021 23.57, Simon Marchi wrote:
> Huh, I started working on this because I didn't see your message and
> didn't realize you had implemented it already. Here's my WIP branch for
> reference:
>
> https://github.com/simark/binutils-gdb/tree/observer-dep
>
> I made some random improvements to observable.h in the form of
> preparatory patches. They could be merged later, or you could rebase
> your patch on top if you think it would help you.
Thanks, I've rebased on top of commit f7716715f1d2a71a5c95b7ac0e0323b742692e6d
("gdbsupport: add observer_debug_printf, OBSERVER_SCOPED_DEBUG_ENTER_EXIT")
from that WIP branch and dropped those parts from my patch that were already
included in commits on that branch up to that point.
I've left out the last two patches from that branch which were doing
similar things to my patch, but it didn't look straightforward to me
how to reasonably "merge" the two versions.
I've taken some inspiration for naming (which was nicer on your branch) and split
my patch into two separate ones similar to how it is done on your branch,
though (one patch to add the functionality to order observers + corresponding
unit tests; one patch to make use of that and add the testcase
for the testsuite).
I was uncertain of whether or not to include your commits from the WIP
branch in the patch series to send to the mailing list, and haven't done
so yet in v4 (but can of course do so in the next version if it makes
sense) that I just sent:
https://sourceware.org/pipermail/gdb-patches/2021-April/178069.html
>
> I left a few comments below. But in general this looks great, pretty
> much what I had in mind when I suggested this idea.
>
> Random question for you: while doing this work, I stumbled upon this
> line in the detach method:
>
> m_observers.erase (iter, m_observers.end ());
>
> Does that look right to you? Doesn't this delete more observers than
> the one we want to detach, if there are more observers after it?
As I understand it, that should be OK, since 'std::remove_if' is called
first, which - as I understand it - should make sure that all
not-to-be-removed observers will be before 'iter' afterwards.
http://www.cplusplus.com/reference/algorithm/remove_if/?kw=remove_if
says:
"Transforms the range [first,last) into a range with all the elements for which
pred returns true removed, and returns an iterator to the new end of that
range.
The function cannot alter the properties of the object containing the range of
elements (i.e., it cannot alter the size of an array or a container):
The removal is done by replacing the elements for which pred returns true
by the next element for which it does not, and signaling the new size of
the shortened range by returning an iterator to the element that should be
considered its new past-the-end element."
>
> On 2021-03-26 4:29 a.m., Michael Weghorn via Gdb-patches wrote:
>> Previously, the observers attached to the 'gdb::observers::new_objfile'
>> observable were always notified in the order in which they had been
>> attached.
>>
>> The new_objfile observer callback to auto-load scripts is attached in
>> '_initialize_auto_load'.
>> The new_objfile observer callback that propagates the new_objfile event
>> to the Python side is attached in 'gdbpy_initialize_inferior', which is
>> called via '_initialize_python'.
>> With '_initialize_python' happening before '_initialize_auto_load',
>> the consequence was that the new_objfile event was emitted on the Python
>> side before autoloaded scripts had been executed when a new objfile was
>> loaded.
>> As a result, trying to access the objfile's pretty printers (defined in
>> the autoloaded script) from a handler for the Python-side
>> 'new_objfile' event would fail. Those would only be initialized later on
>> (when the 'auto_load_new_objfile' callback was called).
>>
>> To make sure that the objfile passed to the Python event handler
>> is properly initialized (including its 'pretty_printers' member),
>> make sure that the 'auto_load_new_objfile' observer is notified
>> before the 'python_new_objfile' one that propagates the event
>> to the Python side.
>>
>> To do this, extend the 'observable' class to allow explicitly
>> specifying dependencies when attaching observers, by adding
>> the possibility to specify a key for the observer, and the
>> keys for other observers it depends on, and make use of that
>> mechanism to specify a dependency from 'python_new_objfile'
>> on 'auto_load_new_objfile'.
>
> In my version, I was able to re-use the gdb::observers::token things
> that already exists to identify one observer. It seems to me like it
> would better to use that instead of introducing a new key, they seeom to
> do the same thing.
>
Indeed. that's much better. I've changed it in v4.
>> To make sure dependencies are notified before observers
>> depending on them, the vector holding the observers is sorted in
>> a way that dependencies come before observers depending on them.
>> The current implementation for sorting uses the depth-first search
>> algorithm for topological sorting as described at [1].
>
> Good!
>
>> Add a corresponding testcase that involves a test library with an autoloaded
>> Python script and a handler for the Python 'new_objfile' event.
>
> Thanks for the test. In addition, we could perhaps add something to
> the unittests/observable-selftests.c file? It should be possible to
> register some observers in different orders and verify they are always
> in a correct order. These tests are in C++, so easier to write than the
> Dejagnu tests.
I've added a test there.
>
>> (The real world use case where I came across this issue was in an attempt
>> to extend handling for GDB pretty printers for dynamically loaded
>> objfiles in the Qt Creator IDE, s. [2] and [3] for more background.)
>>
>> [1] https://en.wikipedia.org/wiki/Topological_sorting#Depth-first_search
>> [2] https://bugreports.qt.io/browse/QTCREATORBUG-25339
>> [3] https://codereview.qt-project.org/c/qt-creator/qt-creator/+/333857/1
>>
>> Tested on x86_64-linux (Debian testing).
>>
>> gdb/ChangeLog:
>>
>> * auto-load.c (_initialize_auto_load): Specify key when
>> attaching the 'auto_load_new_objfile' observer, so
>> other observers can specify it as a dependency.
>> * auto-load.h (AUTO_LOAD_H): Declare
>> 'auto_load_new_objfile_observer' as key to be used
>> for the 'auto_load_new_objfile' observer.
>> * python/py-inferior.c (gdbpy_initialize_inferior): Make
>> 'python_new_objfile' observer depend on 'auto_load_new_objfile'
>> observer, so it gets notified after the latter.
>>
>> gdb/testsuite/ChangeLog:
>>
>> * gdb.python/libpy-autoloaded-pretty-printers-in-newobjfile-event.so-gdb.py: New test.
>> * gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.cc: New test.
>> * gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.h: New test.
>> * gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-main.cc: New test.
>> * gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.exp: New test.
>> * gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.py: New test.
>>
>> gdbsupport/ChangeLog:
>> * observable.h (struct observer_key): New struct that can be
>> used as a key when attaching observers, and specyfing
>> dependencies to other observers
>> (class observable): Extend to allow specifying dependencies
>> between observers, keep vector holding observers sorted
>> that dependencies are notified before observers depending
>> on them.
>> ---
>> gdb/auto-load.c | 5 +-
>> gdb/auto-load.h | 11 ++
>> gdb/python/py-inferior.c | 6 +-
>> ...tty-printers-in-newobjfile-event.so-gdb.py | 44 ++++++
>> ...pretty-printers-in-newobjfile-event-lib.cc | 26 ++++
>> ...-pretty-printers-in-newobjfile-event-lib.h | 31 +++++
>> ...retty-printers-in-newobjfile-event-main.cc | 23 ++++
>> ...ed-pretty-printers-in-newobjfile-event.exp | 96 +++++++++++++
>> ...ded-pretty-printers-in-newobjfile-event.py | 34 +++++
>> gdbsupport/observable.h | 127 ++++++++++++++++--
>> 10 files changed, 389 insertions(+), 14 deletions(-)
>> create mode 100644 gdb/testsuite/gdb.python/libpy-autoloaded-pretty-printers-in-newobjfile-event.so-gdb.py
>> create mode 100644 gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.cc
>> create mode 100644 gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.h
>> create mode 100644 gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-main.cc
>> create mode 100644 gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.exp
>> create mode 100644 gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.py
>>
>> diff --git a/gdb/auto-load.c b/gdb/auto-load.c
>> index 1dfcf21eeeb..4ecfdd28f35 100644
>> --- a/gdb/auto-load.c
>> +++ b/gdb/auto-load.c
>> @@ -1494,6 +1494,8 @@ found and/or loaded."),
>> return &retval;
>> }
>>
>> +gdb::observers::observer_key auto_load_new_objfile_observer;
>> +
>> void _initialize_auto_load ();
>> void
>> _initialize_auto_load ()
>> @@ -1503,7 +1505,8 @@ _initialize_auto_load ()
>> char *guile_name_help;
>> const char *suffix;
>>
>> - gdb::observers::new_objfile.attach (auto_load_new_objfile);
>> + gdb::observers::new_objfile.attach (auto_load_new_objfile,
>> + &auto_load_new_objfile_observer);
>>
>> add_setshow_boolean_cmd ("gdb-scripts", class_support,
>> &auto_load_gdb_scripts, _("\
>> diff --git a/gdb/auto-load.h b/gdb/auto-load.h
>> index f726126c554..e599b862ff9 100644
>> --- a/gdb/auto-load.h
>> +++ b/gdb/auto-load.h
>> @@ -25,6 +25,13 @@ struct program_space;
>> struct auto_load_pspace_info;
>> struct extension_language_defn;
>>
>> +namespace gdb
>> +{
>> +namespace observers {
>> +struct observer_key;
>> +}
>> +}
>
> You can use:
>
> namespace gdb::observers {
Done in v4.
>
> In my version, I just included observable.h, but it's true that if we
> can use just a forward-declaration, it's preferable.
>
>> +
>> /* Value of the 'set debug auto-load' configuration variable. */
>>
>> extern bool debug_auto_load;
>> @@ -40,6 +47,10 @@ extern bool auto_load_local_gdbinit;
>> extern char *auto_load_local_gdbinit_pathname;
>> extern bool auto_load_local_gdbinit_loaded;
>>
>> +/* Key used for the auto_load_new_objfile observer, so other observers can
>> + * specify it as a dependency. */
>> +extern gdb::observers::observer_key auto_load_new_objfile_observer;
>> +
>> extern struct auto_load_pspace_info *
>> get_auto_load_pspace_data_for_loading (struct program_space *pspace);
>> extern void auto_load_objfile_script (struct objfile *objfile,
>> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
>> index a3d5952a10b..6ec3f1599ca 100644
>> --- a/gdb/python/py-inferior.c
>> +++ b/gdb/python/py-inferior.c
>> @@ -18,6 +18,7 @@
>> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>>
>> #include "defs.h"
>> +#include "auto-load.h"
>> #include "gdbcore.h"
>> #include "gdbthread.h"
>> #include "inferior.h"
>> @@ -913,7 +914,10 @@ gdbpy_initialize_inferior (void)
>> gdb::observers::memory_changed.attach (python_on_memory_change);
>> gdb::observers::register_changed.attach (python_on_register_change);
>> gdb::observers::inferior_exit.attach (python_inferior_exit);
>> - gdb::observers::new_objfile.attach (python_new_objfile);
>> + // needs to run after auto_load_new_objfile observer, so autoloaded
>> + // pretty-printers are available
>
> Use capital letter, /*-style comments, period at the end:
>
> /* Need to run after auto-load's new_objfile observer, so that
> auto-loaded pretty-printers are available. */
>
>> + gdb::observers::new_objfile.attach (python_new_objfile, nullptr,
>> + { &auto_load_new_objfile_observer });
>> gdb::observers::inferior_added.attach (python_new_inferior);
>> gdb::observers::inferior_removed.attach (python_inferior_deleted);
>>
>> diff --git a/gdb/testsuite/gdb.python/libpy-autoloaded-pretty-printers-in-newobjfile-event.so-gdb.py b/gdb/testsuite/gdb.python/libpy-autoloaded-pretty-printers-in-newobjfile-event.so-gdb.py
>> new file mode 100644
>> index 00000000000..2327e4a7384
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/libpy-autoloaded-pretty-printers-in-newobjfile-event.so-gdb.py
>> @@ -0,0 +1,44 @@
>> +# Copyright (C) 2021 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# This file is part of the GDB testsuite. It tests that python pretty
>> +# printers defined in a python script that is autoloaded have been
>> +# registered when a custom event handler for the new_objfile event
>> +# is called.
>> +
>> +import gdb.printing
>> +
>> +
>> +class MyClassTestLibPrinter(object):
>> + "Print a MyClassTestLib"
>> +
>> + def __init__(self, val):
>> + self.val = val
>> +
>> + def to_string(self):
>> + return "MyClassTestLib object, id: {}".format(self.val['id'])
>> +
>> + def display_hint(self):
>> + return "string"
>> +
>> +
>> +def build_pretty_printer():
>> + pp = gdb.printing.RegexpCollectionPrettyPrinter(
>> + "my_library")
>> + pp.add_printer("MyClassTestLib", "^MyClassTestLib$", MyClassTestLibPrinter)
>> + return pp
>> +
>> +
>> +gdb.printing.register_pretty_printer(gdb.current_objfile(), build_pretty_printer())
>> diff --git a/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.cc b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.cc
>> new file mode 100644
>> index 00000000000..7e06cf3903f
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.cc
>> @@ -0,0 +1,26 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> + Copyright 2021 Free Software Foundation, Inc.
>> +
>> + This program is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU General Public License as published by
>> + the Free Software Foundation; either version 3 of the License, or
>> + (at your option) any later version.
>> +
>> + This program is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + GNU General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
>> +
>> +#include "py-autoloaded-pretty-printers-in-newobjfile-event-lib.h"
>> +
>> +MyClassTestLib::MyClassTestLib(int theId) {
>> + id = theId;
>> +}
>> +
>> +int MyClassTestLib::getId() {
>> + return id;
>> +}
>> diff --git a/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.h b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.h
>> new file mode 100644
>> index 00000000000..fa2501bf6de
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-lib.h
>> @@ -0,0 +1,31 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> + Copyright 2021 Free Software Foundation, Inc.
>> +
>> + This program is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU General Public License as published by
>> + the Free Software Foundation; either version 3 of the License, or
>> + (at your option) any later version.
>> +
>> + This program is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + GNU General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
>> +
>> +#ifndef TESTLIBRARY_H
>> +#define TESTLIBRARY_H
>> +
>> +class MyClassTestLib {
>> +
>> +public:
>> + explicit MyClassTestLib(int theId);
>> + int getId();
>> +
>> +private:
>> + int id;
>> +};
>> +
>> +#endif // TESTLIBRARY_H
>> diff --git a/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-main.cc b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-main.cc
>> new file mode 100644
>> index 00000000000..6e66bbe3d43
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event-main.cc
>> @@ -0,0 +1,23 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> + Copyright 2021 Free Software Foundation, Inc.
>> +
>> + This program is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU General Public License as published by
>> + the Free Software Foundation; either version 3 of the License, or
>> + (at your option) any later version.
>> +
>> + This program is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + GNU General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
>> +
>> +#include "py-autoloaded-pretty-printers-in-newobjfile-event-lib.h"
>> +
>> +int main() {
>> + MyClassTestLib test(1);
>> + return 0; /* break to inspect */
>> +}
>> diff --git a/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.exp b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.exp
>> new file mode 100644
>> index 00000000000..ed1c3230835
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.exp
>> @@ -0,0 +1,96 @@
>> +# Copyright (C) 2021 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# This file is part of the GDB testsuite. It tests that python pretty
>> +# printers defined in a python script that is autoloaded have been
>> +# registered when a custom event handler for the new_objfile event
>> +# is called.
>> +
>> +if [use_gdb_stub] {
>> + return 0
>> +}
>> +
>> +load_lib gdb-python.exp
>> +
>> +set test "py-autoloaded-pretty-printers-in-newobjfile-event"
>> +set srcfile_main "${test}-main.cc"
>> +set executable_main ${test}-test
>> +set binfile_main [standard_output_file ${executable_main}]
>> +set srcfile_lib "${test}-lib.cc"
>> +set libname "lib${test}"
>> +set pyscriptfile_lib "${libname}.so-gdb.py"
>> +set binfile_lib [standard_output_file ${libname}.so]
>> +
>> +
>> +# Start with a fresh gdb.
>> +gdb_exit
>> +gdb_start
>> +
>> +# Skip all tests if Python scripting is not enabled.
>> +if { [skip_python_tests] } { continue }
>> +
>> +if {[gdb_compile_shlib ${srcdir}/${subdir}/${srcfile_lib} ${binfile_lib} \
>> + [list debug c++ -Wl,-soname,${libname}.so]] != ""} {
>> + return -1
>> +}
>> +
>> +if {[gdb_compile "${srcdir}/${subdir}/${srcfile_main}" "${binfile_main}.o" \
>> + object [list debug c++]] != ""} {
>> + return -1
>> +}
>> +
>> +set testobjdir [standard_output_file {}]
>> +if {[gdb_compile "${binfile_main}.o" "${binfile_main}" executable \
>> + [list debug "additional_flags=-L$testobjdir -l${test} \
>> + -Wl,-rpath=$testobjdir"]] != ""} {
>> + return -1
>> +}
>> +
>> +# Start with a fresh gdb.
>> +gdb_exit
>> +gdb_start
>> +gdb_reinitialize_dir $srcdir/$subdir
>> +
>> +# Make the -gdb.py script available to gdb, it is automatically loaded by
>> +# gdb if it is put in the same directory as the library.
>> +set remote_python_file_autoload [gdb_remote_download host \
>> + ${srcdir}/${subdir}/${pyscriptfile_lib}]
>> +
>> +gdb_test_no_output "set auto-load safe-path ${remote_python_file_autoload}" \
>> + "set auto-load safe-path"
>> +
>> +# load the python file that defines a handler for the new_objfile event,
>> +# which will generate the output to check later
>> +# (prints information on available pretty-printers for objfile)
>> +set remote_python_file [gdb_remote_download host \
>> + ${srcdir}/${subdir}/${test}.py]
>> +gdb_test_no_output "source ${remote_python_file}" "load python file"
>> +
>> +gdb_load ${binfile_main}
>> +
>> +gdb_test_no_output "set print pretty on"
>> +
>> +gdb_breakpoint [gdb_get_line_number "break to inspect" ${srcfile_main}]
>> +
>> +# check that the handler prints output when test library is loaded
>> +# and that the pretty printer from autoloaded python file has been registered
>> +gdb_test "run" "new_objfile event for test library.*
>> +.*number of pretty printers: 1.*
>> +.*name of the first pretty printer: my_library.*"
>> +
>> +# check that pretty printer actually works
>> +gdb_test "print test" " .*MyClassTestLib object, id: 1.*"
>> +
>> +gdb_continue_to_end
>> diff --git a/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.py b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.py
>> new file mode 100644
>> index 00000000000..924f304fd33
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.py
>> @@ -0,0 +1,34 @@
>> +# Copyright (C) 2021 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# This file is part of the GDB testsuite. It tests that python pretty
>> +# printers defined in a python script that is autoloaded have been
>> +# registered when a custom event handler for the new_objfile event
>> +# is called.
>> +
>> +import gdb
>> +import os
>> +
>> +
>> +def new_objfile_handler(event):
>> + assert(isinstance(event, gdb.NewObjFileEvent))
>> + # only observe the custom test library
>> + if os.path.basename(event.new_objfile.filename).find("libpy-autoloaded-pretty-printers-in-newobjfile-event") == 0:
>> + print("new_objfile event for test library")
>> + assert(len(event.new_objfile.pretty_printers) == 1)
>> + print("number of pretty printers: {}".format(len(event.new_objfile.pretty_printers)))
>> + print("name of the first pretty printer: {}".format(event.new_objfile.pretty_printers[0].name))
>> +
>> +gdb.events.new_objfile.connect(new_objfile_handler)
>> diff --git a/gdbsupport/observable.h b/gdbsupport/observable.h
>> index 1a9e767ba18..524f98e5a78 100644
>> --- a/gdbsupport/observable.h
>> +++ b/gdbsupport/observable.h
>> @@ -52,6 +52,13 @@ struct token
>> DISABLE_COPY_AND_ASSIGN (token);
>> };
>>
>> +struct observer_key
>> +{
>> + observer_key () = default;
>> +
>> + DISABLE_COPY_AND_ASSIGN (observer_key);
>> +};
>> +
>> template<typename... T>
>> class observable
>> {
>> @@ -59,6 +66,22 @@ class observable
>>
>> typedef std::function<void (T...)> func_type;
>>
>> +private:
>> + struct observer_entry
>> + {
>> + const token *tok;
>> + func_type func;
>> + observer_key *key;
>> + std::vector<observer_key *> deps;
>
> You could make all the fields const.
>
I've only made the template type for the std::vector const,
i.e. changed
std::vector<observer_key *> deps;
to
std::vector<const struct token *> dependencies;
Using
const std::vector<const struct token *> dependencies;
wouldn't compile due to the handling in the 'detach' method.
I didn't change 'func_type func' to const since that is already added
in a commit on your branch (but can of course update that if
sending a patch series with your commits included).
>> +
>> + observer_entry (const token *t, func_type f, observer_key *k,
>> + std::vector<observer_key *> d)
>> + : tok (t), func (f), key (k), deps (d)
>> + {
>> + }
>
> Put the constructor before the fields. Make `d` a const-reference.
>
Done in v4 (most of it already contained in your branch).
>> + };
>> +
>> +public:
>> explicit observable (const char *name)
>> : m_name (name)
>> {
>> @@ -67,17 +90,29 @@ class observable
>> DISABLE_COPY_AND_ASSIGN (observable);
>>
>> /* Attach F as an observer to this observable. F cannot be
>> - detached. */
>> - void attach (const func_type &f)
>> + detached.
>> + Optional:
>> + * key that can be used to specify a dependency on
>> + the attached observer
>> + * keys for other observers this observer depends on
>> + Dependencies are notified before observers depending on them. */
>> + void attach (const func_type &f, observer_key *key = nullptr,
>> + std::vector<observer_key *> dependencies = {})
>> {
>> - m_observers.emplace_back (nullptr, f);
>> + attach (f, nullptr, key, dependencies);
>> }
>>
>> /* Attach F as an observer to this observable. T is a reference to
>> - a token that can be used to later remove F. */
>> - void attach (const func_type &f, const token &t)
>> + a token that can be used to later remove F.
>> + Optional:
>> + * key that can be used to specify a dependency on
>> + the attached observer
>> + * keys for other observers this observer depends on
>> + Dependencies are notified before observers depending on them. */
>> + void attach (const func_type &f, const token &t, observer_key *key = nullptr,
>> + std::vector<observer_key *> dependencies = {})
>> {
>> - m_observers.emplace_back (&t, f);
>> + attach (f, &t, key, dependencies);
>> }
>>
>> /* Remove observers associated with T from this observable. T is
>> @@ -87,10 +122,9 @@ class observable
>> {
>> auto iter = std::remove_if (m_observers.begin (),
>> m_observers.end (),
>> - [&] (const std::pair<const token *,
>> - func_type> &e)
>> + [&] (const observer_entry &e)
>> {
>> - return e.first == &t;
>> + return e.tok == &t;
>> });
>>
>> m_observers.erase (iter, m_observers.end ());
>> @@ -103,13 +137,82 @@ class observable
>> fprintf_unfiltered (gdb_stdlog, "observable %s notify() called\n",
>> m_name);
>> for (auto &&e : m_observers)
>> - e.second (args...);
>> + e.func (args...);
>> }
>>
>> private:
>> -
>> - std::vector<std::pair<const token *, func_type>> m_observers;
>> + std::vector<observer_entry> m_observers;
>> const char *m_name;
>> +
>> + /* used for sorting algorithm */
>
> Use capital letter and period at the end and two spaces after the
> period:
>
> /* Use for sorting algorithm. */
>
Done.
>> + enum class mark
>> + {
>> + NONE,
>> + PERMANENT,
>> + TEMPORARY
>
> A bit of nit-picking, but since I think good naming helps: what would
> you think of renaming:
>
> - NONE -> NOT_VISITED
> - TEMPORARY -> VISITING
> - PERMANENT -> VISITED
>
That's indeed much better and I've renamed it in v4.
>> + };
>> +
>> + /* Helper method for topological sort using depth-first search algorithm */
>> + void visit_for_sorting (std::vector<observer_entry> &sorted_elems,
>> + std::vector<mark> &marks, int index)
>
> Align the second line with the parenthesis:
>
> void visit_for_sorting (std::vector<observer_entry> &sorted_elems,
> std::vector<mark> &marks, int index)
>
>
Done
>
>> + {
>> + if (marks[index] == mark::PERMANENT)
>> + return;
>> + if (marks[index] == mark::TEMPORARY)
>> + error (_("Cyclic dependencies in observers."));
>
> Since such a cycle would be the result of a programming error in GDB,
> and not the result of bad input, for example, let's use gdb_assert:
>
> /* If we already visited this observer, it means there's a cycle. */
> gdb_assert (marks[index] != mark::TEMPORARY);
Done.
>
>> +
>> + marks[index] = mark::TEMPORARY;
>> +
>> + for (observer_key *dep : m_observers[index].deps)
>> + {
>> + auto it_dep
>> + = std::find_if (m_observers.begin (), m_observers.end (),
>> + [&] (observer_entry e) { return e.key == dep; });
>> + if (it_dep != m_observers.end ())
>> + {
>> + int i = std::distance (m_observers.begin (), it_dep);
>> + visit_for_sorting (sorted_elems, marks, i);
>> + }
>> + }
>> +
>> + marks[index] = mark::PERMANENT;
>> + sorted_elems.push_back (m_observers[index]);
>> + }
>> +
>> + /* Sorts the elements, so that dependencies come before observers
>> + * depending on them.
>> + *
>> + * Currently uses depth-first search algorithm for topological sorting,
>> + * s. https://en.wikipedia.org/wiki/Topological_sorting#Depth-first_search . */
>
> Remove the leading asterisks starting from the 2nd line of the command,
> and use two spaces after the final period.
Done.
>
>> + void sort_elements ()
>> + {
>> + std::vector<observer_entry> sorted_elems;
>> + std::vector<mark> marks (m_observers.size (), mark::NONE);
>> +
>> + auto it = marks.begin();
>
> Space before paren.
That line was dropped altogether in v4.
>
>> + while (it != marks.end ())
>> + {
>> + int index = std::distance (marks.begin (), it);
>> + visit_for_sorting (sorted_elems, marks, index);
>> +
>> + it = std::find_if (marks.begin (), marks.end (),
>> + [] (mark m) { return m == mark::NONE; });
>> + }
>
> I think it would be simpler to understand as a simple for loop:
>
> for (size_t i = 0; i < m_observers.size (); i++)
> visit_for_sorting (sorted_elems, marks, index);
>
> That avoids restarting the search for a mark::NONE element at the start
> at each iteration.
Done, that's indeed much simpler.
>
>> + // assign sorted result
>> + m_observers = sorted_elems;
>
> That could be
>
> m_observers = std::move (sorted_elems);
Done.
>
>> + }
>> +
>> + void attach (const func_type &f, const token *t, observer_key *key,
>> + std::vector<observer_key *> dependencies)
>
> Align the second line with the parenthesis.
Done.
>
>> + {
>> + m_observers.emplace_back (t, f, key, dependencies);
>> +
>> + if (key != nullptr)
>> + {
>> + // other observers might depend on this one -> sort anew
>> + sort_elements ();
>> + }
>
> I think we need to call sort_elements even if key == nullptr, because
> the two observers can be added in any order. If observer A depends on
> B, A won't have a key (it doesn't need to). If B if registered, then A,
> you want to sort at that time. So I would just sort all the time on
> attach.
I think that case should be fine without reordering, because the observer
is initially always added at the end of the vector (using 'emplace_back'),
i.e. after all potential dependencies that have been inserted earlier),
i.e. A already comes after B if it is registered later.
The idea was to avoid reordering the vector if it's not necessary, but
I don't know whether that makes any difference performance-wise in practice
and can drop that if you think it makes sense.
(I've left it in v4 to hear about your opinion first.)
>
> Thanks,
>
> Simon
>
Thanks,
Michael
More information about the Gdb-patches
mailing list