From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 2C13F3858000 for ; Thu, 31 Aug 2023 17:57:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2C13F3858000 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 37VHupQc007023 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 31 Aug 2023 13:56:56 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 37VHupQc007023 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1693504617; bh=z2UAGohidIci1e4QzMTn+BGV8yNVeD7h2nHfxi/w0Xw=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=NCJ3JCo3i7QHwzv83vUapLSQIwLZZgqVyjQT8VLiuaRjxxiQPsncT6/HJK+rLAGZy RVGtcMcvzJfooowdIA7RJUgzbPMwud12pFZn/7v+6WDtGPoigesngxGtwa3Cmh4edd pH/DXEhOJWRd8d5s9aO7y8p27brehZZktCuaWXSY= Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9AE261E092; Thu, 31 Aug 2023 13:56:51 -0400 (EDT) Message-ID: <6dc411fa-5e25-44a1-81b4-9454aa424507@polymtl.ca> Date: Thu, 31 Aug 2023 13:56:50 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH^2] gdb: mips: Add MIPSR6 support Content-Language: fr To: Dragan Mladjenovic , "gdb-patches@sourceware.org" Cc: Chao-ying Fu , "Maciej W . Rozycki" References: From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 31 Aug 2023 17:56:52 +0000 X-Spam-Status: No, score=-3031.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,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: Hi Dragan, I can do a superficial review (formatting / coding style / general feeling), but can't speak about the behavior. Ideally Maciej would look at it, but if he's not available, I would be fine approving the patch, since it's isolated to the MIPS support. The code looks nice and well-organized. Not absolutely necessary, but it would be nice to have tests (arch-specific tests in assembly in gdb.arch) for stepping and putting breakpoints on those instructions that you add support for. See superficial comments below. Simon On 10/12/22 14:02, Dragan Mladjenovic wrote: > @@ -1633,6 +1660,70 @@ is_octeon_bbit_op (int op, struct gdbarch *gdbarch) > return 0; > } > > +/* Return true if addition produces 32-bit overflow. */ > + > +static bool > +is_add32bit_overflow (int32_t a, int32_t b) > +{ > + int32_t r = (uint32_t) a + (uint32_t) b; > + return (a < 0 && b < 0 && r >= 0) || (a >= 0 && b >= 0 && r < 0); > +} > + > +/* Return true if addition produces 32-bit overflow or > + one of the inputs is not sign-extended 32-bit value. */ > + > +static bool > +is_add64bit_overflow (int64_t a, int64_t b) > +{ > + if (a != (int32_t) a) > + return 1; > + if (b != (int32_t) b) > + return 1; return true > @@ -1704,7 +1801,119 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > else > pc += 8; /* After the delay slot. */ > } > + else if (is_mipsr6_isa (gdbarch)) > + { > + /* BOVC, BEQZALC, BEQC and BNVC, BNEZALC, BNEC */ > + if (op == 8 || op == 24) > + { > + int rs = rtype_rs (inst); > + int rt = rtype_rt (inst); > + LONGEST val_rs = regcache_raw_get_signed (regcache, rs); > + LONGEST val_rt = regcache_raw_get_signed (regcache, rt); > + bool taken = false; > + /* BOVC (BNVC) */ > + if (rs >= rt) > + { > + if (mips64bitreg) > + taken = is_add64bit_overflow (val_rs, val_rt); > + else > + taken = is_add32bit_overflow (val_rs, val_rt); > + } > + else if (rs < rt && rs == 0) > + /* BEQZALC (BNEZALC) */ > + taken = (val_rt == 0); Because of the comment, you would need to add braces here. Ref: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Whitespaces So we would typically write: else if (rs < rt && rs == 0) { /* BEQZALC (BNEZALC) */ taken = (val_rt == 0); } But I realize this may clash with the existing style used in the file, which deviates a bit from the rest of GDB. So I'm also fine with leaving it like that on the account of following surrounding style. > @@ -2448,6 +2659,72 @@ micromips_instruction_is_compact_branch (unsigned short insn) > } > } > > +/* Return non-zero if the MIPS instruction INSN is a compact branch > + or jump. A value of 1 indicates an unconditional compact branch > + and a value of 2 indicates a conditional compact branch. */ > + > +static int > +mips32_instruction_is_compact_branch (struct gdbarch *gdbarch, ULONGEST insn) Instead of hardcoded integer values, can you change this function to make it return an enum? Simon