[v4,4/8] Python QUIT processing updates

  See the previous patches in this series for the motivation behind
these changes.

This commit contains updates to Python's QUIT handling.  Ideally, we'd
like to throw gdb_exception_forced_quit through the extension
language; I made an attempt to do this for gdb_exception_quit in an
earlier version of this patch, but Pedro pointed out that it is
(almost certainly) not safe to do so.

Still, we definitely don't want to swallow the exception representing
a SIGTERM for GDB, nor do we want to force modules written in the
extension language to have to explicitly handle this case.  Since the
idea is for GDB to cleanup and quit for this exception, we'll simply
call quit_force() just as if the gdb_exception_forced_quit propagation
had managed to make it back to the top level.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
---
 gdb/python/py-finishbreakpoint.c | 5 +++++
 gdb/python/py-gdb-readline.c     | 4 ++++
 gdb/python/py-symbol.c           | 5 +++++
 gdb/python/py-utils.c            | 3 +++
 gdb/python/py-value.c            | 5 +++++
 5 files changed, 22 insertions(+)
  

On 2023-01-12 1:56 a.m., Kevin Buettner wrote:
> See the previous patches in this series for the motivation behind
> these changes.
> 
> This commit contains updates to Python's QUIT handling.  Ideally, we'd
> like to throw gdb_exception_forced_quit through the extension
> language; I made an attempt to do this for gdb_exception_quit in an
> earlier version of this patch, but Pedro pointed out that it is
> (almost certainly) not safe to do so.
> 
> Still, we definitely don't want to swallow the exception representing
> a SIGTERM for GDB, nor do we want to force modules written in the
> extension language to have to explicitly handle this case.  Since the
> idea is for GDB to cleanup and quit for this exception, we'll simply
> call quit_force() just as if the gdb_exception_forced_quit propagation
> had managed to make it back to the top level.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761

I guess this is OK as long as we don't hook up Python at a very low level
where running quit_force() would be dangerous.

Alternatively, couldn't we force some Python exception/error which is not
catch-able from Python code?  Set some error flag in the Python interpreter
or something, so that it aborts the Python code as soon as we return to the
Python interpreter.  We'd use the set_quit_flag() trick here where you're calling
quit_force (and instead of calling quit_force), so that as soon as we get out of
the Python interpreter on the other end, we'd re-raise gdb_exception_forced_quit.

Pedro Alves

