From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com [IPv6:2a00:1450:4864:20::42c]) by sourceware.org (Postfix) with ESMTPS id 25E79387089F for ; Tue, 2 Mar 2021 11:15:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 25E79387089F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x42c.google.com with SMTP id b18so12941731wrn.6 for ; Tue, 02 Mar 2021 03:15:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=desbfKNLjtd3V7jJr74nZKkkI4ku5fnNsncqLXjb/hM=; b=EBLqPcdQkUzNa0bQs2IVq4FNqKxAqpVwRpzgw9CcxcLz7UqbDoyd2/pbJ1S1Y6px0e bl0JIPOngkB+MIFeN/wPyyHxzWP5kE15epdOLUFYS8jzkgPABNJGmI43iTsZGqhj5Wf4 2hL+bpT2kPlI2fKw0c3BafHyDwN/OSFlWRp2ZqG9Wh5YbgGj62+RI6h1NJywA4aZ78ax pZibJuYHFqzhG9OPYKyRGI3pMMAXR638NCGc2XyAWco5TFSgaieTa9D4xiR5ZxUmdSt4 23tM/6KgMnxSn+FYJeorVqQh8yPJ0JWQucHuf9xnhupXytW/5JKpxuySJqToLayLa3S1 2sFQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=desbfKNLjtd3V7jJr74nZKkkI4ku5fnNsncqLXjb/hM=; b=OX54VQeqswbfLQwyw+j4JH/b9s3LE60KKVdN4wTDLeOv1oZJ/h+f9BQzEIvaxz9zEY QeJhB7i7/U4ACnYvN4C0hgj4/ZUjBPCkZWj2lSasUf9JwD3t7dxY/Lr8/TUIGR8Sj8bf KNF4Ae3OM399uxGOuHpBdGHkuWf3UR1QSKRpMvm/sGZOPF8FMVoWoibagONRp1w66JZv yrUIZKouBnL3PSUm0Gw8LRKPoLB+vhDV3fSt9GvIEqrvNgC0fEQLPLfz8bkoZJB1tY8t dy9WZx7nQKyBBoAbFdNt52QgbN1k/AjQwUrKX+Ku1RKUac9G0NqB2tJlALZlIfNDV/CE pqtQ== X-Gm-Message-State: AOAM533I16mZSHNdV5Imtet7d4nz8kTcx7Z/KNDlsKbXaG+yjpiQ/8J5 u4spUJaovvRnCOazzkvfYGr/CA== X-Google-Smtp-Source: ABdhPJxmgis9M8CLMvVQsSp1OWOJvDOBaPzQEYfdQ0olY2hWt2VfSjW0cZfyyJHneWFvXXqo9H+V+g== X-Received: by 2002:adf:8b0d:: with SMTP id n13mr21929653wra.94.1614683728187; Tue, 02 Mar 2021 03:15:28 -0800 (PST) Received: from localhost (host86-186-80-154.range86-186.btcentralplus.com. [86.186.80.154]) by smtp.gmail.com with ESMTPSA id m132sm2276265wmf.45.2021.03.02.03.15.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Mar 2021 03:15:27 -0800 (PST) Date: Tue, 2 Mar 2021 11:15:25 +0000 From: Andrew Burgess To: Natalia Saiapova Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 2/6] gdb/infrun: in condition evaluation resume only current thread. Message-ID: <20210302111525.GP265215@embecosm.com> References: <20201009112719.629-1-natalia.saiapova@intel.com> <20201009112719.629-3-natalia.saiapova@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201009112719.629-3-natalia.saiapova@intel.com> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 10:44:58 up 83 days, 15:29, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Tue, 02 Mar 2021 11:15:34 -0000 * Natalia Saiapova via Gdb-patches [2020-10-09 13:27:14 +0200]: > When stopped at a conditional BP, GDB evaluates the condition for the > event thread in order to decide whether the thread should be stopped or > resumed. Thus, if the condition includes an inferior call, during > the call not all threads have finalized state, some of them might > still be shown as running. While executing the proceed for the inferior > call, GDB tries to resume all non-exited threads. This leads to > the following assertion, when GDB tries to resume a running thread: > > (gdb) b 43 if foo() > Breakpoint 1 at 0x119b: file main.c, line 43. > (gdb) run > Starting program: main > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". > [New Thread 0x7ffff77c4700 (LWP 21421)] > [New Thread 0x7ffff6fc3700 (LWP 21422)] > gdb/nat/x86-linux-dregs.c:146: internal-error: void x86_linux_update_debug_registers(lwp_info*): Assertion `lwp_is_stopped (lwp)' failed. > A problem internal to GDB has been detected, > further debugging may prove unreliable. > > This is a bug, please report it. For instructions, see: > . > > This patch changes the behavior, so if the current thread is in the > middle of a condition evaluation, only the current thread is resumed. > > gdb/ChangeLog: > 2020-08-26 Natalia Saiapova > Tankut Baris Aktemur > > * infrun.c (user_visible_resume_ptid): In BP condition > evaluation, resume only the current thread This patch is definitely going to need a test. Maybe some of the tests you add in later patch(es) cover this case, but ideally tests should be included in the patch that fixes the issue. The reason why bundling tests with the patch is good thing is that if in the future I'm run into a problem with this patch then I can easily (git blame) find this patch. If the tests are included in the same commit then I know that if I modify this code then these specific tests must still work. But, I hear you say, surely I run all tests when changing GDB? Yes, but as GDB changes it might be that the test you add now no longer exercises this code, thus, removing, or changing, this code might no longer cause the tests you add to fail. At that point I'm stuck, this code is basically untested. If I have the tests included in this commit then I can start to dig in to figure out why this code is no longer needed for those tests to pass. > > Co-authored-by: Natalia Saiapova > Co-authored-by: Tankut Baris Aktemur > > Signed-off-by: Natalia Saiapova > --- > gdb/infrun.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index d552fb3adb2..91271c2ddbe 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -2156,6 +2156,14 @@ user_visible_resume_ptid (int step) > mode. */ > resume_ptid = inferior_ptid; > } > + else if (inferior_ptid != null_ptid > + && inferior_thread ()->control.in_cond_eval) > + { > + /* The inferior thread is evaluating a BP condition. Other threads > + might be stopped or running and we do not want to change their > + state, thus, resume only the current thread. */ > + resume_ptid = inferior_ptid; > + } I'm not sure about this. We set threads going during an inferior call so that any cross-thread interaction will not block. At a minimum this probably requires additional documentation. I'd be interested to understand more about the different states that a thread might be in here, and what conditions might put them into that state. If we assume a process with threads A, B, and C, then what happens is: 1. Thread A bits conditional breakpoint, threads B & C are still running, 2. GDB spots A has stopped and needs to execute the condition. In this case we should only resume A as B & C are still running. But, we might also have: 1. Thread A hits condition breakpoint, threads B & C are still running, 2. Thread B hits condition breakpoint, thread C is still running, 3. GDB spots A has stopped and needs to execute the condition. In this case it's not clear what you would do... if you resume B to evaluate A then you'll miss B's breakpoint hit. But neither can you evaluate B as that would require setting A running again. Hmm, I think I've probably convinced myself that your approach might be the only safe solution. I think that I'd like to see the commit message expanded to give more depth rather than just saying: "...some of them might still be shown as running", I think you should say, what states a thread could be in and why. Also I think this probably is worth documenting in the manual (that b/p conditions might be evaluated with some threads stopped, even when running in all-stop mode). Thanks, Andrew > else if (!sched_multi && target_supports_multi_process ()) > { > /* Resume all threads of the current process (and none of other > -- > 2.17.1 > > Intel Deutschland GmbH > Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de > Managing Directors: Christin Eisenschmid, Gary Kershaw > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928 >