From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id KlIVCdUqbWNHlBYAWB0awg (envelope-from ) for ; Thu, 10 Nov 2022 11:46:13 -0500 Received: by simark.ca (Postfix, from userid 112) id 17B9C1E124; Thu, 10 Nov 2022 11:46:13 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00,MAILING_LIST_MULTI, NICE_REPLY_A,RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 504E61E0CB for ; Thu, 10 Nov 2022 11:46:12 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F3FD9383B78D for ; Thu, 10 Nov 2022 16:46:08 +0000 (GMT) Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) by sourceware.org (Postfix) with ESMTPS id D927F385558F for ; Thu, 10 Nov 2022 16:45:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D927F385558F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f44.google.com with SMTP id h133-20020a1c218b000000b003cf4d389c41so3863267wmh.3 for ; Thu, 10 Nov 2022 08:45:52 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=W8LAYiP+Y1Sc4LIS5UaJO8+vA0I7sEdy0i7OoU16Z8Q=; b=ivJE/hENWRMnmqEzd+NA5r9xg7PDCEyglxKl5Q66edgvrOhQge4Num39B8h0Hzjzpw yN5bdCH/IhSA5RLBXdP8jIQdzIYP5L5QRUvzbKrxOjzwGPeHS5MMvvOdQdn3ciUe0o27 b6bBRFv0l+BXyIES3VO4nwHYH8QzblZK9b869ilme29c6HFYTcQJL6GB2bgEDB0G8Mk0 aUbURp7Cr2R9JYvolfBwlZWnwY5TMadHCrY396Jt/uJa3aIRo0R2cHsrdCVWk8uMWBBy XDC0+FuEyHS0pW/lRpkywF2tVg43eNqlxKbThPNkFtRbaGjNe0CPqP/bbO/Cx0LKiCEP nrdA== X-Gm-Message-State: ACrzQf3ig00Qf7/q4LheA5o+jLWUUyX08KM7AZZFg0E3FoRAAqmyTqlk eYQm5+qkosuvESSPwdE0n2aX6KR9agM= X-Google-Smtp-Source: AMsMyM5+WncE4ohrs+oR33kKQNN4S5XXTP4obYvnqkx/qeGF/OEYJM3qAjexoXZCo+2Mqg1b+Ekhsw== X-Received: by 2002:a05:600c:1d96:b0:3cf:7cdb:fbc2 with SMTP id p22-20020a05600c1d9600b003cf7cdbfbc2mr34905878wms.37.1668098751101; Thu, 10 Nov 2022 08:45:51 -0800 (PST) Received: from ?IPv6:2001:8a0:f93a:3b00:e038:5cdc:b8bf:4653? ([2001:8a0:f93a:3b00:e038:5cdc:b8bf:4653]) by smtp.gmail.com with ESMTPSA id j15-20020a5d604f000000b00236705daefesm16157784wrt.39.2022.11.10.08.45.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 10 Nov 2022 08:45:50 -0800 (PST) Subject: Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific To: Simon Marchi , gdb-patches@sourceware.org References: <691c5a58-68ae-5fe9-2f3d-34fb7af69ad0@palves.net> <20221108212008.1792090-1-simon.marchi@efficios.com> From: Pedro Alves Message-ID: <47b696c4-6584-8165-0799-5d742132361a@palves.net> Date: Thu, 10 Nov 2022 16:45:49 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20221108212008.1792090-1-simon.marchi@efficios.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On 2022-11-08 9:20 p.m., Simon Marchi via Gdb-patches wrote: > New in v2: > > - Change the test so it doesn't call the main function > > 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 [] ()] > (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 [] ()] > (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. > Even though this does work, it still sets the breakpoint on all the pspaces unnecessarily. It would be nice if the breakpoint was pspace specific, in addition to inferior specific like you have (or some other way). But, what you have is fine with me as is, as it is better than what we have today. Maybe just add a little comment suggesting that it would be even better to make the breakpoint apply to the current pspace only? > Add a test that does: > > - start in background inferior 1 that sleeps 3 seconds before reaching > its main function (using a sleep in a global C++ object constructor) > - start inferior 2, which also sleeps 3 seconds before reaching its m > ain function, 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. There could be some unfortunate scheduling causing the test not > to catch the bug, for instance if the scheduler decides not to schedule > inferior 1 for a long time, but it would be really rare. If the bug is > re-introduced, the test will catch it much more often than not, so it > will be noticed. My thinking when I saw that both inferiors wait 3 seconds before reaching main (before reading the commit log), was, "???, I don't understand this." So this is assuming that because the first inferior was started first, that its 3 seconds always finish before the second inferior's 3 seconds? That seems a bit risky. gdb and the other inferiors may all be running on different cores, and on a fast machine, gdb may be fast enough to spawn both processes roughtly at the same time, and then inferior 2 may end up reporting the hit first. Why not make it so that inferior 3 takes like 3 seconds to reach main, but inferior 2 takes 4 or 5 seconds? Or 2 vs 4. Something like that. I.e., make sure that inferior 2's time is larger than inferior 1's. That could be done by tweaking the sleep calls in the programs, or adding a sleep call in the .exp file between "run&" and starting the second inferior. But maybe I'm missing something? > +proc do_test {} { > + # With remote, to be able to start an inferior while another one is > + # running, we need to use the non-stop variant of the protocol. > + save_vars { ::GDBFLAGS } { > + if { [target_info gdb_protocol] == "extended-remote"} { > + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\"" > + } > + > + clean_restart ${::binfile_other} > + } > + > + gdb_test "run&" "Starting program: .*" "start background inferior" I was going to point out that if the inferior prints something, this can timeout, as that output would appear after the prompt. I then looked around the tree for "run&" uses, to confirm we are using gdb_test_multiple with that, and found that you just recently added "gdb_test -no-prompt-anchor", for exactly this scenario. :-) I think that should be used here. > + gdb_test "add-inferior" "Added inferior 2.*" > + gdb_test "inferior 2" "Switching to inferior 2.*" > + gdb_file_cmd ${::binfile} > + gdb_test "start" "Thread 2.1 .* hit Temporary breakpoint .*" > +} > +