From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f47.google.com (mail-wm1-f47.google.com [209.85.128.47]) by sourceware.org (Postfix) with ESMTPS id E03043858438 for ; Mon, 29 Nov 2021 11:50:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E03043858438 Received: by mail-wm1-f47.google.com with SMTP id m25-20020a7bcb99000000b0033aa12cdd33so5763601wmi.1 for ; Mon, 29 Nov 2021 03:50:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=S6V4zCU6v2BfmLwMCh/bV4OoVI+yF2tVcZqSaXpnWlU=; b=rCLs2sMxGuQcs0yIaDJ02vWobG3raxFoIzF5EJujHba7q3diFFf1Q99+GhKz3hNQUL 8MvdYSEGjqn5Tv6+iuwTba8v4/yQJ5buNLBiOdDLDOzohslgyz6rsHhKe+w57KnwISKp NZjH2APmsIH/XmXTOMiNx4jKwfrhGigp+n66QHv9cFQI34DJUuaiuDfl1emyt2bCIIWA bccpY3FfjHQI2cDNNRO8KWf0oGLsAVow+530M1P381yJ8XT/fNxCSDj6ALbFgHjPqWHz OXvWqEFKNC+CnrCgGipi21Hyz6MVK+5K8M4tb8pP+D23OBVlnxbAzq691WG+ulz3rVZs sfww== X-Gm-Message-State: AOAM532k94Ds+gg2n6w7dA84nuecGDUkC5L/dKBmxvfpuO03RBviDhPX Y3E3B+CdeOcJBRdNuZkCJ1wTzo3bQec= X-Google-Smtp-Source: ABdhPJzEG6pHJrvxedROUpOAsKTxnSWGYovM92Q331ztzAjyQTKklPXVT/TXvOXZXc35Pw1438b0Vw== X-Received: by 2002:a7b:c0d5:: with SMTP id s21mr9296440wmh.115.1638186650906; Mon, 29 Nov 2021 03:50:50 -0800 (PST) Received: from [192.168.8.216] ([83.219.56.252]) by smtp.gmail.com with ESMTPSA id ay21sm18680306wmb.7.2021.11.29.03.50.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 Nov 2021 03:50:50 -0800 (PST) Message-ID: <08be85df9760d969375f31febb1df71c0936384e.camel@labware.com> Subject: Re: [PATCH v2 1/2] ppc: use 'trap' ('tw, 31, 0, 0', 0x7fe00008) as breakpoint instruction From: Jan Vrany To: "gdb-patches@sourceware.org" Cc: Joel Brobecker Date: Mon, 29 Nov 2021 11:50:49 +0000 In-Reply-To: References: <20211123154237.2335848-1-jan.vrany@labware.com> <20211124130926.2412617-1-jan.vrany@labware.com> <20211124130926.2412617-2-jan.vrany@labware.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.3-1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Mon, 29 Nov 2021 11:50:54 -0000 Hi Joel On Sat, 2021-11-27 at 10:45 +0400, Joel Brobecker wrote: > Hi Jan, > > On Wed, Nov 24, 2021 at 01:09:25PM +0000, Jan Vrany via Gdb-patches > wrote: > > GDB used to use "tw 12, r2, r2" as a breakpoint instruction. While > > it > > works, the PowerPC specifies 'tw, 31, 0, 0' (0x7fe00008) as the > > canonical unconditional trap. > > --- > >  gdb/rs6000-tdep.c | 4 ++-- > >  1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > > index 87a494e0bb8..43880fa4426 100644 > > --- a/gdb/rs6000-tdep.c > > +++ b/gdb/rs6000-tdep.c > > @@ -824,8 +824,8 @@ rs6000_fetch_pointer_argument (struct > > frame_info *frame, int argi, > >   > >  /* Sequence of bytes for breakpoint instruction.  */ > >   > > -constexpr gdb_byte big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 }; > > -constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d > > }; > > +constexpr gdb_byte big_breakpoint[] = { 0x7f, 0xe0, 0x00, 0x08 }; > > +constexpr gdb_byte little_breakpoint[] = { 0x08, 0x00, 0xe0, 0x7f > > }; > >   > >  typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint) > >    rs6000_breakpoint; > > I'm going to say OK, but I'd like to know what your source is > for this, so we can document that change. Part of me wonders > why we're making a change like this one on some code which > works, which is always a risk, no matter how small, unless we have > enough evidence that this is going to be helpful in practice. According to Power ISA 3.0 B spec [1], section 3.3.11 Fixed-Point Trap Instructions (also section C.6 Trap Mnemonics), unconditional trap is encoded as `tw 31, 0, 0`. While debugging a problem on POWER (gdb not stopping on a `trap` instruction) I found that GDB is using `tdnl r2, r2` (trap if not less than) which works but it is different than unconditional trap as specified. I could not find out why `tdnl` was used instead of unconditional `trap`. I thought it is  more "natural" to use canonical unconditional trap. So this is merely a "cosmetic" change. I agree that I should have included reference to the spec in the commit message and maybe also add comment to the code. Will do in next version (should we decide to keep this patch). Now, I do understand the risk of changing something that works. I'm happy to drop this patch since the next one in this miniseries  fixes the problem we were experiencing properly (that is, GDB not stopping on software-planted breakpoint instructions). What do you think? Thanks!  Jan [1]: https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0 > > Thank you,