[python] [patch] PR 13369/13374

[python] [patch] PR 13369/13374

[Phil Muldoon]

This patch address two different bugs.  There are some places where we
acquire the GIL where it is not needed.  Also, in a similar vein we do
not properly handle some GDB exceptions.

OK?

Cheers,

Phil

--

2011-11-21  Phil Muldoon  <pmuldoon@redhat.com>

        PR python/13369
        PR python 13374

	* python/python.c (gdbpy_decode_line): Do not acquire GIL.
	* python/py-inferior.c (inferior_to_inferior_object): Ditto.
	* python/py-value.c (valpy_nonzero): Use TRY_CATCH to catch GDB
	exceptions.
	* python/py-type.c (typy_strip_typedefs): Ditto.
	(typy_legacy_template_argument): Ditto.
	* python/py-inferior.c (inferior_to_inferior_object): Ditto.
	* python/py-breakpoint.c (bppy_set_ignore_count): Ditto.

--

diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
index 3f4467a..6c6df2e 100644
--- a/gdb/python/py-block.c
+++ b/gdb/python/py-block.c
@@ -371,14 +371,20 @@ gdbpy_block_for_pc (PyObject *self, PyObject *args)
 {
   gdb_py_ulongest pc;
   struct block *block;
-  struct obj_section *section;
-  struct symtab *symtab;
+  struct obj_section *section = NULL;
+  struct symtab *symtab = NULL;
+  volatile struct gdb_exception except;
 
   if (!PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &pc))
     return NULL;
 
