Re: [PATCH] gdb: Change init order so pretty printers are set in new_objfile event

[Michael Weghorn <m.weghorn@posteo.de>]

On 11/02/2021 10.42, Andrew Burgess wrote:
> * Michael Weghorn via Gdb-patches <gdb-patches@sourceware.org> [2021-02-10 16:40:53 +0100]:
> 
>> The observers attached to the 'gdb::observers::new_objfile' observable
>> are notified in the order in which they have been attached (s. class
>> 'observable' in gdbsupport/observable.h).
>>
>> 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 the previous initialization order, '_initialize_python' happened
>> before '_initialize_auto_load', with the consequence 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), change
>> the initialization order so that autoloading happens before the
>> propagation of the 'new_objfile' event to the Python side.
>>
>> The initialization happens in 'initialize_all_files' in 'gdb/init.c'
>> which is autogenerated by Makefile 'gdb/Makefile.in', so change the
>> order there accordingly by moving the 'CONFIG_OBS' part (which includes
>> the Python part) after the 'COMMON_SFILES' one (which includes the
>> autoload part).
> 
> I have a crazy thought, but I want to mention it in case it has any
> value.
> 
> The core of the idea is to modify the attach functions in
> gdbsupport/observable.h like this:P
> 
>    void attach (const func_type &f, int bias = 0)
>    {
>      m_observers.emplace_back (nullptr, f, bias);
>      if (m_sorted || bias != 0)
>        {
> 	m_sorted = true;
> 	std::sort (m_observers.begin (), m_observers.end (), sort_by_bias);
>        }
>    }
> 
> Where 'm_sorted' is just a boolean flag, and `sort_by_bias` just
> orders the observers from highest bias to lowest.  Then, if you have
> an observer that you want to run late in the process you just attach
> it with a low bias.  So in py-inferior.c:
> 
>    /* Pass a low bias to ensure this observer triggers after any
>       GDB internal observers.  */
>    gdb::observers::new_objfile.attach (python_new_objfile, -10);

To me, that sounds like a reasonable way to do it, in particular if the 
need for a more fine-grained control arises in other places as well.
I think you and other experienced developers can judge better
what's the best way to handle it.

> 
> That said, I looked through the patch and it all looked good except
> for a couple of nits, pointed out below.

Thanks a lot for the quick review. I've sent an updated v2 which 
addresses those nits, but is otherwise unchanged for now.

Michael

> 
> Thanks,
> Andrew
> 
>>
>> Add a corresponding testcase that involves a test library with an autoloaded
>> Python script and a handler for the Python 'new_objfile' event.
>>
>> (The real world use case where I came across this was in an attempt to
>> extend handling for GDB pretty printers for dynamically loaded
>> objfiles in the Qt Creator IDE, s. [1] and [2] for more background.)
>>
>> [1] https://bugreports.qt.io/browse/QTCREATORBUG-25339
>> [2] https://codereview.qt-project.org/c/qt-creator/qt-creator/+/333857/1
>>
>> Tested on x86_64-linux (Debian testing).
>>
>> gdb/ChangeLog:
>>
>>          * Makefile.in: Change order so initialization for 'CONFIG_OBS'
>>          happens after 'COMMON_SFILES' so e.g. autoloaded Python pretty
>>          printers are available in handler for Python new_objfile event handler.
>>
>> 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.
>> ---
>>   gdb/Makefile.in                               |  3 +-
>>   ...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 | 93 +++++++++++++++++++
>>   ...ded-pretty-printers-in-newobjfile-event.py | 34 +++++++
>>   7 files changed, 253 insertions(+), 1 deletion(-)
>>   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/Makefile.in b/gdb/Makefile.in
>> index 36ef45d4559..2c34714b66d 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -1573,11 +1573,12 @@ TAGFILES_NO_SRCDIR = $(SFILES) $(HFILES_NO_SRCDIR) $(ALLDEPFILES) \
>>   	$(CONFIG_SRCS)
>>   TAGFILES_WITH_SRCDIR = $(HFILES_WITH_SRCDIR)
>>   
>> -COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>> +COMMON_OBS = $(DEPFILES) $(YYOBJ) \
>>   	mi/mi-common.o \
>>   	version.o \
>>   	xml-builtin.o \
>>   	$(patsubst %.c,%.o,$(COMMON_SFILES)) \
>> +	$(CONFIG_OBS) \
>>   	$(SUBDIR_CLI_OBS) \
>>   	$(SUBDIR_TARGET_OBS) \
>>   	$(SUBDIR_GCC_COMPILE_OBS)
>> 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..28ca712a3ce
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-autoloaded-pretty-printers-in-newobjfile-event.exp
>> @@ -0,0 +1,93 @@
>> +# 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]] != ""} {
> 
> This long line should probably be split.
> 
>> +    return -1
>> +}
>> +
>> +if {[gdb_compile "${srcdir}/${subdir}/${srcfile_main}" "${binfile_main}.o" object [list debug c++]] != ""} {
> 
> And again.
> 
>> +    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"]] != ""} {
> 
> If feels like you could reduce the whitespace on the last line here to
> keep it under 80 characters.
> 
>> +    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"
> 
> Long line again.
> 
>> +
>> +# 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)
>> -- 
>> 2.30.0
>>