Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file

[Pedro Alves <pedro@palves.net>]

Hi!

On 2023-02-10 7:22 p.m., Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
> 
>> On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote:
>>
>>>  breakpoint::print_recreate_thread (struct ui_file *fp) const
>>>  {
>>>    if (thread != -1)
>>> -    gdb_printf (fp, " thread %d", thread);
>>> +    {
>>> +      struct thread_info *thr = find_thread_global_id (thread);
>>> +      gdb_printf (fp, " thread %s", print_thread_id (thr));
>>
>> print_thread_id only prints the inferior-qualified thread id if
>> there are multiple inferiors.  I am wondering whether the save breakpoints
>> file should _always_ end up with inferior-qualified thread ids, so that
>> reloading the saved file works the same if you meanwhile add another
>> inferior.
> 
> As a counter argument; if the user has a single inferior and places
> breakpoints on a particular thread, we'll have a save like:
> 
>   break foo thread 2
> 
> Then if the user sets up two inferiors, they can select which inferior
> the breakpoints should apply to - source the saves from inferior 2, and
> the b/p will apply to inferior 2 threads, source from inferior 1, and
> the b/p will apply to inferior 1 threads.
> 
> If the user has changed the inferior setup when sourcing the breakpoint
> save file, I think they have to take some responsibility for knowing
> what they want ... maybe?
> 
> If you feel strongly then it's easy enough to print the qualified
> thread-id, just let me know and I'll get it done.
> 

My thinking is that internally, the thread is really inferior-qualified.
It is just a presentation detail in the CLI that we don't print the
inferior when there's only one inferior, for backwards compatibility.
That may even change in the future.  An MI frontend / GUI may be presenting
the qualified ID, for instance.

It seems to be that there are two valid approaches:

#1 - we consider what the user typed when the breakpoint was created as canonical,
     and thus we save the breakpoint using the same breakpoint spec string that
     user typed originally, meaning, if the user typed:

       "break foo thread 1"

     then that's what we'd save, even if the user added a second
     inferior after creating the breakpoint.

     Of course, it follows then that if the breakpoint is created with

       "break foo thread 1.1"

     then that's what we save.  So the user would have the option.

     I'm really not sure whether this is an option that we should be giving
     users, though.  What if the breakpoint was created via Python, or via the
     MI with --thread ?  Then the concept of original "thread" may not even exists,
     even though we save such a breakpoint too.

#2 - we consider that the thread that the breakpoint ended up bound to is what
     is canonical and thus we always print the qualified id to the file.

The approach in your patch is neither of the above -- it prints the qualified
or non-qualified thread id depending on a CLI presentation detail, which seems
brittle to me.

Option #2 seems the simplest to explain, document, and implement, to me,
but I could be convinced to go with #1 too.

Pedro Alves

> Thanks,
> Andrew
> 
>>
>> Otherwise,
>>
>>  Approved-By: Pedro Alves <pedro@palves.net>
>