diff mbox series

Check gdb_python_module in gdbpy_handle_missing_debuginfo

Message ID 20231115135204.3747216-1-tromey@adacore.com
State New
Headers
Series Check gdb_python_module in gdbpy_handle_missing_debuginfo |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 warning Patch is already merged
linaro-tcwg-bot/tcwg_gdb_build--master-arm warning Patch is already merged

Commit Message

Tom Tromey Nov. 15, 2023, 1:52 p.m. UTC 
  If you run gdb in the build tree without --data-directory, on a
program that does not have debug info, it will crash, because
gdbpy_handle_missing_debuginfo unconditionally uses gdb_python_module.

Other code in gdb using gdb_python_module checks it first.  I'm not
really sure why gdb_python_initialized does not cover this case (maybe
so that python can be partially initialized and still somewhat work?),
but it seemes harmless to do the same thing here.
---
 gdb/python/python.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

John Baldwin Nov. 15, 2023, 3:47 p.m. UTC | #1 
On 11/15/23 5:52 AM, Tom Tromey wrote:
> If you run gdb in the build tree without --data-directory, on a
> program that does not have debug info, it will crash, because
> gdbpy_handle_missing_debuginfo unconditionally uses gdb_python_module.
> 
> Other code in gdb using gdb_python_module checks it first.  I'm not
> really sure why gdb_python_initialized does not cover this case (maybe
> so that python can be partially initialized and still somewhat work?),
> but it seemes harmless to do the same thing here.
> ---
>   gdb/python/python.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 4523e30ed24..7e48165db21 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -1703,7 +1703,7 @@ gdbpy_handle_missing_debuginfo (const struct extension_language_defn *extlang,
>   				struct objfile *objfile)
>   {
>     /* Early exit if Python is not initialised.  */
> -  if (!gdb_python_initialized)
> +  if (!gdb_python_initialized || gdb_python_module == nullptr)
>       return {};
>   
>     struct gdbarch *gdbarch = objfile->arch ();

Looks good to me.  I suspect you can only check gdb_python_module == nullptr
here and assume that if it is non-null, gdb_python_initialized is also non-zero
as we set gdb_python_module in do_initialize() IFF do_start_initialize()
(which sets gdb_python_initialized) succeeds.

The error in do_initialize when the gdb module fails to load does seem to
explain why this condition exists and lines up with what you guess in the
commit log:

   gdb_python_module = PyImport_ImportModule ("gdb");
   if (gdb_python_module == NULL)
     {
       gdbpy_print_stack ();
       /* This is passed in one call to warning so that blank lines aren't
	 inserted between each line of text.  */
       warning (_("\n"
		 "Could not load the Python gdb module from `%s'.\n"
		 "Limited Python support is available from the _gdb module.\n"
		 "Suggest passing --data-directory=/path/to/gdb/data-directory."),
	       gdb_pythondir.c_str ());
       /* We return "success" here as we've already emitted the
	 warning.  */
       return true;
     }
Tom Tromey Nov. 15, 2023, 4:38 p.m. UTC | #2 
>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> Looks good to me.  I suspect you can only check gdb_python_module == nullptr
John> here and assume that if it is non-null, gdb_python_initialized is also non-zero
John> as we set gdb_python_module in do_initialize() IFF do_start_initialize()
John> (which sets gdb_python_initialized) succeeds.

Yeah.  I think you're right, but other spots also check
gdb_python_initialized, so I think we might as well leave it as-is.

John> The error in do_initialize when the gdb module fails to load does seem to
John> explain why this condition exists and lines up with what you guess in the
John> commit log:

I'll update the commit message to reflect this.

Tom
Andrew Burgess Nov. 16, 2023, 10:18 a.m. UTC | #3 
Tom Tromey <tromey@adacore.com> writes:

> If you run gdb in the build tree without --data-directory, on a
> program that does not have debug info, it will crash, because
> gdbpy_handle_missing_debuginfo unconditionally uses gdb_python_module.
>
> Other code in gdb using gdb_python_module checks it first.  I'm not
> really sure why gdb_python_initialized does not cover this case (maybe
> so that python can be partially initialized and still somewhat work?),
> but it seemes harmless to do the same thing here.

Gah.  I was dropping the ball all over the place with those patches.

Thanks for fixing this (with whatever updated commit message you use).

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew



> ---
>  gdb/python/python.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 4523e30ed24..7e48165db21 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -1703,7 +1703,7 @@ gdbpy_handle_missing_debuginfo (const struct extension_language_defn *extlang,
>  				struct objfile *objfile)
>  {
>    /* Early exit if Python is not initialised.  */
> -  if (!gdb_python_initialized)
> +  if (!gdb_python_initialized || gdb_python_module == nullptr)
>      return {};
>  
>    struct gdbarch *gdbarch = objfile->arch ();
> -- 
> 2.41.0
Tom Tromey Nov. 16, 2023, 3:14 p.m. UTC | #4 
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Gah.  I was dropping the ball all over the place with those patches.

Don't worry about it, and especially don't feel bad about this case,
which is a weird corner case that only affects gdb developers and people
who messed up their gdb install.

Tom
diff mbox series

Patch

diff --git a/gdb/python/python.c b/gdb/python/python.c
index 4523e30ed24..7e48165db21 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1703,7 +1703,7 @@  gdbpy_handle_missing_debuginfo (const struct extension_language_defn *extlang,
 				struct objfile *objfile)
 {
   /* Early exit if Python is not initialised.  */
-  if (!gdb_python_initialized)
+  if (!gdb_python_initialized || gdb_python_module == nullptr)
     return {};
 
   struct gdbarch *gdbarch = objfile->arch ();