Re: [PATCH] gdb/breakpoint: remove assertion when printing internal breakpoints without locspecs

["Sébastien Darche" <sdarche@efficios.com>]

Hi Andrew.

Thanks for the review. I just sent an updated patch to the mailing list 
with your comments.

The test passes with some remote target boards. I however noticed that 
some of them do not compile due to the missing header "jit-protocol.h" 
that is common to all the jit tests, as it does not get uploaded. All 
the jit tests that rely on this header show as unsupported instead of 
failed. Maybe this is something we might want to look into in the future.

Best,
Sébastien

On 2026-05-29 11:45, Andrew Burgess wrote:
> 
> Hi Sébastien,
> 
> Thanks for looking at this.  I think this looks good, I have a couple of
> minor nits to feedback, see inline below.
> 
> Sébastien Darche <sdarche@efficios.com> writes:
> 
>> When debugging gdb with `set debug breakpoint on`, gdb would crash if
>> the inferior happened to unload (through dlclose ()) a shared library
>> that contains jit debugging symbols:
>>
>> 	../../gdb/breakpoint.c:6473: internal-error: print_breakpoint_location:
>> 	Assertion `b->locspec != nullptr || (!user_breakpoint_p (b) && (b->type
>> 	== bp_shlib_event || b->type == bp_thread_event))' failed.
>> 	A problem internal to GDB has been detected [...]
>>
>> This assertion was added in commit 5770f68 ("gdb: handle empty locspec
>> when printing breakpoints"). The assumption for this assert is that the
>> user may not call any (gdb-)debugging command between the time a shared
>> library is marked (bp_location->shlib_disabled is set) and the
>> breakpoint is actually removed. This would be true if the user called
>> `maint info breakpoints` as explained in the original commit message.
>>
>> However, if `set debug breakpoint on` is called, gdb prints debugging
>> info as the shared library is being unloaded. The ~jiter_objfile_data
>> dtor deletes the breakpoint, which calls remove_breakpoint_1 on the
>> bp_location in the recently unloaded shared object. Since `debug
>> breakpoint` is enabled, we go through print_breakpoint_location with the
>> bp_location's shlib_disabled flag set.. which results in the assert
>> above.
> 
> I think the ".. " should be replace with ", ".
> 
>>
>> This commit removes the assertion as it is not true in all cases. I've
>> also included a minimal test case that loads a shared library with a
>> __jit_debug_register_code symbol. The test fails with the assert.
> 
> 
>>
>> Change-Id: Ia19c84f194a6f0c10315548c7f423bfd86ef0266
>> ---
>>   gdb/breakpoint.c                          | 17 ---------
>>   gdb/testsuite/gdb.base/jit-unload-solib.c | 21 +++++++++++
>>   gdb/testsuite/gdb.base/jit-unload.c       | 45 +++++++++++++++++++++++
>>   gdb/testsuite/gdb.base/jit-unload.exp     | 44 ++++++++++++++++++++++
>>   4 files changed, 110 insertions(+), 17 deletions(-)
>>   create mode 100644 gdb/testsuite/gdb.base/jit-unload-solib.c
>>   create mode 100644 gdb/testsuite/gdb.base/jit-unload.c
>>   create mode 100644 gdb/testsuite/gdb.base/jit-unload.exp
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 101dc57ee6b..85c4ffc3171 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -6457,23 +6457,6 @@ print_breakpoint_location (const breakpoint *b, const bp_location *loc)
>>       }
>>     else
>>       {
>> -      /* Internal breakpoints don't have a locspec string, but can become
>> -	 pending if the shared library the breakpoint is in is unloaded.
>> -	 For most internal breakpoint types though, after unloading the
>> -	 shared library, the breakpoint will be deleted and never recreated
>> -	 (see internal_breakpoint::re_set).  But for two internal
>> -	 breakpoint types bp_shlib_event and bp_thread_event this is not
>> -	 true.  Usually we don't expect the libraries that contain these
>> -	 breakpoints to ever be unloaded, but a buggy inferior might do
>> -	 such a thing, in which case GDB should be prepared to handle this
>> -	 case.
>> -
>> -	 If these two breakpoint types become pending then there will be no
>> -	 locspec string.  */
>> -      gdb_assert (b->locspec != nullptr
>> -		  || (!user_breakpoint_p (b)
>> -		      && (b->type == bp_shlib_event
>> -			  || b->type == bp_thread_event)));
>>         const char *locspec_str
>>   	= (b->locspec != nullptr ? b->locspec->to_string () : "");
>>         uiout->field_string ("pending", locspec_str);
>> diff --git a/gdb/testsuite/gdb.base/jit-unload-solib.c b/gdb/testsuite/gdb.base/jit-unload-solib.c
>> new file mode 100644
>> index 00000000000..3cf2c580320
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/jit-unload-solib.c
>> @@ -0,0 +1,21 @@
>> +/* This test program is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2026 Free Software Foundation, Inc.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +/* This simulates a JIT library.  */
>> +
>> +
>> +#include "jit-protocol.h"
>> diff --git a/gdb/testsuite/gdb.base/jit-unload.c b/gdb/testsuite/gdb.base/jit-unload.c
>> new file mode 100644
>> index 00000000000..def66b53861
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/jit-unload.c
>> @@ -0,0 +1,45 @@
>> +/* This test program is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2026 Free Software Foundation, Inc.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +/* Simulate loading of a library JIT code.  */
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +
>> +#ifdef __WIN32__
>> +#include <windows.h>
>> +#define dlopen(name, mode) LoadLibrary (TEXT (name))
>> +#define dlclose(handle) FreeLibrary (handle)
>> +#else
>> +#include <dlfcn.h>
>> +#endif
>> +
>> +
>> +int
>> +main()
> 
> Missing space after 'main'.  Also, for C code I think 'main (void)' is
> what we should be writing, though we do have a lot of 'main ()' already.
> 
>> +{
>> +  void *handle = dlopen (SHLIB_NAME, RTLD_NOW);
>> +
>> +  if (handle == nullptr)
> 
> The 'nullptr' is a C23 feature.  To allow this test to compile with
> older compilers it would be better to use 'NULL'.
> 
>> +    {
>> +      fprintf (stderr, "%s\n", dlerror ());
>> +      exit (1);
>> +    }
>> +
>> +  dlclose (handle);
>> +  return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.base/jit-unload.exp b/gdb/testsuite/gdb.base/jit-unload.exp
>> new file mode 100644
>> index 00000000000..0bf9c59385d
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/jit-unload.exp
>> @@ -0,0 +1,44 @@
>> +# Copyright 2026 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# This test checks that gdb does not assert when unloading a shared library
>> +# that defines a __jit_debug_register_code if `set debug breakpoint on` is set.
>> +
>> +require allow_shlib_tests
>> +
>> +standard_testfile .c -solib.c
>> +set libfile ${testfile}-lib
>> +
>> +set options [list \
>> +		 debug \
>> +		 shlib_load \
>> +		 additional_flags=-DSHLIB_NAME=\"${libfile}\"]
>> +
>> +if { [build_executable "build main file" $testfile $srcfile $options] == -1 } {
>> +    return
>> +}
>> +
>> +if { [build_executable "build shlib" $libfile $srcfile2 {debug shlib}] == -1 } {
>> +    return
>> +}
> 
> This isn't going to work for remote target boards as the shlib will
> still be on the build machine.  If you rewrite like this to make use of
> gdb_download_shlib then things will be better.  I tested this using
> 'make check-all-boards', which is described in gdb/testsuite/README:
> 
>    require allow_shlib_tests
>    
>    standard_testfile .c -solib.c
>    
>    set lib_testfile ${testfile}-lib
>    set lib_binfile [standard_output_file $lib_testfile]
>    
>    if { [build_executable "build shlib" $lib_testfile $srcfile2 \
>    	  {debug shlib}] == -1 } {
>        return
>    }
>    
>    set lib_testfile_on_target [gdb_download_shlib $lib_binfile]
>    
>    set options [list \
>    		 debug \
>    		 shlib_load \
>    		 additional_flags=-DSHLIB_NAME=\"${lib_testfile_on_target}\"]
>    
>    if { [prepare_for_testing "build main file" $testfile $srcfile \
>    	  $options] == -1 } {
>        return
>    }
> 
>> +
>> +clean_restart $::testfile
> 
> I propose using prepare_for_testing above, so this line will no longer
> be needed.
> 
>> +
>> +if { ![runto_main] } {
>> +    return
>> +}
>> +
>> +gdb_test_no_output "set debug breakpoint on"
>> +gdb_continue_to_end "unload" continue 1
> 
> I worry about trying to handle all of the possible debug breakpoint
> output using gdb_continue_to_end, which matches all the output with a
> ".*" pattern; there's a risk of buffer overflow in the future.
> 
> I'd prefer to see this handled with something like:
> 
>    set saw_exit false
>    gdb_test_multiple "continue" unload {
>        -re "^\\\[Inferior $decimal \[^\r\n\]+ exited normally\\\]\r\n" {
>    	set saw_exit true
>    	exp_continue
>        }
>    
>        -re "^$gdb_prompt $" {
>    	gdb_assert ${saw_exit} $gdb_test_name
>        }
>    
>        -re "^\[^\r\n\]*\r\n" {
>    	exp_continue
>        }
>    }
> 
> which handles the output line by line, and so will be happy with any
> volume of debug output.
> 
> Thanks,
> Andrew
> 
>