From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) by sourceware.org (Postfix) with ESMTPS id 7C40F38555B3 for ; Fri, 7 Jul 2023 15:20:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7C40F38555B3 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-wr1-f42.google.com with SMTP id ffacd0b85a97d-3143b72c5ffso2261579f8f.3 for ; Fri, 07 Jul 2023 08:20:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688743256; x=1691335256; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=BqMEpqCzXV3nTRKOS7lvMPPurBDMMUgcYu/wyUmo08M=; b=IPpKND/g/wS4YYNTGmhIby1Yo8Ac/cjuOj2Zn+hHYB+rVEQHLO/7eyqpP52dV7BG6q 9zFR+cNf+hjs8BMf0Dmjs5wCwXnK0LQWsI3tuc6wZ6e2cAL9hyu06M2S4X8XdAO1NLfc qXj2rEFlHDcUWq+7SIDZLicMx3tQFIAQw7uuBk02fQWkrfwbwM+5ePKy/VBjazQ0yRXl JEhQ7TgaaonWIEI3DvM5xBUfE5GapyUS+ZsCOuoYb5p+PQ786JFTF6itH2lOuqW9jfov hHTtXAmg1Em1taYXlIAQkrcu6SoiEIIWg7svqDgCVTKbpoY73jzTouXWiahpui2aTE13 e3kA== X-Gm-Message-State: ABy/qLZ7WVOhpNeZjQoYfdufssnvxaGEVna0QzM6yFpScNkCdlgLwyZv liU6L1Pv8tmLckQLDWjAM1L5AVpGT1w= X-Google-Smtp-Source: APBJJlHfclzrjlJw9XBqPHVmFEyGPSfg4OK7/JcyFmEL3GeZtiHbWksZamoFI+6sTzu9UzF5hOsNpQ== X-Received: by 2002:adf:fec7:0:b0:313:f7de:6356 with SMTP id q7-20020adffec7000000b00313f7de6356mr5938925wrs.15.1688743256082; Fri, 07 Jul 2023 08:20:56 -0700 (PDT) Received: from ?IPV6:2001:8a0:f91d:bc00:1b6e:208c:60cc:dcc? ([2001:8a0:f91d:bc00:1b6e:208c:60cc:dcc]) by smtp.gmail.com with ESMTPSA id z19-20020a1c4c13000000b003fbcdba1a63sm2722652wmf.12.2023.07.07.08.20.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 07 Jul 2023 08:20:55 -0700 (PDT) Message-ID: <6fd4aa13-6003-2563-5841-e80d5a55d59e@palves.net> Date: Fri, 7 Jul 2023 16:20:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCHv5 05/11] gdb: don't always print breakpoint location after failed condition check Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: From: Pedro Alves In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi! On 2023-03-16 17:37, Andrew Burgess via Gdb-patches wrote: > The user can still find the number of the breakpoint that triggered > the initial stop in this line: > > Error in testing condition for breakpoint 1: > > But there's now only one stop reason reported, the SIGSEGV, which I > think is much clearer. That's reasonable. > @@ -5545,6 +5546,17 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) > exception_fprintf (gdb_stderr, ex, > "Error in testing condition for breakpoint %d:\n", > b->number); > + > + /* If the pc value changed as a result of evaluating the > + condition then we probably stopped within an inferior > + function call due to some unexpected stop, e.g. the thread > + hit another breakpoint, or the thread received an > + unexpected signal. In this case we don't want to also > + print the information about this breakpoint. */ > + CORE_ADDR pc_after_check > + = get_frame_pc (get_selected_frame (nullptr)); > + if (pc_before_check != pc_after_check) > + bs->print = 0; Not a great fan of this PC heuristic, though. We can reuse the logic stop_id logic used by normal_stop to detect whether a hook-stop changed the execution context. See patch below. It passes the whole testsuite cleanly for me (on native, that's all I tested). Note, it adds thread_state to struct stop_context, because in run_inferior_call, the thread for which we are saving the context for is THREAD_RUNNING. In this particular scenario, probably only comparing the last stop_id would suffice, but, it doesn't hurt to compare the whole context. It can actually be a bit better, in case e.g., the exception was due to the remote target connection dropping, in which case the current get_selected_frame call would throw, while with this approach, we'll just detect that the context changed. (haven't actually tried that scenario, just going by inspection.) The other thing that I think we could/should do better is best seen with MI: Before your change, we had: *stopped,reason="breakpoint-hit",disp="keep",bkptno="3",frame={addr="0x0000555555555149",func="func_bp",args=[],file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="32",arch="i386:x86-64"},thread-id="1",stopped-threads="all",core="2" &"Error in testing condition for breakpoint 2:\n" &"The program being debugged stopped while in a function called from GDB.\n" &"Evaluation of the expression containing the function\n" &"(func_bp) will be abandoned.\n" &"When the function is done executing, GDB will silently stop.\n" =breakpoint-modified,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x000055555555515d",func="foo",file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="39",thread-groups=["i1"],cond="(func_bp ())",times="1",original-location="infcall-failure.c:39"} ~"\n" ~"Breakpoint 2, func_bp () at gdb.base/infcall-failure.c:32\n" ~"32\t int res = 0;\t/* Second breakpoint. */\n" *stopped,reason="breakpoint-hit",disp="keep",bkptno="2",frame={addr="0x0000555555555149",func="func_bp",args=[],file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="32",arch="i386:x86-64"},thread-id="1",stopped-threads="all",core="2" (gdb) Note the last *stopped for the breakpoint we tried to evaluate the condition. While in current master (with your patch merged), we have: *stopped,reason="breakpoint-hit",disp="keep",bkptno="3",frame={addr="0x0000555555555149",func="func_bp",args=[],file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="32",arch="i386:x86-64"},thread-id="1",stopped-threads="all",core="1" &"Error in testing condition for breakpoint 2:\n" &"The program being debugged stopped while in a function called from GDB.\n" &"Evaluation of the expression containing the function\n" &"(func_bp) will be abandoned.\n" &"When the function is done executing, GDB will silently stop.\n" =breakpoint-modified,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x000055555555515d",func="foo",file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="39",thread-groups=["i1"],cond="(func_bp ())",times="1",original-location="infcall-failure.c:39"} *stopped <<<<<<<<<<<<< here (gdb) Note the "*stopped" without any context in the "<<<< here" line. I'd think the second *stopped shouldn't even be there at all, following the logic used to remove the stop from the CLI. Maybe we should try moving or copying this "don't report this stop" logic to fetch_inferior_event, to avoid calling the second normal_stop at all. I gave it a quick try, but what I tried without thinking much about it just hung GDB. I didn't dig too much as I want to move on to review the rest of your series. >From e8584146cfbb372214097a519c5a2738a72fbb4d Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 7 Jul 2023 14:21:18 +0100 Subject: [PATCH] stop_context/stop_id instead of PC Change-Id: I3e70c3fdb95b9bc4996f6c0c18431a3df48cc3a6 --- gdb/breakpoint.c | 18 ++++++++---------- gdb/infrun.c | 30 ++++-------------------------- gdb/infrun.h | 27 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 36 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index da6c8de9d14..f791a74a246 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -5535,9 +5535,11 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) else within_current_scope = false; } - CORE_ADDR pc_before_check = get_frame_pc (get_selected_frame (nullptr)); + if (within_current_scope) { + stop_context saved_context; + try { condition_result = breakpoint_cond_eval (cond); @@ -5548,15 +5550,11 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) "Error in testing condition for breakpoint %d:\n", b->number); - /* If the pc value changed as a result of evaluating the - condition then we probably stopped within an inferior - function call due to some unexpected stop, e.g. the thread - hit another breakpoint, or the thread received an - unexpected signal. In this case we don't want to also - print the information about this breakpoint. */ - CORE_ADDR pc_after_check - = get_frame_pc (get_selected_frame (nullptr)); - if (pc_before_check != pc_after_check) + /* If we stopped within an inferior function call, + e.g. the thread hit another breakpoint, or the thread + received an unexpected signal, don't print the + information about this breakpoint. */ + if (saved_context.changed ()) bs->print = 0; } } diff --git a/gdb/infrun.c b/gdb/infrun.c index 58da1cef29e..3dd24fccf6e 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -8755,31 +8755,6 @@ maybe_remove_breakpoints (void) } } -/* The execution context that just caused a normal stop. */ - -struct stop_context -{ - stop_context (); - - DISABLE_COPY_AND_ASSIGN (stop_context); - - bool changed () const; - - /* The stop ID. */ - ULONGEST stop_id; - - /* The event PTID. */ - - ptid_t ptid; - - /* If stopp for a thread event, this is the thread that caused the - stop. */ - thread_info_ref thread; - - /* The inferior that caused the stop. */ - int inf_num; -}; - /* Initializes a new stop context. If stopped for a thread event, this takes a strong reference to the thread. */ @@ -8794,7 +8769,10 @@ stop_context::stop_context () /* Take a strong reference so that the thread can't be deleted yet. */ thread = thread_info_ref::new_reference (inferior_thread ()); + thread_state = thread->state; } + else + thread_state = THREAD_EXITED; } /* Return true if the current context no longer matches the saved stop @@ -8807,7 +8785,7 @@ stop_context::changed () const return true; if (inf_num != current_inferior ()->num) return true; - if (thread != nullptr && thread->state != THREAD_STOPPED) + if (thread != nullptr && thread->state != thread_state) return true; if (get_stop_id () != stop_id) return true; diff --git a/gdb/infrun.h b/gdb/infrun.h index a343d27f72d..b5bd00f5561 100644 --- a/gdb/infrun.h +++ b/gdb/infrun.h @@ -416,5 +416,32 @@ struct scoped_enable_commit_resumed bool m_prev_enable_commit_resumed; }; +/* The execution context that just caused a normal stop. */ + +struct stop_context +{ + stop_context (); + + DISABLE_COPY_AND_ASSIGN (stop_context); + + bool changed () const; + + /* The stop ID. */ + ULONGEST stop_id; + + /* The event PTID. */ + ptid_t ptid; + + /* If stopped for a thread event, this is the thread that caused the + stop. */ + thread_info_ref thread; + + /* If stopped for a thread event, this is the state of the thread + that caused the stop. */ + enum thread_state thread_state; + + /* The inferior that caused the stop. */ + int inf_num; +}; #endif /* INFRUN_H */ base-commit: c0c3bb70f2f13e07295041cdf24a4d2997fe99a4 -- 2.34.1