Re: [PATCH] gdb: make "start" breakpoint inferior-specific

[Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>]

On 04/11/2022 17:52, Simon Marchi wrote:
> On 8/31/22 10:03, Bruno Larsen via Gdb-patches wrote:
>> On 04/08/2022 19:40, Simon Marchi via Gdb-patches wrote:
>>> I saw this failure on a CI:
>>>
>>>       (gdb) add-inferior
>>>       [New inferior 2]
>>>       Added inferior 2
>>>       (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
>>>       inferior 2
>>>       [Switching to inferior 2 [<null>] (<noexec>)]
>>>       (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
>>>       kill
>>>       The program is not being run.
>>>       (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
>>>       Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
>>>       (gdb) run &
>>>       Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
>>>       (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
>>>       inferior 1
>>>       [Switching to inferior 1 [<null>] (<noexec>)]
>>>       (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
>>>       kill
>>>       The program is not being run.
>>>       (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
>>>       Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
>>>       (gdb) break should_break_here
>>>       Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
>>>       (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
>>>       [Thread debugging using libthread_db enabled]
>>>       Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>>       start
>>>       Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
>>>       Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
>>>       [Thread debugging using libthread_db enabled]
>>>       Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>>
>>>       Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
>>>       23      sleep (30);
>>>       (gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
>>>
>>> What happens is:
>>>
>>>    1. We start inferior 2 with "run&", it runs very slowly, takes time to
>>>       get to main
>>>    2. We switch to inferior 1, and run "start"
>>>    3. The temporary breakpoint inserted by "start" applies to all inferiors
>>>    4. Inferior 2 hits that breakpoint and GDB reports that hit
>>>
>>> To avoid this, breakpoints inserted by "start" should be
>>> inferior-specific.  However, we don't have a nice way to make
>>> inferior-specific breakpoints yet.  It's possible to make
>>> pspace-specific breakpoints (for example how the internal_breakpoint
>>> constructor does) by creating a symtab_and_line manually.  However,
>>> inferiors can share program spaces (usually on particular embedded
>>> targets), so we could have a situation where two inferiors run the same
>>> code in the same program space.  In that case, it would just not be
>>> possible to insert a breakpoint in one inferior but not the other.
>>>
>>> A simple solution that should work all the time is to add a condition to
>>> the breakpoint inserted by "start", to check the inferior reporting the
>>> hit is the expected one.  This is what this patch implements.
>>>
>>> Add a test that does:
>>>
>>>    - start a background inferior 1 that calls main in a loop
>>>    - start aninferior 2 with the "start" command
>>>    - validate that we hit the breakpoint in inferior 2
>>>
>>> Without the fix, we hit the breakpoint in inferior 1 pretty much all the
>>> time.
>>>
>>> Change-Id: Ib0148498a476bfa634ed62353c95f163623c686a
>> Hi Simon,
>>
>> This change makes a lot of sense, I just wonder if it would make sense to make breakpoints inferior-specific. I could see a situation where someone would run the same program twice, one with an ok input while the other has a buggy input for instance, to see the difference between them, and inferior-specific breakpoints could come in handy in a situation like this.
>>
>> If you think it would be too much work for a usecase that is too niche, this patch is pretty straight forward, I'd suggest you approve your patch.
> I certainly don't want to add a new user-facing feature just for this
> fix, as that's a lot of work (figuring out the syntax, doc, NEWS, tests,
> etc).  We could enhance the internal API to allow passing an inferior to
> make a breakpoint inferior-specific, but I don't think it should be a
> prerequisite for this fix.  It would end up doing functionally
> identical.

What I had in mind is closer to your second idea, just internal API 
changes to keep track of the inferior, though maybe I just hadn't 
thought through the suggestion, since letting users add breakpoint to 
other inferiors makes sense given the use case I mentioned above.

This is a long winded way of saying I agree that this is unreasonable 
feature creep for a functionally identical end result for now, so the 
patch LGTM.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Cheers,
Bruno

>
> Would you like to to add your Reviewed-By on this patch?
>
> Simon
>