diff mbox

fix build failure with Python 3.7

Message ID 96198491-96D8-42F0-9956-1C2BC9277050@dell.com
State New, archived
Headers

Commit Message

Koning, Paul May 31, 2018, 5:12 p.m. UTC 
  This cures PR gdb/33470, build failure due to calling an internal 
API in Python that changed in V3.7.  The code now uses the
"inittab" mechanism in Python, which is the approved way to
make compiled-in modules known to Python.

Gdb starts properly and I can see the _gdb module.  On my test
system (a Mac) it's hard to get gdb to run at all, so the
test suite is a bit problematic.  What other tests are needed?

	paul

gdb/ChangeLog:

2018-05-31  Paul Koning  <paul_koning@dell.com>

	PR gdb/33470

	* python/python.c (do_start_initialization):
	Avoid call to internal Python API.
	(PyInit__gdb): New function.

Comments

Sergio Durigan Junior May 31, 2018, 8:10 p.m. UTC | #1 
On Thursday, May 31 2018, Paul Koning wrote:

> This cures PR gdb/33470, build failure due to calling an internal 
> API in Python that changed in V3.7.  The code now uses the
> "inittab" mechanism in Python, which is the approved way to
> make compiled-in modules known to Python.
>
> Gdb starts properly and I can see the _gdb module.  On my test
> system (a Mac) it's hard to get gdb to run at all, so the
> test suite is a bit problematic.  What other tests are needed?

Thanks for the patch, Paul.  I'll run this on BuildBot and see what
comes out of it.  Meanwhile...

> 	paul
>
> gdb/ChangeLog:
>
> 2018-05-31  Paul Koning  <paul_koning@dell.com>
>
> 	PR gdb/33470

This bug number is actually from Python's bugzilla, not GDB's.  So it's
not correct to mention it here in the ChangeLog/commit message.  AFAIK,
there's no correspondent GDB bug filed for this issue.

> 	* python/python.c (do_start_initialization):
> 	Avoid call to internal Python API.
> 	(PyInit__gdb): New function.
>
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index c29e7d7a6b..89443aee25 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -1667,6 +1667,14 @@ finalize_python (void *ignore)
>    restore_active_ext_lang (previous_active);
>  }
>  
> +#ifdef IS_PY3K
> +PyMODINIT_FUNC
> +PyInit__gdb (void)
> +{
> +  return PyModule_Create (&python_GdbModuleDef);
> +}
> +#endif

I think it's a good idea to add a comment to this function.

> +
>  static bool
>  do_start_initialization ()
>  {
> @@ -1707,6 +1715,9 @@ do_start_initialization ()
>       remain alive for the duration of the program's execution, so
>       it is not freed after this call.  */
>    Py_SetProgramName (progname_copy);
> +
> +  /* Define _gdb as a built-in module.  */
> +  PyImport_AppendInittab ("_gdb", PyInit__gdb);
>  #else
>    Py_SetProgramName (progname.release ());
>  #endif
> @@ -1716,9 +1727,7 @@ do_start_initialization ()
>    PyEval_InitThreads ();
>  
>  #ifdef IS_PY3K
> -  gdb_module = PyModule_Create (&python_GdbModuleDef);
> -  /* Add _gdb module to the list of known built-in modules.  */
> -  _PyImport_FixupBuiltin (gdb_module, "_gdb");
> +  gdb_module = PyImport_ImportModule ("_gdb");
>  #else
>    gdb_module = Py_InitModule ("_gdb", python_GdbMethods);
>  #endif
Koning, Paul May 31, 2018, 8:45 p.m. UTC | #2 
> On May 31, 2018, at 4:10 PM, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
> 
>> 
>> gdb/ChangeLog:
>> 
>> 2018-05-31  Paul Koning  <paul_koning@dell.com>
>> 
>> 	PR gdb/33470
> 
> This bug number is actually from Python's bugzilla, not GDB's.  So it's
> not correct to mention it here in the ChangeLog/commit message.  AFAIK,
> there's no correspondent GDB bug filed for this issue.

Ok, I removed that.

> 
>> 	* python/python.c (do_start_initialization):
>> 	Avoid call to internal Python API.
>> 	(PyInit__gdb): New function.
>> 
>> diff --git a/gdb/python/python.c b/gdb/python/python.c
>> index c29e7d7a6b..89443aee25 100644
>> --- a/gdb/python/python.c
>> +++ b/gdb/python/python.c
>> @@ -1667,6 +1667,14 @@ finalize_python (void *ignore)
>>   restore_active_ext_lang (previous_active);
>> }
>> 
>> +#ifdef IS_PY3K
>> +PyMODINIT_FUNC
>> +PyInit__gdb (void)
>> +{
>> +  return PyModule_Create (&python_GdbModuleDef);
>> +}
>> +#endif
> 
> I think it's a good idea to add a comment to this function.