> ---
>  gdb/python/py-finishbreakpoint.c | 5 +++++
>  gdb/python/py-gdb-readline.c     | 4 ++++
>  gdb/python/py-symbol.c           | 5 +++++
>  gdb/python/py-utils.c            | 3 +++
>  gdb/python/py-value.c            | 5 +++++
>  5 files changed, 22 insertions(+)
> 
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index d4d129110e9..1a224b35779 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -20,6 +20,7 @@
>  
>  
>  #include "defs.h"
> +#include "top.h"		/* For quit_force().  */
>  #include "python-internal.h"
>  #include "breakpoint.h"
>  #include "frame.h"
> @@ -271,6 +272,10 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  	    }
>  	}
>      }
> +  catch (const gdb_exception_forced_quit &except)
> +    {
> +      quit_force (NULL, 0);
> +    }
>    catch (const gdb_exception &except)
>      {
>        /* Just swallow.  Either the return type or the function value
> diff --git a/gdb/python/py-gdb-readline.c b/gdb/python/py-gdb-readline.c
> index ea0f78c9ad8..b9294ad9afc 100644
> --- a/gdb/python/py-gdb-readline.c
> +++ b/gdb/python/py-gdb-readline.c
> @@ -46,6 +46,10 @@ gdbpy_readline_wrapper (FILE *sys_stdin, FILE *sys_stdout,
>        p = command_line_input (buffer, prompt, "python");
>      }
>    /* Handle errors by raising Python exceptions.  */
> +  catch (const gdb_exception_forced_quit &e)
> +    {
> +      quit_force (NULL, 0);
> +    }
>    catch (const gdb_exception &except)
>      {
>        /* Detect user interrupt (Ctrl-C).  */
> diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> index b8777966c47..899b0787582 100644
> --- a/gdb/python/py-symbol.c
> +++ b/gdb/python/py-symbol.c
> @@ -18,6 +18,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include "defs.h"
> +#include "top.h"		/* For force_quit ().  */
>  #include "block.h"
>  #include "frame.h"
>  #include "symtab.h"
> @@ -515,6 +516,10 @@ gdbpy_lookup_static_symbol (PyObject *self, PyObject *args, PyObject *kw)
>  	= get_selected_frame (_("No frame selected."));
>        block = get_frame_block (selected_frame, NULL);
>      }
> +  catch (const gdb_exception_forced_quit &e)
> +    {
> +      quit_force (NULL, 0);
> +    }
>    catch (const gdb_exception &except)
>      {
>        /* Nothing.  */
> diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
> index 624b90a827f..d5b07a80d82 100644
> --- a/gdb/python/py-utils.c
> +++ b/gdb/python/py-utils.c
> @@ -18,6 +18,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include "defs.h"
> +#include "top.h"		/* For quit_force ().  */
>  #include "charset.h"
>  #include "value.h"
>  #include "python-internal.h"
> @@ -219,6 +220,8 @@ gdbpy_convert_exception (const struct gdb_exception &exception)
>  
>    if (exception.reason == RETURN_QUIT)
>      exc_class = PyExc_KeyboardInterrupt;
> +  else if (exception.reason == RETURN_FORCED_QUIT)
> +    quit_force (NULL, 0);
>    else if (exception.error == MEMORY_ERROR)
>      exc_class = gdbpy_gdb_memory_error;
>    else
> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
> index dcc92e51b60..9473468035b 100644
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -18,6 +18,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include "defs.h"
> +#include "top.h"		/* For quit_force ().  */
>  #include "charset.h"
>  #include "value.h"
>  #include "language.h"
> @@ -371,6 +372,10 @@ valpy_get_address (PyObject *self, void *closure)
>  	  res_val = value_addr (val_obj->value);
>  	  val_obj->address = value_to_value_object (res_val);
>  	}
> +      catch (const gdb_exception_forced_quit &except)
> +	{
> +	  quit_force (NULL, 0);
> +	}
>        catch (const gdb_exception &except)
>  	{
>  	  val_obj->address = Py_None;
>
  

On Mon, 30 Jan 2023 19:01:16 +0000
Pedro Alves <pedro@palves.net> wrote:

> On 2023-01-12 1:56 a.m., Kevin Buettner wrote:
> > See the previous patches in this series for the motivation behind
> > these changes.
> > 
> > This commit contains updates to Python's QUIT handling.  Ideally, we'd
> > like to throw gdb_exception_forced_quit through the extension
> > language; I made an attempt to do this for gdb_exception_quit in an
> > earlier version of this patch, but Pedro pointed out that it is
> > (almost certainly) not safe to do so.
> > 
> > Still, we definitely don't want to swallow the exception representing
> > a SIGTERM for GDB, nor do we want to force modules written in the
> > extension language to have to explicitly handle this case.  Since the
> > idea is for GDB to cleanup and quit for this exception, we'll simply
> > call quit_force() just as if the gdb_exception_forced_quit propagation
> > had managed to make it back to the top level.
> > 
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761  
> 
> I guess this is OK as long as we don't hook up Python at a very low level
> where running quit_force() would be dangerous.
> 
> Alternatively, couldn't we force some Python exception/error which
> is not catch-able from Python code?  Set some error flag in the
> Python interpreter or something, so that it aborts the Python code
> as soon as we return to the Python interpreter.  We'd use the
> set_quit_flag() trick here where you're calling quit_force (and
> instead of calling quit_force), so that as soon as we get out of the
> Python interpreter on the other end, we'd re-raise
> gdb_exception_forced_quit.

I've looked into doing this, but I haven't found a way to force/throw
an uncatchable Python error or exception.

We could define a Python exception, say GDBForcedQuitException, that's
derived from BaseException, but it can still be caught either via:

    try:
      ...
    except GDBForcedQuitException:
      ...

Or:

    try:
      ...
    except BaseException:
      ...

Or even:

    try:
      ...
    except:
      ...

The latter construct will basically catch everything.  This is why
there are no uncatchable exceptions (or errors) in Python.  That said,
deriving GDBForcedQuitException from BaseException (instead of Exception)
means that it can't be caught via "except Exception:", which is a catch-all
for normal exceptions.

I also took a look at calling PyErr_SetInterruptEx (SIGTERM) which
would simulate the effect of the Python layer receiving a SIGTERM
signal.  For Ctrl-C/SIGINT, We use something similar - a call to
PyErr_SetInterrupt() which is equivalent to PyErr_SetInterruptEx(SIGINT).
However, for SIGINT, python defines the default signal handler for
SIGINT to raise the KeyboardInterrupt exception.  However, from my
reading of the documentation, Python doesn't have a default handler
for SIGTERM.  The documentation says that such signals will be ignored.

Even though it's not uncatchable, it might be worth pursuing your
idea via an exception derived from BaseException as described above.
I started looking into this, but doing it looks somewhat involved.
So, instead of including it in this series, I'd prefer to push this
commit as-is and work on raising a GDBForcedQuitException in a separate
series.

Kevin
  

Hi Kevin,

On 2023-02-20 2:52 a.m., Kevin Buettner wrote:
> On Mon, 30 Jan 2023 19:01:16 +0000
> Pedro Alves <pedro@palves.net> wrote:
> 
>> On 2023-01-12 1:56 a.m., Kevin Buettner wrote:
>>> See the previous patches in this series for the motivation behind
>>> these changes.
>>>
>>> This commit contains updates to Python's QUIT handling.  Ideally, we'd
>>> like to throw gdb_exception_forced_quit through the extension
>>> language; I made an attempt to do this for gdb_exception_quit in an
>>> earlier version of this patch, but Pedro pointed out that it is
>>> (almost certainly) not safe to do so.
>>>
>>> Still, we definitely don't want to swallow the exception representing
>>> a SIGTERM for GDB, nor do we want to force modules written in the
>>> extension language to have to explicitly handle this case.  Since the
>>> idea is for GDB to cleanup and quit for this exception, we'll simply
>>> call quit_force() just as if the gdb_exception_forced_quit propagation
>>> had managed to make it back to the top level.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761  
>>
>> I guess this is OK as long as we don't hook up Python at a very low level
>> where running quit_force() would be dangerous.
>>
>> Alternatively, couldn't we force some Python exception/error which
>> is not catch-able from Python code?  Set some error flag in the
>> Python interpreter or something, so that it aborts the Python code
>> as soon as we return to the Python interpreter.  We'd use the
>> set_quit_flag() trick here where you're calling quit_force (and
>> instead of calling quit_force), so that as soon as we get out of the
>> Python interpreter on the other end, we'd re-raise
>> gdb_exception_forced_quit.
> 
> I've looked into doing this, but I haven't found a way to force/throw
> an uncatchable Python error or exception.

