From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 5A4463858D21 for ; Wed, 6 Nov 2024 14:24:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5A4463858D21 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 5A4463858D21 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1730903058; cv=none; b=eiQG2sQkECeerVhNTs4vbUVT/zHBydciKwlmHQhwXcwiVVQcJ02Fmu649lfO/lHyerFhhVrxCnTYNNtAoWs/viYVmcDlMYidTSjEQafSWZkW9lHZ1HuLAXeKK/L2LVUs+DqF0xNv6Kvlogc23xWXJ6+qz3bb2UepqQMTSef78AI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1730903058; c=relaxed/simple; bh=CWd1puRuXl/zpWwgNCpBvsf3kIZgHPa/tnFJjJZJEZA=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=WDKVvSImd4O5yb/eOI8G6IYFk/ufpo8BsDsU20z2eqxwfDuyAniGP8x2xEQTqfa0iHI3Ea393+Yi0bNtHYaAf7fiYMGAYrYMVWjqPFtEeXwb6kXqxaYGQ7kSarNbJBgjdQD3VK4c4tWOkxjYL+qom5FzbtugCkex4KqJ1jfvj1I= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1730903055; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=8P0NAdY+V45lYgr34ubN2IzE7bQBnyIXPlRat/AXTqQ=; b=BbcJWzg9F49flg7zj3pYn0cJzNBdOshwjARtICztydVqrJrW+3E4dNp+VdSF8SxrB2d3Ic h9IzEa8Q60dg7jzb4Wf7jRs6zlBc65gfRCHXWmr8dGbzL7VihIRv6L5/lsdIH2YobRZ+8E 5gysjiuY5ivRYuEWwS5fD+qFAsOV+w4= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-94-z-BWv6MDObGHwQ4Nv-klQQ-1; Wed, 06 Nov 2024 09:24:14 -0500 X-MC-Unique: z-BWv6MDObGHwQ4Nv-klQQ-1 X-Mimecast-MFC-AGG-ID: z-BWv6MDObGHwQ4Nv-klQQ Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-381d0582ad3so3403153f8f.0 for ; Wed, 06 Nov 2024 06:24:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730903052; x=1731507852; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=8P0NAdY+V45lYgr34ubN2IzE7bQBnyIXPlRat/AXTqQ=; b=SWVB+EKk9qVSkr0SFuA3qLGOQiJS+ugXqchcJ3+LIvQr5nudyLypPDMxrPmunBjcb+ niscE2kWBwZM9h4/iQsFr98EfH+m3uRgZWKIGwKFo4ZYiJLan3c5+o2B1cpB/FbDiZof KswnC93hGQ/vAaTQDdJwiBRWNOByid6ejmlRImb/vFOCTQDRkAReEW5tI0EZCfVURkvo /f6hG+0iqY4sDlqQx+7wSqWdd5L9h/o0MfI8Jzu6vHrBk1y667UF9/zJc/c2YMs/MfBY gPfnqKbF5WDbuGDmJCgCRAphCDhlXMFFY9NNCOT2WmiSwBgXw+kuQJC08zDQS7EA4SdP E8dA== X-Forwarded-Encrypted: i=1; AJvYcCWKDmRk/tqAXNOe9Az8JbaB1lqGAfDzqgZte5QeXZWaMCRmwWC1V7fddajzKxu1hpvUDjUpnZE28g2A2g==@sourceware.org X-Gm-Message-State: AOJu0YxBUuMeqtOFPnXwJwgr1Issk/jv+5iEODKrtWAda9PSC4vVcqKV uktuab8LqKwMS6PA3UdAkJmSbI5mmDy7eclDB0m7GDByggSw6SGwvrtVnk7ssQavIeVFwf98Qv/ E0mjGf3bXuuZ88bHFPtMklpIDf8CO/CUB1UtR5dFubqVYftfFpk1R4icWN7T0D6BfngQ= X-Received: by 2002:a05:6000:4029:b0:37c:cd1d:b870 with SMTP id ffacd0b85a97d-381c7a6c58fmr17851987f8f.29.1730903052415; Wed, 06 Nov 2024 06:24:12 -0800 (PST) X-Google-Smtp-Source: AGHT+IHqq6dM98+4fc+HNhAmFfmsL0jIlqTHOdmHjvUzRIWtZlka1EHMgZ+xqdhbO6V8fl4hR3QA1g== X-Received: by 2002:a05:6000:4029:b0:37c:cd1d:b870 with SMTP id ffacd0b85a97d-381c7a6c58fmr17851958f8f.29.1730903051900; Wed, 06 Nov 2024 06:24:11 -0800 (PST) Received: from localhost (197.209.200.146.dyn.plus.net. [146.200.209.197]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-381c10b7b75sm19193262f8f.15.2024.11.06.06.24.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 Nov 2024 06:24:11 -0800 (PST) From: Andrew Burgess To: Klaus Gerlicher , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: avoid conversion of SIGSEGV to SIGTRAP on user breakpoints In-Reply-To: <20241002141025.591441-1-klaus.gerlicher@intel.com> References: <20241002141025.591441-1-klaus.gerlicher@intel.com> Date: Wed, 06 Nov 2024 14:24:10 +0000 Message-ID: <87ses4qw39.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: MCWlPG-hFpw-wrufLGIAEnv1SMfeaVipw39tSiPE8Q0_1730903053 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP 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: Klaus Gerlicher writes: > From: "Gerlicher, Klaus" > > GDB converts signals GDB_SIGNAL_ILL, GDB_SIGNAL_SEGV and GDB_SIGNAL_EMT to > GDB_SIGNAL_SEGV if a breakpoint is inserted at the fault location. If, due > to imprecise page fault reporting, a breakpoint is at the same address as > the fault address, this signal would always be reported as GDB_SIGNAL_TRAP. > > Limit the check for a breakpoint at the current PC to internal breakpoints. > Internal breakpoints have a number less than or equal to zero. > --- > gdb/breakpoint.c | 18 ++++++++++++++++++ > gdb/breakpoint.h | 5 +++++ > gdb/infrun.c | 8 +++++--- > 3 files changed, 28 insertions(+), 3 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index b80b3522dcb..09526fa557f 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -4569,6 +4569,24 @@ hardware_watchpoint_inserted_in_range (const address_space *aspace, > > /* See breakpoint.h. */ > > +bool > +internal_breakpoint_inserted_here_p (const address_space *aspace, > + CORE_ADDR pc) > +{ > + for (bp_location *bl : all_bp_locations_at_addr (pc)) > + { > + if (bl->owner == nullptr || user_breakpoint_p (bl->owner)) > + continue; > + > + if (bp_location_inserted_here_p (bl, aspace, pc)) > + return true; > + } > + > + return false; > +} > + > +/* See breakpoint.h. */ > + > bool > is_catchpoint (struct breakpoint *b) > { > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h > index cef1d727ed2..6134b546f4e 100644 > --- a/gdb/breakpoint.h > +++ b/gdb/breakpoint.h > @@ -1499,6 +1499,11 @@ extern int hardware_watchpoint_inserted_in_range (const address_space *, > CORE_ADDR addr, > ULONGEST len); > > +/* Returns true if any non-user breakpoints are present and inserted > + at ADDR. Internal breakpoints have a number less than or equal to zero. */ > +extern bool internal_breakpoint_inserted_here_p (const address_space *aspace, > + CORE_ADDR addr); > + > /* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the > same breakpoint location. In most targets, this can only be true > if ASPACE1 matches ASPACE2. On targets that have global > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 4ca15450afe..af9c6d291b7 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -6135,7 +6135,8 @@ handle_inferior_event (struct execution_control_state *ecs) > when we're trying to execute a breakpoint instruction on a > non-executable stack. This happens for call dummy breakpoints > for architectures like SPARC that place call dummies on the > - stack. */ > + stack. Note that only internal breakpoints must be used for this > + check. */ > if (ecs->ws.kind () == TARGET_WAITKIND_STOPPED > && (ecs->ws.sig () == GDB_SIGNAL_ILL > || ecs->ws.sig () == GDB_SIGNAL_SEGV > @@ -6143,8 +6144,9 @@ handle_inferior_event (struct execution_control_state *ecs) > { > struct regcache *regcache = get_thread_regcache (ecs->event_thread); > > - if (breakpoint_inserted_here_p (ecs->event_thread->inf->aspace.get (), > - regcache_read_pc (regcache))) > + if (internal_breakpoint_inserted_here_p ( > + ecs->event_thread->inf->aspace.get (), > + regcache_read_pc (regcache))) Klaus, First, huge thanks for your excellent explanation of this patch in your other reply, that really helped. I think what you're doing above is not quite right though, but maybe I'm still not understanding things. The comment was truncated in the diff above, so let me quote it here again: /* First, distinguish signals caused by the debugger from signals that have to do with the program's own actions. Note that breakpoint insns may cause SIGTRAP or SIGILL or SIGEMT, depending on the operating system version. Here we detect when a SIGILL or SIGEMT is really a breakpoint and change it to SIGTRAP.... In your explanation you said: "I'm assuming based on this that this was really added to support returns from inferior calls. From that I deduce that we would not have to convert the signal for all breakpoint types." I think the SIGSEGV case was added to support inferior function calls, but I think the other two cases are not. The SIGILL and SIGEMT cases are there to support targets where any software breakpoint might generate those signals instead of SIGTRAP, at least, that's my reading of the comment. With your change, for those targets (if I'm right), we'll no longer convert the e.g. SIGILL to SIGTRAP for a user breakpoint. So I think at a minimum you need to restrict the use of internal_breakpoint_inserted_here_p to the SIGSEGV case only. But, I wonder if there's a different approach that could be used? I assume that your out of tree architecture with these imprecise page faults has a new gdbarch to represent it. You are limiting the SIGSEGV -> SIGTRAP conversion because, I assume, you've hit cases where a SIGSEGV occurs and then your target has run on and reported the stop at the address of a user breakpoint. But all you're really doing is reducing the scope for errors. It could be the case that the SIGSEGV is reported at the site of an internal breakpoint, and then you'll still have issues. So what if, instead, you added a new gdbarch method, something like: bool gdbarch_has_imprecise_page_faults (struct gdbarch *gdbarch); The default for this, and for all currently in-tree targets, would be to return true. For your target this will return false. Then in this code we would do: if (ecs->ws.kind () == TARGET_WAITKIND_STOPPED && (ecs->ws.sig () == GDB_SIGNAL_ILL || (ecs->ws.sig () == GDB_SIGNAL_SEGV !gdbarch_has_imprecise_page_faults (gdbarch)) || ecs->ws.sig () == GDB_SIGNAL_EMT)) How does this approach sound? Thanks, Andrew > { > infrun_debug_printf ("Treating signal as SIGTRAP"); > ecs->ws.set_stopped (GDB_SIGNAL_TRAP); > -- > 2.34.1 > > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928