Re: [PATCHv2 3/6] gdb: fix display of thread condition for multi-location breakpoints

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> On 2023-02-06 2:48 p.m., Andrew Burgess wrote:
>> Pedro Alves <pedro@palves.net> writes:
>> 
>
>>>    (gdb) info breakpoints
>>>    Num     Type           Disp Enb Address            What
>>>    2       breakpoint     keep y                      foo thread 1
>>>            stop only in thread 1   
>>>     2.1                        y   0x0000000000401114  in foo at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25
>>>     2.2                        y   0x0000000000401146  in foo at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25
>>>     2.3                        y   0x0000000000401168  in foo at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25
>>>    3       breakpoint     keep y                      bar thread 1
>>>            stop only in thread 1
>>>     3.1    breakpoint     keep y   0x000000000040110a  in bar at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:32
>>>
>>> ... for instance.  Or not include the "thread 1".  But my point is that if we were to show it, that's where we would
>>> show it, not in the breapoint locations.
>> 
>> I dug out your multi-location proposal, and it looks good.  It's a shame
>> it got bogged down as it did....
>
> Thanks.  I still hope to get back to it at some point, but probably only after I manage to
> be done with the step over thread clone/exit series, and the big ctrl-c rework.
>
>>> To fix that, we should merge those three gdb_asserts into a single gdb_assert,
>>> and we should $gdb_test_name for its test name, along with passing a meaningful
>>> test name to gdb_test_multiple, so internal FAILs get that meaningful name
>>> as well.  If we want to show the individual conditions, that can
>>> still be done by outputting them to the log.
>> 
>> Done.
>
> Thank you.
>
>> 
>> I agree that, if/when your multi-location work is merged we might want
>> to once again adjust how this information is displayed, inline with your
>> suggestion above.
>> 
>> However... how would you feel if this patch (as shown below) was merged
>> now?  I think this fixes the stray "thread 1" text immediately, and the
>> multi-location display patch should be easily updated on top of this, if
>> that was something that you plan to continue developing?
>
> Oh yes, I totally agree with merging your patch.  I only referenced the
> multi-locations proposal to help with justifying why I think your patch
> is good.  I did not mean to suggest that getting my proposal in would
> remove the need for your patch.
>
> In sum, in my view, code locations are not thread specific, so the "thread N" part
> should not be displayed as if part of the location.
>
> Approved-By: Pedro Alves <pedro@palves.net>

I pushed this.

Thanks,
Andrew