[PATCH][gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

[PATCH][gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

[Tom de Vries]

Hi,

The test-case gdb.gdb/python-interrupts.exp:
- runs to captured_command_loop
- sets a breakpoint at set_active_ext_lang
- calls a python command
- verifies the command triggers the breakpoint
- sends a signal and verifies the result

The test-case is fragile, because (f.i. with -flto) it cannot be guaranteed
that captured_command_loop and set_active_ext_lang are available for setting
breakpoints.

Reimplement the test-case as unittest, using:
- execute_command_to_string to capture the output
- try/catch to catch the "Error while executing Python code" exception
- a new hook selftests::hook_set_active_ext_lang to raise the signal

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

---
 gdb/extension.c     | 12 +++++++++++
 gdb/python/python.c | 62 ++++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 59 insertions(+), 15 deletions(-)

diff --git a/gdb/extension.c b/gdb/extension.c
index 27dce9befa0..3edecb21d6b 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -682,6 +682,13 @@ install_gdb_sigint_handler (struct signal_handler *previous)
     previous->handler_saved = 0;
 }
 
+#if GDB_SELF_TEST
+namespace selftests {
+extern void (*hook_set_active_ext_lang) (void);
+void (*hook_set_active_ext_lang) (void) = nullptr;
+}
+#endif
+
 /* Set the currently active extension language to NOW_ACTIVE.
    The result is a pointer to a malloc'd block of memory to pass to
    restore_active_ext_lang.
@@ -708,6 +715,11 @@ install_gdb_sigint_handler (struct signal_handler *previous)
 struct active_ext_lang_state *
 set_active_ext_lang (const struct extension_language_defn *now_active)
 {
+#if GDB_SELF_TEST
+  if (selftests::hook_set_active_ext_lang)
+    selftests::hook_set_active_ext_lang ();
+#endif
+
   struct active_ext_lang_state *previous
     = XCNEW (struct active_ext_lang_state);
 
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 34c1be76a16..b17b6b69268 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1883,6 +1883,14 @@ do_start_initialization ()
 #if GDB_SELF_TEST
 namespace selftests {
 
+extern void (*hook_set_active_ext_lang) (void);
+
+static void
+raise_sigint (void)
+{
+  raise (SIGINT);
+}
+
 /* Entry point for python unit tests.  */
 
 static void
@@ -1897,21 +1905,45 @@ test_python ()
   output.clear ();
 
   bool saw_exception = false;
-  scoped_restore reset_gdb_python_initialized
-    = make_scoped_restore (&gdb_python_initialized, 0);
-  try
-    {
-      CMD (output);
-    }
-  catch (const gdb_exception &e)
-    {
-      saw_exception = true;
-      SELF_CHECK (e.reason == RETURN_ERROR);
-      SELF_CHECK (e.error == GENERIC_ERROR);
-      SELF_CHECK (*e.message == "Python not initialized");
-    }
-  SELF_CHECK (saw_exception);
-  SELF_CHECK (output.empty ());
+  {
+    scoped_restore reset_gdb_python_initialized
+      = make_scoped_restore (&gdb_python_initialized, 0);
+    try
+      {
+	CMD (output);
+      }
+    catch (const gdb_exception &e)
+      {
+	saw_exception = true;
+	SELF_CHECK (e.reason == RETURN_ERROR);
+	SELF_CHECK (e.error == GENERIC_ERROR);
+	SELF_CHECK (*e.message == "Python not initialized");
+      }
+    SELF_CHECK (saw_exception);
+    SELF_CHECK (output.empty ());
+  }
+
+  saw_exception = false;
+  {
+    scoped_restore save_hook
+      = make_scoped_restore (&hook_set_active_ext_lang, raise_sigint);
+    try
+      {
+	CMD (output);
+      }
+    catch (const gdb_exception &e)
+      {
+	saw_exception = true;
+	SELF_CHECK (e.reason == RETURN_ERROR);
+	SELF_CHECK (e.error == GENERIC_ERROR);
+	SELF_CHECK (*e.message == "Error while executing Python code.");
+      }
+    SELF_CHECK (saw_exception);
+    std::string ref_output("Traceback (most recent call last):\n"
+			   "  File \"<string>\", line 1, in <module>\n"
+			   "KeyboardInterrupt\n");
+    SELF_CHECK (output == ref_output);
+  }
 
 #undef CMD
 }