-  section = find_pc_mapped_section (pc);
-  symtab = find_pc_sect_symtab (pc, section);
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      section = find_pc_mapped_section (pc);
+      symtab = find_pc_sect_symtab (pc, section);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+
   if (!symtab || symtab->objfile == NULL)
     {
       PyErr_SetString (PyExc_RuntimeError,
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index f235bbc..11d60fe 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -334,6 +334,7 @@ bppy_set_ignore_count (PyObject *self, PyObject *newvalue, void *closure)
 {
   breakpoint_object *self_bp = (breakpoint_object *) self;
   long value;
+  volatile struct gdb_exception except;
 
   BPPY_SET_REQUIRE_VALID (self_bp);
 
@@ -355,7 +356,12 @@ bppy_set_ignore_count (PyObject *self, PyObject *newvalue, void *closure)
 
   if (value < 0)
     value = 0;
-  set_ignore_count (self_bp->number, (int) value, 0);
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      set_ignore_count (self_bp->number, (int) value, 0);
+    }
+  GDB_PY_SET_HANDLE_EXCEPTION (except);
 
   return 0;
 }
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index d6086dc..be31f5d 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -157,15 +157,9 @@ inferior_to_inferior_object (struct inferior *inferior)
   inf_obj = inferior_data (inferior, infpy_inf_data_key);
   if (!inf_obj)
     {
-      struct cleanup *cleanup;
-      cleanup = ensure_python_env (python_gdbarch, python_language);
-
       inf_obj = PyObject_New (inferior_object, &inferior_object_type);
       if (!inf_obj)
-	{
-	  do_cleanups (cleanup);
 	  return NULL;
-	}
 
       inf_obj->inferior = inferior;
       inf_obj->threads = NULL;
@@ -173,7 +167,6 @@ inferior_to_inferior_object (struct inferior *inferior)
 
       set_inferior_data (inferior, infpy_inf_data_key, inf_obj);
 
-      do_cleanups (cleanup);
     }
   else
     Py_INCREF ((PyObject *)inf_obj);
diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index f76b1c7..b671cef 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -395,6 +395,13 @@ static PyObject *
 typy_strip_typedefs (PyObject *self, PyObject *args)
 {
   struct type *type = ((type_object *) self)->type;
+  volatile struct gdb_exception except;
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      type = check_typedef (type);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
 
   return type_to_type_object (check_typedef (type));
 }
@@ -768,10 +775,11 @@ typy_legacy_template_argument (struct type *type, const struct block *block,
 {
   int i;
   struct demangle_component *demangled;
-  struct demangle_parse_info *info;
+  struct demangle_parse_info *info = NULL;
   const char *err;
   struct type *argtype;
   struct cleanup *cleanup;
+  volatile struct gdb_exception except;
 
   if (TYPE_NAME (type) == NULL)
     {
@@ -779,8 +787,13 @@ typy_legacy_template_argument (struct type *type, const struct block *block,
       return NULL;
     }
 
-  /* Note -- this is not thread-safe.  */
-  info = cp_demangled_name_to_comp (TYPE_NAME (type), &err);
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      /* Note -- this is not thread-safe.  */
+      info = cp_demangled_name_to_comp (TYPE_NAME (type), &err);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+
   if (! info)
     {
       PyErr_SetString (PyExc_RuntimeError, err);
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 15c183a..05e592f 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -880,10 +880,10 @@ valpy_nonzero (PyObject *self)
   struct type *type;
   int nonzero = 0; /* Appease GCC warning.  */
 
-  type = check_typedef (value_type (self_value->value));
-
   TRY_CATCH (except, RETURN_MASK_ALL)
     {
+      type = check_typedef (value_type (self_value->value));
+
       if (is_integral_type (type) || TYPE_CODE (type) == TYPE_CODE_PTR)
 	nonzero = !!value_as_long (self_value->value);
       else if (TYPE_CODE (type) == TYPE_CODE_FLT)
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 108e542..6022572 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -504,7 +504,7 @@ gdbpy_decode_line (PyObject *self, PyObject *args)
   if (! PyArg_ParseTuple (args, "|s", &arg))
     return NULL;
 
-  cleanups = ensure_python_env (get_current_arch (), current_language);
+  cleanups = make_cleanup (null_cleanup, NULL);
 
   TRY_CATCH (except, RETURN_MASK_ALL)
     {

RE: [python] [patch] PR 13369/13374

[Paul_Koning]

>...
>diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
>index f76b1c7..b671cef 100644
>--- a/gdb/python/py-type.c
>+++ b/gdb/python/py-type.c
>...
>@@ -779,8 +787,13 @@ typy_legacy_template_argument (struct type *type, const struct block >*block,
>       return NULL;
>     }
> 
>-  /* Note -- this is not thread-safe.  */
>-  info = cp_demangled_name_to_comp (TYPE_NAME (type), &err);
>+  TRY_CATCH (except, RETURN_MASK_ALL)
>+    {
>+      /* Note -- this is not thread-safe.  */

Does the comment "not thread safe" indicate that this is a spot where the GIL should be acquired and isn't currently?  Or doesn't the GIL help for that?

	paul

Re: [python] [patch] PR 13369/13374

[Tom Tromey]

>>>>> "Paul" ==   <Paul_Koning@Dell.com> writes:

>> +      /* Note -- this is not thread-safe.  */

Paul> Does the comment "not thread safe" indicate that this is a spot where
Paul> the GIL should be acquired and isn't currently?  Or doesn't the GIL
Paul> help for that?

The comment refers to the code being called here.
I think the comment can be removed.  It isn't ok to call gdb stuff from
other Python threads anyway.

Tom

Re: [python] [patch] PR 13369/13374

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> @@ -157,15 +157,9 @@ inferior_to_inferior_object (struct inferior *inferior)
Phil>    inf_obj = inferior_data (inferior, infpy_inf_data_key);
Phil>    if (!inf_obj)
Phil>      {
Phil> -      struct cleanup *cleanup;
Phil> -      cleanup = ensure_python_env (python_gdbarch, python_language);
Phil> -
Phil>        inf_obj = PyObject_New (inferior_object, &inferior_object_type);
Phil>        if (!inf_obj)
Phil> -	{
Phil> -	  do_cleanups (cleanup);
Phil>  	  return NULL;
Phil> -	}

This doesn't handle the call chain

delete_thread_object -> find_inferior_object -> inferior_to_inferior_object

Tom

Re: [python] [patch] PR 13369/13374

[Phil Muldoon]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Phil> @@ -157,15 +157,9 @@ inferior_to_inferior_object (struct inferior *inferior)
> Phil>    inf_obj = inferior_data (inferior, infpy_inf_data_key);
> Phil>    if (!inf_obj)
> Phil>      {
> Phil> -      struct cleanup *cleanup;
> Phil> -      cleanup = ensure_python_env (python_gdbarch, python_language);
> Phil> -
> Phil>        inf_obj = PyObject_New (inferior_object, &inferior_object_type);
> Phil>        if (!inf_obj)
> Phil> -	{
> Phil> -	  do_cleanups (cleanup);
> Phil>  	  return NULL;
> Phil> -	}
>
> This doesn't handle the call chain
>
> delete_thread_object -> find_inferior_object -> inferior_to_inferior_object

I see what I forgot.  I forgot to move the late GIL acquisition up too a
little earlier in the function body.  Thanks for spotting!

Cheers,

Phil

--

diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
index 3f4467a..6c6df2e 100644
--- a/gdb/python/py-block.c
+++ b/gdb/python/py-block.c
@@ -371,14 +371,20 @@ gdbpy_block_for_pc (PyObject *self, PyObject *args)
 {
   gdb_py_ulongest pc;
   struct block *block;
-  struct obj_section *section;
-  struct symtab *symtab;
+  struct obj_section *section = NULL;
+  struct symtab *symtab = NULL;
+  volatile struct gdb_exception except;
 
   if (!PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &pc))
     return NULL;
 
-  section = find_pc_mapped_section (pc);
-  symtab = find_pc_sect_symtab (pc, section);
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      section = find_pc_mapped_section (pc);
+      symtab = find_pc_sect_symtab (pc, section);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+
   if (!symtab || symtab->objfile == NULL)
     {
       PyErr_SetString (PyExc_RuntimeError,
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index f235bbc..11d60fe 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -334,6 +334,7 @@ bppy_set_ignore_count (PyObject *self, PyObject *newvalue, void *closure)
 {
   breakpoint_object *self_bp = (breakpoint_object *) self;
   long value;
+  volatile struct gdb_exception except;
 
   BPPY_SET_REQUIRE_VALID (self_bp);
 
@@ -355,7 +356,12 @@ bppy_set_ignore_count (PyObject *self, PyObject *newvalue, void *closure)
 
   if (value < 0)
     value = 0;
-  set_ignore_count (self_bp->number, (int) value, 0);
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      set_ignore_count (self_bp->number, (int) value, 0);
+    }
+  GDB_PY_SET_HANDLE_EXCEPTION (except);
 
   return 0;
 }
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index d6086dc..7b2981e 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -157,15 +157,9 @@ inferior_to_inferior_object (struct inferior *inferior)
   inf_obj = inferior_data (inferior, infpy_inf_data_key);
   if (!inf_obj)
     {
-      struct cleanup *cleanup;
-      cleanup = ensure_python_env (python_gdbarch, python_language);
-
       inf_obj = PyObject_New (inferior_object, &inferior_object_type);
       if (!inf_obj)
-	{
-	  do_cleanups (cleanup);
 	  return NULL;
-	}
 
       inf_obj->inferior = inferior;
       inf_obj->threads = NULL;
@@ -173,7 +167,6 @@ inferior_to_inferior_object (struct inferior *inferior)
 
       set_inferior_data (inferior, infpy_inf_data_key, inf_obj);
 
-      do_cleanups (cleanup);
     }
   else
     Py_INCREF ((PyObject *)inf_obj);
@@ -266,10 +259,15 @@ delete_thread_object (struct thread_info *tp, int ignore)
   inferior_object *inf_obj;
   thread_object *thread_obj;
   struct threadlist_entry **entry, *tmp;
+  
+  cleanup = ensure_python_env (python_gdbarch, python_language);
 
   inf_obj = (inferior_object *) find_inferior_object (PIDGET(tp->ptid));
   if (!inf_obj)
-    return;
+    {
+      do_cleanups (cleanup);
+      return;
+    }
 
   /* Find thread entry in its inferior's thread_list.  */
   for (entry = &inf_obj->threads; *entry != NULL; entry =
@@ -280,11 +278,10 @@ delete_thread_object (struct thread_info *tp, int ignore)
   if (!*entry)
     {
       Py_DECREF (inf_obj);
+      do_cleanups (cleanup);
       return;
     }
 
-  cleanup = ensure_python_env (python_gdbarch, python_language);
-
   tmp = *entry;
   tmp->thread_obj->thread = NULL;
 
diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index f76b1c7..b671cef 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -395,6 +395,13 @@ static PyObject *
 typy_strip_typedefs (PyObject *self, PyObject *args)
 {
   struct type *type = ((type_object *) self)->type;
+  volatile struct gdb_exception except;
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      type = check_typedef (type);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
 
   return type_to_type_object (check_typedef (type));
 }
@@ -768,10 +775,11 @@ typy_legacy_template_argument (struct type *type, const struct block *block,
 {
   int i;
   struct demangle_component *demangled;
-  struct demangle_parse_info *info;
+  struct demangle_parse_info *info = NULL;
   const char *err;
   struct type *argtype;
   struct cleanup *cleanup;
+  volatile struct gdb_exception except;
 
   if (TYPE_NAME (type) == NULL)
     {
@@ -779,8 +787,13 @@ typy_legacy_template_argument (struct type *type, const struct block *block,
       return NULL;
     }
 
-  /* Note -- this is not thread-safe.  */
-  info = cp_demangled_name_to_comp (TYPE_NAME (type), &err);
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      /* Note -- this is not thread-safe.  */
+      info = cp_demangled_name_to_comp (TYPE_NAME (type), &err);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+
   if (! info)
     {
       PyErr_SetString (PyExc_RuntimeError, err);
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 15c183a..05e592f 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -880,10 +880,10 @@ valpy_nonzero (PyObject *self)
   struct type *type;
   int nonzero = 0; /* Appease GCC warning.  */
 
-  type = check_typedef (value_type (self_value->value));
-
   TRY_CATCH (except, RETURN_MASK_ALL)
     {
+      type = check_typedef (value_type (self_value->value));
+
       if (is_integral_type (type) || TYPE_CODE (type) == TYPE_CODE_PTR)
 	nonzero = !!value_as_long (self_value->value);
       else if (TYPE_CODE (type) == TYPE_CODE_FLT)
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 108e542..6022572 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -504,7 +504,7 @@ gdbpy_decode_line (PyObject *self, PyObject *args)
   if (! PyArg_ParseTuple (args, "|s", &arg))
     return NULL;
 
-  cleanups = ensure_python_env (get_current_arch (), current_language);
+  cleanups = make_cleanup (null_cleanup, NULL);
 
   TRY_CATCH (except, RETURN_MASK_ALL)
     {

Re: [python] [patch] PR 13369/13374

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> I see what I forgot.  I forgot to move the late GIL acquisition up too a
Phil> little earlier in the function body.  Thanks for spotting!

This is ok.

Tom

Re: [python] [patch] PR 13369/13374

[Phil Muldoon]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Phil> I see what I forgot.  I forgot to move the late GIL acquisition up too a
> Phil> little earlier in the function body.  Thanks for spotting!
>
> This is ok.

So committed, thanks.