From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by sourceware.org (Postfix) with ESMTPS id E2AAA385781C for ; Thu, 11 Feb 2021 11:23:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E2AAA385781C Received: from submission (posteo.de [89.146.220.130]) by mout01.posteo.de (Postfix) with ESMTPS id 8BFDE160064 for ; Thu, 11 Feb 2021 12:23:06 +0100 (CET) Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4DbvPk118Sz9rxQ; Thu, 11 Feb 2021 12:23:05 +0100 (CET) Subject: Re: [PATCH] gdb: Change init order so pretty printers are set in new_objfile event To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <20210210154053.82927-1-m.weghorn@posteo.de> <20210211094234.GM265215@embecosm.com> From: Michael Weghorn Message-ID: <8ec59d83-831b-fd79-ec31-dca3c817fe4c@posteo.de> Date: Thu, 11 Feb 2021 12:23:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210211094234.GM265215@embecosm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Feb 2021 11:23:11 -0000 On 11/02/2021 10.42, Andrew Burgess wrote: > * Michael Weghorn via Gdb-patches [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 . >> + >> +# 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 . */ >> + >> +#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 . */ >> + >> +#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 . */ >> + >> +#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 . >> + >> +# 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 . >> + >> +# 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 >>