Re: [PATCH][gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

[Tom de Vries]

On 9/11/21 2:02 PM, Tom de Vries wrote:
> +#if GDB_SELF_TEST
> +namespace selftests {
> +extern void (*hook_set_active_ext_lang) (void);
> +void (*hook_set_active_ext_lang) (void) = nullptr;
> +}
> +#endif
> +
>  /* Set the currently active extension language to NOW_ACTIVE.
>     The result is a pointer to a malloc'd block of memory to pass to
>     restore_active_ext_lang.
> @@ -708,6 +715,11 @@ install_gdb_sigint_handler (struct signal_handler *previous)
>  struct active_ext_lang_state *
>  set_active_ext_lang (const struct extension_language_defn *now_active)
>  {
> +#if GDB_SELF_TEST
> +  if (selftests::hook_set_active_ext_lang)
> +    selftests::hook_set_active_ext_lang ();
> +#endif

In particular, I'd like feedback on whether this is acceptable.

F.i., I could do instead the less intrusive (but also less flexible):
...
#if GDB_SELF_TEST
namespace selftests {
extern int hook_raise_at_entry_active_ext_lang;
int hook_raise_at_entry_active_ext_lang = 0;
#endif

struct active_ext_lang_state *
set_active_ext_lang (const struct extension_language_defn *now_active)
{
#if GDB_SELF_TEST
  if (selftests::hook_raise_at_entry_active_ext_lang)
    raise (hook_raise_at_entry_active_ext_lang);
#endif
...

The hook that is a function pointer is the most flexible, but possibly
less safe.  OTOH, it is in the selftest code, which we disable by
default in release branches, so by default this is not included in
releases.  OTOH, it's still possible to enable it in releases using
--enable-unit-tests. Perhaps this could be handled using a
DEVEL_GDB_SELF_TEST, which is disabled in release branches independent
of --enable-unit-tests?

Any comment appreciated.

Thanks,
- Tom

[PING][PATCH][gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

[Tom de Vries]

On 9/22/21 12:17 PM, Tom de Vries wrote:
> On 9/11/21 2:02 PM, Tom de Vries wrote:
>> +#if GDB_SELF_TEST
>> +namespace selftests {
>> +extern void (*hook_set_active_ext_lang) (void);
>> +void (*hook_set_active_ext_lang) (void) = nullptr;
>> +}
>> +#endif
>> +
>>  /* Set the currently active extension language to NOW_ACTIVE.
>>     The result is a pointer to a malloc'd block of memory to pass to
>>     restore_active_ext_lang.
>> @@ -708,6 +715,11 @@ install_gdb_sigint_handler (struct signal_handler *previous)
>>  struct active_ext_lang_state *
>>  set_active_ext_lang (const struct extension_language_defn *now_active)
>>  {
>> +#if GDB_SELF_TEST
>> +  if (selftests::hook_set_active_ext_lang)
>> +    selftests::hook_set_active_ext_lang ();
>> +#endif
> 
> In particular, I'd like feedback on whether this is acceptable.
> 
> F.i., I could do instead the less intrusive (but also less flexible):
> ...
> #if GDB_SELF_TEST
> namespace selftests {
> extern int hook_raise_at_entry_active_ext_lang;
> int hook_raise_at_entry_active_ext_lang = 0;
> #endif
> 
> struct active_ext_lang_state *
> set_active_ext_lang (const struct extension_language_defn *now_active)
> {
> #if GDB_SELF_TEST
>   if (selftests::hook_raise_at_entry_active_ext_lang)
>     raise (hook_raise_at_entry_active_ext_lang);
> #endif
> ...
> 
> The hook that is a function pointer is the most flexible, but possibly
> less safe.  OTOH, it is in the selftest code, which we disable by
> default in release branches, so by default this is not included in
> releases.  OTOH, it's still possible to enable it in releases using
> --enable-unit-tests. Perhaps this could be handled using a
> DEVEL_GDB_SELF_TEST, which is disabled in release branches independent
> of --enable-unit-tests?
> 
> Any comment appreciated.
> 

Ping.

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

[Tom Tromey]

Tom> [gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

Thanks for doing this.

Tom> +#if GDB_SELF_TEST
Tom> +namespace selftests {
Tom> +extern void (*hook_set_active_ext_lang) (void);
Tom> +void (*hook_set_active_ext_lang) (void) = nullptr;

Shouldn't need the "(void)" any more, just "()" should be ok.

It seems weird that both a declaration and definition are needed.
I guess it's a compiler issue though.

Tom>  #if GDB_SELF_TEST
Tom>  namespace selftests {
 
Tom> +extern void (*hook_set_active_ext_lang) (void);

I suppose it's hard to stick this declaration somewhere else.

Tom> +
Tom> +static void
Tom> +raise_sigint (void)

The (void) thing again, though this could also just be a tiny lambda in
the code as well.

Tom

Re: [PATCH][gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

>> +#if GDB_SELF_TEST
>> +  if (selftests::hook_set_active_ext_lang)
>> +    selftests::hook_set_active_ext_lang ();
>> +#endif

Tom> In particular, I'd like feedback on whether this is acceptable.

Tom> F.i., I could do instead the less intrusive (but also less flexible):

Either way is fine by me.  It's clearly code just for testing purposes,
so, for me at least, I don't mind if it's too flexible or whatever.

Tom> Perhaps this could be handled using a
Tom> DEVEL_GDB_SELF_TEST, which is disabled in release branches independent
Tom> of --enable-unit-tests?

I don't really understand what you meant by this.
Do you mean having some way to disable some subset of the self-tests?
If so, I think it's fine to just have them all around.

Tom

Re: [PATCH][gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

[Tom de Vries]

On 10/19/21 8:09 PM, Tom Tromey wrote:
> Tom> [gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest
> 
> Thanks for doing this.
> 
> Tom> +#if GDB_SELF_TEST
> Tom> +namespace selftests {
> Tom> +extern void (*hook_set_active_ext_lang) (void);
> Tom> +void (*hook_set_active_ext_lang) (void) = nullptr;
> 
> Shouldn't need the "(void)" any more, just "()" should be ok.
> 

Done.

> It seems weird that both a declaration and definition are needed.
> I guess it's a compiler issue though.
> 

Indeed, some warning, -Wmissing-declarations if I'm not mistaken.

> Tom>  #if GDB_SELF_TEST
> Tom>  namespace selftests {
>  
> Tom> +extern void (*hook_set_active_ext_lang) (void);
> 
> I suppose it's hard to stick this declaration somewhere else.
> 

I've moved it to gdb/extension.h.

I suppose I usually do this to avoid .h-triggered rebuilds while writing
the patch, and then forget to move it to a proper location.

> Tom> +
> Tom> +static void
> Tom> +raise_sigint (void)
> 
> The (void) thing again, though this could also just be a tiny lambda in
> the code as well.

Done, and committed.

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as unittest

[Tom de Vries]

On 10/19/21 8:10 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>>> +#if GDB_SELF_TEST
>>> +  if (selftests::hook_set_active_ext_lang)
>>> +    selftests::hook_set_active_ext_lang ();
>>> +#endif
> 
> Tom> In particular, I'd like feedback on whether this is acceptable.
> 
> Tom> F.i., I could do instead the less intrusive (but also less flexible):
> 
> Either way is fine by me.  It's clearly code just for testing purposes,
> so, for me at least, I don't mind if it's too flexible or whatever.
> 

Ack, I've left it as is.

> Tom> Perhaps this could be handled using a
> Tom> DEVEL_GDB_SELF_TEST, which is disabled in release branches independent
> Tom> of --enable-unit-tests?
> 
> I don't really understand what you meant by this.
> Do you mean having some way to disable some subset of the self-tests?

I mean having a class of unit tests that is not enabled in release
branches, even with --enable-unit-tests, because they're considered too
risky.

> If so, I think it's fine to just have them all around.
> 

Ack, thanks for confirming.

- Tom