I added this (after the #ifdef):

/* This is called via the PyImport_AppendInittab mechanism called
   during initialization, to make the built-in _gdb module known to
   Python.  */

	paul
Sergio Durigan Junior May 31, 2018, 9:20 p.m. UTC | #3 
On Thursday, May 31 2018, Paul Koning wrote:

>> On May 31, 2018, at 4:10 PM, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>> 
>>> 
>>> gdb/ChangeLog:
>>> 
>>> 2018-05-31  Paul Koning  <paul_koning@dell.com>
>>> 
>>> 	PR gdb/33470
>> 
>> This bug number is actually from Python's bugzilla, not GDB's.  So it's
>> not correct to mention it here in the ChangeLog/commit message.  AFAIK,
>> there's no correspondent GDB bug filed for this issue.
>
> Ok, I removed that.
>
>> 
>>> 	* python/python.c (do_start_initialization):
>>> 	Avoid call to internal Python API.
>>> 	(PyInit__gdb): New function.
>>> 
>>> diff --git a/gdb/python/python.c b/gdb/python/python.c
>>> index c29e7d7a6b..89443aee25 100644
>>> --- a/gdb/python/python.c
>>> +++ b/gdb/python/python.c
>>> @@ -1667,6 +1667,14 @@ finalize_python (void *ignore)
>>>   restore_active_ext_lang (previous_active);
>>> }
>>> 
>>> +#ifdef IS_PY3K
>>> +PyMODINIT_FUNC
>>> +PyInit__gdb (void)
>>> +{
>>> +  return PyModule_Create (&python_GdbModuleDef);
>>> +}
>>> +#endif
>> 
>> I think it's a good idea to add a comment to this function.
>
> I added this (after the #ifdef):
>
> /* This is called via the PyImport_AppendInittab mechanism called
>    during initialization, to make the built-in _gdb module known to
>    Python.  */

Thanks.

FWIW, I ran the full testsuite here and no regressions were found.
Tom Tromey June 1, 2018, 1:27 a.m. UTC | #4 
>>>>>   <Paul.Koning@dell.com> writes:

>> This bug number is actually from Python's bugzilla, not GDB's.  So it's
>> not correct to mention it here in the ChangeLog/commit message.  AFAIK,
>> there's no correspondent GDB bug filed for this issue.

> Ok, I removed that.

There may be other cases where we stick a URL of some non-gdb bug into
the ChangeLog (or at least the commit message), and that seems like it
would save some time for some future archaeologist, so I'd suggest doing
that.

Tom
Phil Muldoon June 1, 2018, 11:56 a.m. UTC | #5 
On 31/05/18 22:20, Sergio Durigan Junior wrote:
> On Thursday, May 31 2018, Paul Koning wrote:
> 
>>> On May 31, 2018, at 4:10 PM, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>>
>>>>
>>>> gdb/ChangeLog:
>>>>
>>>> 2018-05-31  Paul Koning  <paul_koning@dell.com>
>>>>
>>>> 	PR gdb/33470
>>>
>>> This bug number is actually from Python's bugzilla, not GDB's.  So it's
>>> not correct to mention it here in the ChangeLog/commit message.  AFAIK,
>>> there's no correspondent GDB bug filed for this issue.
>>
>> Ok, I removed that.
>>
>>>
>>>> 	* python/python.c (do_start_initialization):
>>>> 	Avoid call to internal Python API.
>>>> 	(PyInit__gdb): New function.
>>>>
>>>> diff --git a/gdb/python/python.c b/gdb/python/python.c
>>>> index c29e7d7a6b..89443aee25 100644
>>>> --- a/gdb/python/python.c
>>>> +++ b/gdb/python/python.c
>>>> @@ -1667,6 +1667,14 @@ finalize_python (void *ignore)
>>>>   restore_active_ext_lang (previous_active);
>>>> }
>>>>
>>>> +#ifdef IS_PY3K
>>>> +PyMODINIT_FUNC
>>>> +PyInit__gdb (void)
>>>> +{
>>>> +  return PyModule_Create (&python_GdbModuleDef);
>>>> +}
>>>> +#endif
>>>
>>> I think it's a good idea to add a comment to this function.
>>
>> I added this (after the #ifdef):
>>
>> /* This is called via the PyImport_AppendInittab mechanism called
>>    during initialization, to make the built-in _gdb module known to
>>    Python.  */
> 
> Thanks.
> 
> FWIW, I ran the full testsuite here and no regressions were found.
> 

Hey Sergio,

Thanks for doing this. Did you test it against a GDB built against 3.7
and a GDB build with a 2.x version of Python?

Cheers

Phil
Phil Muldoon June 1, 2018, 11:58 a.m. UTC | #6 
On 01/06/18 02:27, Tom Tromey wrote:
>>>>>>   <Paul.Koning@dell.com> writes:
> 
>>> This bug number is actually from Python's bugzilla, not GDB's.  So it's
>>> not correct to mention it here in the ChangeLog/commit message.  AFAIK,
>>> there's no correspondent GDB bug filed for this issue.
> 
>> Ok, I removed that.
> 
> There may be other cases where we stick a URL of some non-gdb bug into
> the ChangeLog (or at least the commit message), and that seems like it
> would save some time for some future archaeologist, so I'd suggest doing
> that.
> 
> Tom
> 

Hi all,

I think the best thing to do would be to create a GDB bug that then
has a reference to the Python bug tracking system.  There is a Red Hat
bugzilla entry for this, but from an upstream perspective, that's
neither here nor there.

Cheers,

Phil
Pedro Alves June 1, 2018, 12:15 p.m. UTC | #7 
On 05/31/2018 09:45 PM, Paul.Koning@dell.com wrote:

>>> @@ -1667,6 +1667,14 @@ finalize_python (void *ignore)
>>>   restore_active_ext_lang (previous_active);
>>> }
>>>
>>> +#ifdef IS_PY3K
>>> +PyMODINIT_FUNC
>>> +PyInit__gdb (void)
>>> +{
>>> +  return PyModule_Create (&python_GdbModuleDef);
>>> +}
>>> +#endif
>>
>> I think it's a good idea to add a comment to this function.
> 
> I added this (after the #ifdef):
> 
> /* This is called via the PyImport_AppendInittab mechanism called
>    during initialization, to make the built-in _gdb module known to
>    Python.  */

Can the function be made static?

I'm a little surprised to see the function being named "Py...", since
that kind of looks like stepping in Python's namespace.

Thanks,
Pedro Alves
Koning, Paul June 1, 2018, 12:54 p.m. UTC | #8 
> On Jun 1, 2018, at 8:15 AM, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/31/2018 09:45 PM, Paul.Koning@dell.com wrote:
> 
>>>> @@ -1667,6 +1667,14 @@ finalize_python (void *ignore)
>>>>  restore_active_ext_lang (previous_active);
>>>> }
>>>> 
>>>> +#ifdef IS_PY3K
>>>> +PyMODINIT_FUNC
>>>> +PyInit__gdb (void)
>>>> +{
>>>> +  return PyModule_Create (&python_GdbModuleDef);
>>>> +}
>>>> +#endif
>>> 
>>> I think it's a good idea to add a comment to this function.
>> 
>> I added this (after the #ifdef):
>> 
>> /* This is called via the PyImport_AppendInittab mechanism called
>>   during initialization, to make the built-in _gdb module known to
>>   Python.  */
> 
> Can the function be made static?

No; I did that first but PyMODINIT_FUNC is a #define that conflicts with "static".

> I'm a little surprised to see the function being named "Py...", since
> that kind of looks like stepping in Python's namespace.

True.  How about "init__gdb_module"?

	paul
Pedro Alves June 1, 2018, 1:10 p.m. UTC | #9 
On 06/01/2018 01:54 PM, Paul.Koning@dell.com wrote:

>> Can the function be made static?
> 
> No; I did that first but PyMODINIT_FUNC is a #define that conflicts with "static".

I see.

>> I'm a little surprised to see the function being named "Py...", since
>> that kind of looks like stepping in Python's namespace.
> 
> True.  How about "init__gdb_module"?

Sounds fine to me.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/python/python.c b/gdb/python/python.c
index c29e7d7a6b..89443aee25 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1667,6 +1667,14 @@  finalize_python (void *ignore)
   restore_active_ext_lang (previous_active);
 }
 
+#ifdef IS_PY3K
+PyMODINIT_FUNC
+PyInit__gdb (void)
+{
+  return PyModule_Create (&python_GdbModuleDef);
+}
+#endif
+
 static bool
 do_start_initialization ()
 {
@@ -1707,6 +1715,9 @@  do_start_initialization ()
      remain alive for the duration of the program's execution, so
      it is not freed after this call.  */
   Py_SetProgramName (progname_copy);
+
+  /* Define _gdb as a built-in module.  */
+  PyImport_AppendInittab ("_gdb", PyInit__gdb);
 #else
   Py_SetProgramName (progname.release ());
 #endif
@@ -1716,9 +1727,7 @@  do_start_initialization ()
   PyEval_InitThreads ();
 
 #ifdef IS_PY3K
-  gdb_module = PyModule_Create (&python_GdbModuleDef);
-  /* Add _gdb module to the list of known built-in modules.  */
-  _PyImport_FixupBuiltin (gdb_module, "_gdb");
+  gdb_module = PyImport_ImportModule ("_gdb");
 #else
   gdb_module = Py_InitModule ("_gdb", python_GdbMethods);
 #endif