From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id w/llJH6TfWeUbQYAWB0awg (envelope-from ) for ; Tue, 07 Jan 2025 15:50:06 -0500 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=ih4lUIiP; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 860291E0C0; Tue, 7 Jan 2025 15:50:06 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=4.0.0 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 9054E1E05C for ; Tue, 7 Jan 2025 15:50:05 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 093F73858416 for ; Tue, 7 Jan 2025 20:50:05 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 093F73858416 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=ih4lUIiP Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id 3999E385802C for ; Tue, 7 Jan 2025 20:49:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3999E385802C 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 3999E385802C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736282971; cv=none; b=vNOguWG0sPLhYc8PVAAKXB0n98njav19er2UfrVrp0GhYdfF+odxWsW2U7aG0dCwd2fHCAfoAxwHzOxLHzt+8tBZ0r6Y1G2CneuvgQALudowu0JdCT0xjWcVhgfBpOUCc5XU0rNg/ChDW0KQcMWckiQuYc/3HSi6u8H6c2H5CXs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736282971; c=relaxed/simple; bh=m03RB3tIsuSx47zgX64ia4e2H1rGBh/ZZHQT9aqjhTQ=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=A3MErf0blEqt6BJchPftTkk1Lm5ve5UBtI5oIxj+R6z47ucpA3a3Pnk9jJsHUq4+aCJ0Px+PuAHXx+K/7PgD+zyIbtlU+ZDoRhv5LDTV0rETTB4HpHBUXOGtGyjNQphfYM1wJOaaFkdrMx4KvSj/VCiuJKBwG4beLy8CXtfWdVM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3999E385802C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736282970; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mVJceCzmpTnsmcP1IwGjJFvCFddJZkl4m5elFK4wmyc=; b=ih4lUIiP1EpkYEof8NvrVasObMkfpP+/aGZxnMycy9cDJ6tTQpY++G46JX9rfWjgT7RA6O wRw7bETz+8PWnoo/eIRfVnuJspz13eA/YapC1SzVBvyJ6KcQasNCsw/wEycCH/U/1vf6h4 OLIVJL14bsqTPd5s+KUf5R5XyxgNKRM= Received: from mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-621-IUq6Bn1yN8GJOt7avlxKQg-1; Tue, 07 Jan 2025 15:49:27 -0500 X-MC-Unique: IUq6Bn1yN8GJOt7avlxKQg-1 X-Mimecast-MFC-AGG-ID: IUq6Bn1yN8GJOt7avlxKQg Received: from mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.40]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 975E419560AA; Tue, 7 Jan 2025 20:49:26 +0000 (UTC) Received: from f41-zbm-amd (unknown [10.22.88.112]) by mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id C114D195606C; Tue, 7 Jan 2025 20:49:25 +0000 (UTC) Date: Tue, 7 Jan 2025 13:49:21 -0700 From: Kevin Buettner To: Milica Matic Cc: gdb-patches@sourceware.org Subject: Re: [PATCH^8] gdb: mips: Add MIPSR6 support Message-ID: <20250107134921.4ee64acf@f41-zbm-amd> In-Reply-To: References: Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.40 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: RcS3VXtPcJMx2jsHZ39g_YKg8nXXnd0ZpgC2cyZwNHw_1736282966 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org Hi Milica, While applying your patch locally, I saw: warning: squelched 8 whitespace errors warning: 13 lines add whitespace errors. I suggest doing a "git am" using the patch that you sent to the list to identify them. Additional comments inline, below... On Fri, 13 Dec 2024 20:29:05 +0000 Milica Matic wrote: > +/* Calculate address of next instruction after BLEZ. */ > + > +static CORE_ADDR > +mips32_blez_pc (struct gdbarch *gdbarch, struct regcache *regcache, > + ULONGEST inst, CORE_ADDR pc, int invert) > +{ > + int rs = itype_rs (inst); > + int rt = itype_rt (inst); > + LONGEST val_rs = regcache_raw_get_signed (regcache, rs); > + LONGEST val_rt = regcache_raw_get_signed (regcache, rt); > + ULONGEST uval_rs = regcache_raw_get_unsigned (regcache, rs); > + ULONGEST uval_rt = regcache_raw_get_unsigned (regcache, rt); > + int taken = 0; > + int delay_slot_size = 4; > + > + /* BLEZ, BLEZL, BGTZ, BGTZL */ > + if (rt == 0) > + taken = (val_rs <= 0); > + else if (is_mipsr6_isa (gdbarch)) > + { > + /* BLEZALC, BGTZALC */ > + if (rs == 0 && rt != 0) > + taken = (val_rt <= 0); > + /* BGEZALC, BLTZALC */ > + else if (rs == rt && rt != 0) > + taken = (val_rt >= 0); > + /* BGEUC, BLTUC */ > + else if (rs != rt && rs != 0 && rt != 0) > + taken = (uval_rs >= uval_rt); > + > + /* Step through the forbidden slot to avoid repeated exceptions we do > + not currently have access to the BD bit when hitting a breakpoint > + and therefore cannot tell if the breakpoint hit on the branch or the > + forbidden slot. */ > + /* delay_slot_size = 0; */ Should "delay_slot_size = 0;" be commented out? If so, I suggest removing the entire line. And, if it's removed, the comment preceding it probably doesn't make sense either. > + } > + > + if (invert) > + taken = !taken; > + > + /* Calculate branch target. */ > + if (taken) > + pc += mips32_relative_offset (inst); > + else > + pc += delay_slot_size; > + > + return pc; > +} > > /* Determine where to set a single step breakpoint while considering > branch prediction. */ > @@ -1642,58 +1751,66 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > struct gdbarch *gdbarch = regcache->arch (); > unsigned long inst; > int op; > + int mips64bitreg = 0; > + > + if (mips_isa_regsize (gdbarch) == 8) > + mips64bitreg = 1; > + > inst = mips_fetch_instruction (gdbarch, ISA_MIPS, pc, NULL); > op = itype_op (inst); > - if ((inst & 0xe0000000) != 0) /* Not a special, jump or branch > - instruction. */ > + if ((inst & 0xe0000000) != 0) /* Not a special, jump or branch instruction. */ > + { > + if (op >> 2 == 5 && ((op & 0x02) == 0 || itype_rt (inst) == 0)) > + /* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */ > { > - if (op >> 2 == 5) > - /* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */ > + switch (op & 0x03) > { > - switch (op & 0x03) > - { > - case 0: /* BEQL */ > - goto equal_branch; > - case 1: /* BNEL */ > - goto neq_branch; > - case 2: /* BLEZL */ > - goto less_branch; > - case 3: /* BGTZL */ > - goto greater_branch; > - default: > - pc += 4; > - } > + case 0: /* BEQL */ > + goto equal_branch; > + case 1: /* BNEL */ > + goto neq_branch; > + case 2: /* BLEZL */ > + goto lez_branch; > + case 3: /* BGTZL */ > + goto greater_branch; > + default: > + pc += 4; > } > + } The indentation here somehow got messed up here. After applying your patch, I see: if ((inst & 0xe0000000) != 0) /* Not a special, jump or branch instruction. */ { if (op >> 2 == 5 && ((op & 0x02) == 0 || itype_rt (inst) == 0)) /* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */ { switch (op & 0x03) { case 0: /* BEQL */ goto equal_branch; case 1: /* BNEL */ goto neq_branch; case 2: /* BLEZL */ goto lez_branch; case 3: /* BGTZL */ goto greater_branch; default: pc += 4; } } Note that the second '{' and the block that goes along with it are not correctly indented for the 'if' statement just above it. I believe that there are further indentation problems later on too; I'll point out a few of them, but I'll leave it to you to find and fix the rest. [...] > @@ -1763,22 +2000,38 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > pc += 8; /* after the delay slot */ > break; > case 0x1c: /* BPOSGE32 */ > + case 0x1d: /* BPOSGE32C */ Indentation is off by one space here. This is what I see after applying your patch: case 0x1c: /* BPOSGE32 */ case 0x1d: /* BPOSGE32C */ case 0x1e: /* BPOSGE64 */ > case 0x1e: /* BPOSGE64 */ > pc += 4; > if (itype_rs (inst) == 0) > { > unsigned int pos = (op & 2) ? 64 : 32; > int dspctl = mips_regnum (gdbarch)->dspctl; > + int delay_slot_size = 4; Likewise. (Off by one space.) > > if (dspctl == -1) > /* No way to handle; it'll most likely trap anyway. */ > break; > > + /* BPOSGE32C */ > + if (op == 0x1d) > + { > + if (!is_mipsr6_isa (gdbarch)) > + break; > + > + /* Step through the forbidden slot to avoid repeated > + exceptions we do not currently have access to the BD > + bit when hitting a breakpoint and therefore cannot > + tell if the breakpoint hit on the branch or the > + forbidden slot. */ > + /* delay_slot_size = 0; */ > + } Likewise, for the above block. > + > if ((regcache_raw_get_unsigned (regcache, > dspctl) & 0x7f) >= pos) > pc += mips32_relative_offset (inst); > else > - pc += 4; > + pc += delay_slot_size; Likewise. > } > break; > /* All of the other instructions in the REGIMM category */ > @@ -1812,19 +2065,14 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > else > pc += 8; > break; > - case 6: /* BLEZ, BLEZL */ > - if (regcache_raw_get_signed (regcache, itype_rs (inst)) <= 0) > - pc += mips32_relative_offset (inst) + 4; > - else > - pc += 8; > + case 6: /* BLEZ, BLEZL, BLEZALC, BGEZALC, BGEUC */ > + lez_branch: > + pc = mips32_blez_pc (gdbarch, regcache, inst, pc + 4, 0); Likewise. > break; > case 7: > default: > - greater_branch: /* BGTZ, BGTZL */ > - if (regcache_raw_get_signed (regcache, itype_rs (inst)) > 0) > - pc += mips32_relative_offset (inst) + 4; > - else > - pc += 8; > + greater_branch: /* BGTZ, BGTZL, BGTZALC, BLTZALC, BLTUC */ > + pc = mips32_blez_pc (gdbarch, regcache, inst, pc + 4, 1); Likewise. [...] > +/* 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) > +{ > + switch (itype_op (insn)) > + { > + /* BC */ > + case 50: > + /* BALC */ > + case 58: > + if (is_mipsr6_isa (gdbarch)) > + return 1; > + break; > + /* BOVC, BEQZALC, BEQC */ > + case 8: > + /* BNVC, BNEZALC, BNEC */ > + case 24: > + if (is_mipsr6_isa (gdbarch)) Up to this point, indentation appears correct for the new function 'mips32_instruction_is_compact_branch', but... > + return 2; It's wrong for the line above. That line is indented by 7 spaces, but it should be 8, which'll turn it into a tab. > + break; > + /* BEQZC, JIC */ > + case 54: > + /* BNEZC, JIALC */ > + case 62: > + if (is_mipsr6_isa (gdbarch)) > + /* JIC, JIALC are unconditional */ > + return (itype_rs (insn) == 0) ? 1 : 2; Likewise for the two lines above. (7 spaces are used instead of 8 which converts to a tab.) > + break; > + /* BLEZC, BGEZC, BGEC */ > + case 22: > + /* BGTZC, BLTZC, BLTC */ > + case 23: > + /* BLEZALC, BGEZALC, BGEUC */ > + case 6: > + /* BGTZALC, BLTZALC, BLTUC */ > + case 7: > + if (is_mipsr6_isa (gdbarch) > + && itype_rt (insn) != 0) > + return 2; Likewise for the "return 2;" line. > + break; > + /* BPOSGE32C */ > + case 1: > + if (is_mipsr6_isa (gdbarch) > + && itype_rt (insn) == 0x1d && itype_rs (insn) == 0) > + return 2; And here too. [...] > static CORE_ADDR > @@ -3490,7 +3800,8 @@ mips32_scan_prologue (struct gdbarch *gdbarch, > reg = high_word & 0x1f; > > if (high_word == 0x27bd /* addiu $sp,$sp,-i */ > - || high_word == 0x23bd /* addi $sp,$sp,-i */ > + || (high_word == 0x23bd /* addi $sp,$sp,-i */ > + && !is_mipsr6_isa (gdbarch)) > || high_word == 0x67bd) /* daddiu $sp,$sp,-i */ Indentation problem here too. The '||' line that you added should have the same indentation level as the '|| high_word == 0x67bd) /* daddiu $sp,$sp,-i */' line. Okay, so I'm going to stop here. Scanning ahead, I saw other indentation problems which are similar to those mentioned above. I'll leave it to you to find and fix them... I did notice that you fixed some of the existing whitespace problems in this file, including removal of extraneous blank lines and spaces at the end of a line. Thanks for doing that! Kevin