Thanks for checking.  I was hoping that we could at least do something
like calling something like Py_Finalize() from within a Python callback,
but of course a version of that function that signaled that were bringing
down the interpreter, without causing Python to crash as soon as we're
out of the callback back into Python.

> 
> We could define a Python exception, say GDBForcedQuitException, that's
> derived from BaseException, but it can still be caught either via:
> 
>     try:
>       ...
>     except GDBForcedQuitException:
>       ...
> 
> Or:
> 
>     try:
>       ...
>     except BaseException:
>       ...
> 
> Or even:
> 
>     try:
>       ...
>     except:
>       ...
> 
> The latter construct will basically catch everything.  This is why
> there are no uncatchable exceptions (or errors) in Python.  That said,
> deriving GDBForcedQuitException from BaseException (instead of Exception)
> means that it can't be caught via "except Exception:", which is a catch-all
> for normal exceptions.
> 
> I also took a look at calling PyErr_SetInterruptEx (SIGTERM) which
> would simulate the effect of the Python layer receiving a SIGTERM
> signal.  For Ctrl-C/SIGINT, We use something similar - a call to
> PyErr_SetInterrupt() which is equivalent to PyErr_SetInterruptEx(SIGINT).
> However, for SIGINT, python defines the default signal handler for
> SIGINT to raise the KeyboardInterrupt exception.  However, from my
> reading of the documentation, Python doesn't have a default handler
> for SIGTERM.  The documentation says that such signals will be ignored.
> 
> Even though it's not uncatchable, it might be worth pursuing your
> idea via an exception derived from BaseException as described above.
> I started looking into this, but doing it looks somewhat involved.
> So, instead of including it in this series, I'd prefer to push this
> commit as-is and work on raising a GDBForcedQuitException in a separate
> series.

Yes, let's do that.

Thanks again for the investigation.