From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 122020 invoked by alias); 15 Aug 2019 02:58:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 121924 invoked by uid 89); 15 Aug 2019 02:58:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=H*r:sk:server-, H*r:10.0.0, UD:p.m, p.m X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Aug 2019 02:58:14 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 88C311E059; Wed, 14 Aug 2019 22:58:12 -0400 (EDT) Subject: Re: [PATCH v2] Make GDB compile with Python 3 on MinGW To: Christian Biesinger , gdb-patches@sourceware.org References: <20190814000511.140810-1-cbiesinger@google.com> <20190814160611.104374-1-cbiesinger@google.com> From: Simon Marchi Message-ID: <7c69bd9a-8fc3-f82b-4e68-57788151fb16@simark.ca> Date: Thu, 15 Aug 2019 02:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190814160611.104374-1-cbiesinger@google.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-08/txt/msg00316.txt.bz2 On 2019-08-14 12:06 p.m., Christian Biesinger via gdb-patches wrote: > [Now with the comment updated too] > > PyFile_FromString and PyFile_AsFile have been removed in Python 3. > There is no obvious replacement that works here, and we can't just > pass our FILE* to a DLL in Windows because it may use a different > C runtime. > > So we just call a Python function which reads and executes file > contents. Care must be taken to execute it in the context of > __main__. > > Tested by inverting the ifdef and running the testsuite on Debian > Linux (even without the patch, I failed at running the testsuite > on Windows). I haven't deciphered the Python part yet, but here are some early comments. Did you test with both Python 2 and 3? > gdb/ChangeLog: > > 2019-08-13 Christian Biesinger > > * python/lib/gdb/__init__.py: Add an execute_file function. > * python/python.c (python_run_simple_file): Call gdb.execute_file > on Windows. > --- > gdb/python/lib/gdb/__init__.py | 23 +++++++++++++++++++++++ > gdb/python/python.c | 23 +++++++++++++++-------- > 2 files changed, 38 insertions(+), 8 deletions(-) > > diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py > index af74df80c8..f1adc5ffbe 100644 > --- a/gdb/python/lib/gdb/__init__.py > +++ b/gdb/python/lib/gdb/__init__.py > @@ -106,6 +106,29 @@ def execute_unwinders(pending_frame): > > return None > > +def execute_file(filepath): I would suggest naming this "_execute_file", to emphasize that's internal, not to be used by users. > + """This function is used to replace Python 2's PyRun_SimpleFile. > + > + Loads and executes the given file. > + > + We could use the runpy module, but its documentation says: > + "Furthermore, any functions and classes defined by the executed code are > + not guaranteed to work correctly after a runpy function has returned." > + """ > + globals = sys.modules['__main__'].__dict__ > + set_file = False > + if not hasattr(globals, '__file__'): > + globals['__file__'] = filepath > + set_file = True What does setting __file__ help with? A comment explaining why this is needed would be nice. > + try: > + with open(filepath, 'rb') as file: > + # We pass globals also as locals to match what Python does > + # in PyRun_SimpleFile. > + exec(compile(file.read(), filepath, 'exec'), globals, globals) Could you split this line (the exec and compile) in two? If there is an error coming out of either of them, and we get a backtrace, it will be easier to know which one it is if they are on separate lines. > + finally: > + if set_file: > + del globals['__file__'] > + > > # Convenience variable to GDB's python directory > PYTHONDIR = os.path.dirname(os.path.dirname(__file__)) > diff --git a/gdb/python/python.c b/gdb/python/python.c > index 162470dcc0..617bc0b84c 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -323,9 +323,8 @@ python_interactive_command (const char *arg, int from_tty) > A FILE * from one runtime does not necessarily operate correctly in > the other runtime. > > - To work around this potential issue, we create on Windows hosts the > - FILE object using Python routines, thus making sure that it is > - compatible with the Python library. */ > + To work around this potential issue, we run code in Python to load > + the script. */ > > static void > python_run_simple_file (FILE *file, const char *filename) > @@ -339,14 +338,22 @@ python_run_simple_file (FILE *file, const char *filename) > /* Because we have a string for a filename, and are using Python to > open the file, we need to expand any tilde in the path first. */ > gdb::unique_xmalloc_ptr full_path (tilde_expand (filename)); > - gdbpy_ref<> python_file (PyFile_FromString (full_path.get (), (char *) "r")); > - if (python_file == NULL) > + > + if (gdb_python_module == nullptr > + || ! PyObject_HasAttrString (gdb_python_module, "execute_file")) > { > - gdbpy_print_stack (); > - error (_("Error while opening file: %s"), full_path.get ()); > + error (_("Installation error: gdb.execute_file function is missing")); > + return; > } > > - PyRun_SimpleFile (PyFile_AsFile (python_file.get ()), filename); > + gdbpy_ref<> return_value > + (PyObject_CallMethod (gdb_python_module, "execute_file", "s", full_path.get ())); Wrap the line above to fit in 80 columns: gdbpy_ref<> return_value (PyObject_CallMethod (gdb_python_module, "execute_file", "s", full_path.get ())); > + if (return_value == nullptr) > + { > + /* Use PyErr_PrintEx instead of gdbpy_print_stack to better match the > + behavior of the non-Windows codepath. */ > + PyErr_PrintEx(0); I don't think PyErr_PrintEx clears the Python error indicator, and I think we want it cleared when we exit this function, so should we call PyErr_Clear here? Simon