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

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

Hi Andrew,

On 11/02/2021 12.23, Michael Weghorn via Gdb-patches wrote:
> 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.

Is there anything to do from my side at this point in time (e.g. 
implement the "crazy thought" mentioned above and send that as a v3) or 
should I just wait a bit for now?
(I don't want to be impatient, just clarify whether I should actively be 
doing anything right now.)

Michael

> 
> 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
>>>