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 ESMTPS id CE51A3857C55 for ; Mon, 13 Feb 2023 12:07:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CE51A3857C55 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676290079; 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: in-reply-to:in-reply-to:references:references; bh=5eOBJt69uqLfbnwaYepeU3Z+bdHi4LFVeZpzgdV3yb0=; b=FuCDSGONgQum2pPCIaBlgKaI0iwCsRiapikLJfOHc3EAXGrErmh55HwdRx6fIh4GRCM4cy rZbxCBYZlIgO5vNQLQX5n5oGgu9zE8wo7Xz3LJV68WKTHdJhjSAlJYiyGOjuYhHf/P4n37 LmWzc7btIqY9pC4Ho/kE9uutFGSkKIc= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-182-6A32Jx3rPU6WzsK0F_xmTg-1; Mon, 13 Feb 2023 07:07:58 -0500 X-MC-Unique: 6A32Jx3rPU6WzsK0F_xmTg-1 Received: by mail-qk1-f199.google.com with SMTP id g22-20020a05620a13d600b00726e7ad3f44so7269078qkl.8 for ; Mon, 13 Feb 2023 04:07:58 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=5eOBJt69uqLfbnwaYepeU3Z+bdHi4LFVeZpzgdV3yb0=; b=uIJsOHQlF95YzC7O/mdf9QK0Z7hGtA+fFOoTVy+B2qJQ1+MyzYYefsGu6yGjiri3Ct 3kUBHXaL+liUfe5VrIVlZqQ2OCeI8YDrOTT31+zBjctdTOMsQC1eHbtlrxM5Xaqek8MS eb5EMZBcLBvr1L3cGrus/5t1llqkgQF5QVpoCA87C3TSL/WkrKydGo/jELpBtjcVs8ik 4hOD77yDN8vw1Kup4Etoy+PnsROLKCCHp2k+Yr+n52bLjsY1WIfj+4M+cspIrJ9LETJi JesqKGECZQa95HKpeXD6ebDUrK34b/zXY45uLz/fKHLIHXLlELUDNuOu0k+BvyYtf8Lu BncA== X-Gm-Message-State: AO0yUKWPcKMCNE9T0/1d7I8NzjKt80ZOv486JjPENF5czoP2PHWSY12n LhhxRI1qxT3TWeDfi/O4mu+o8d7+koQMdv7G8QCGFEsnEpNv7qNal/z9uGWgUywCIBYGLNzOBbY W7gMEA3Wzak7W/JAR+rcd8xY= X-Received: by 2002:a05:622a:5ce:b0:3b7:ec70:30af with SMTP id d14-20020a05622a05ce00b003b7ec7030afmr44295111qtb.46.1676290077790; Mon, 13 Feb 2023 04:07:57 -0800 (PST) X-Google-Smtp-Source: AK7set+nd+jyNq4FHfkGbyd1R2Usi9btuZEksYFMoGSQ4T8DLna4//LnTq+xJHKqzeA83RFk6CloHg== X-Received: by 2002:a05:622a:5ce:b0:3b7:ec70:30af with SMTP id d14-20020a05622a05ce00b003b7ec7030afmr44295070qtb.46.1676290077454; Mon, 13 Feb 2023 04:07:57 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id q64-20020ae9dc43000000b0071ba3799334sm9641131qkf.58.2023.02.13.04.07.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Feb 2023 04:07:57 -0800 (PST) From: Andrew Burgess To: "Maciej W. Rozycki" Cc: binutils@sourceware.org Subject: Re: [PATCH 1/2] opcodes/mips: use .word/.short for undefined instructions In-Reply-To: <87ilgjrt7r.fsf@redhat.com> References: <87fscny5tr.fsf@redhat.com> <87a62hfoa5.fsf@redhat.com> <87o7qguzzx.fsf@redhat.com> <87ilgjrt7r.fsf@redhat.com> Date: Mon, 13 Feb 2023 12:07:55 +0000 Message-ID: <87o7pxu5t0.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 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_H2,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: Andrew Burgess writes: > "Maciej W. Rozycki" writes: > >> Andrew, >> >>> I've updated the patch. Let me know what you think. >> >> I'd say it's OK, except that I put your change through my MIPS regression >> tester and that revealed failures from your new case for numerous targets, >> e.g.: >> >> mips-elf +FAIL: microMIPS source file contains reserved encoding (o32) >> mips-img-elf +FAIL: microMIPS source file contains reserved encoding (o32) >> mips-img-elf +FAIL: microMIPS source file contains reserved encoding (n32) >> >> etc. The usual suspect is section padding owing to different alignments >> used with individual MIPS targets, e.g.: >> >> extra regexps in >> .../binutils/testsuite/binutils-all/mips/micromips-reserved-enc-o32.d starting with "^ >> \.\.\.$" >> EOF from tmpdir/dump.out >> FAIL: microMIPS source file contains reserved encoding (o32) >> >> See e.g. binutils/testsuite/binutils-all/mips/micromips-branch-alias.s for >> how to add suitable padding at the end. >> >> And then: >> >> mipsisa32r6-elf +FAIL: microMIPS source file contains reserved encoding (o32) >> mipsisa32r6-linux +FAIL: microMIPS source file contains reserved encoding (o32) >> mipsisa32r6el-elf +FAIL: microMIPS source file contains reserved encoding (o32) >> >> etc., due to: >> >> .../binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s: Assembler messages: >> .../binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s:3: Fatal error: `micromips' cannot be used with `mips32r6' >> >> We don't care about different ISA levels here, so let's set a reasonable >> one, as in binutils/testsuite/binutils-all/mips/micromips-branch-alias.s >> again: >> >> .module mips64r3 >> >> (it could be `.set' too, but let's be consistent, and it has to be a >> 64-bit one for the n32/n64 ABIs). >> >> OK with these updates, thank you for your contribution. > > Right, I think I've got it this time :) > > Updated patch is below. Given the updates are minor, if I don't hear > back from you I'll push this sometime next week. > > I tested this against all the targets you listed, the only place this > test still fails is for `mipsisa64sr71k-elf`, but that build looks > pretty broken anyway. The failure is in gas, during argument parsing, > and I see the same failure for many of the tests, so I don't think I > need to worry about that. Everything else looks like a PASS or > UNSUPPORTED, which I think is fine. > > Thanks for all your feedback on this one. > > Andrew > > --- > > commit c84e242a7a45787c9ff60a3cf06b3b7f30d85970 > Author: Andrew Burgess > Date: Fri Jan 6 16:42:23 2023 +0000 > > opcodes/mips: disassemble unknown micromips instructions as two shorts > > Before commit: > > commit 2438b771ee07be19d5b01ea55e077dd8b7cef445 > Date: Wed Nov 2 15:53:43 2022 +0000 > > opcodes/mips: use .word/.short for undefined instructions > > unknown 32-bit microMIPS instructions were disassembled as a raw > 32-bit number with no '.word' directive. The above commit changed > this and added a '.word' directive before the 32-bit number. > > It was pointed out on the mailing list, that for microMIPS it would be > better to display such 32-bit instructions using a '.short' directive > followed by two 16-bit values. > > This commit updates the mips disassembler to do this, and adds a new > test that validates this output. Given the previous version was given the OK once the minor fixes were merged, I've gone ahead and pushed this patch. Do let me know if there are any further problems in this area. Thanks, Andrew > > diff --git a/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n32.d b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n32.d > new file mode 100644 > index 00000000000..e6608f30265 > --- /dev/null > +++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n32.d > @@ -0,0 +1,5 @@ > +#PROG: objcopy > +#objdump: -d --prefix-addresses --show-raw-insn > +#name: microMIPS source file contains reserved encoding (n32) > +#source: micromips-reserved-enc.s > +#dump: micromips-reserved-enc-o32.d > diff --git a/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n64.d b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n64.d > new file mode 100644 > index 00000000000..f892bfabbe7 > --- /dev/null > +++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n64.d > @@ -0,0 +1,5 @@ > +#PROG: objcopy > +#objdump: -d --prefix-addresses --show-raw-insn > +#name: microMIPS source file contains reserved encoding (n64) > +#source: micromips-reserved-enc.s > +#dump: micromips-reserved-enc-o32.d > diff --git a/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-o32.d b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-o32.d > new file mode 100644 > index 00000000000..3de3989b37a > --- /dev/null > +++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-o32.d > @@ -0,0 +1,10 @@ > +#PROG: objcopy > +#objdump: -d --prefix-addresses --show-raw-insn > +#name: microMIPS source file contains reserved encoding (o32) > +#source: micromips-reserved-enc.s > + > +.*: +file format .*mips.* > + > +Disassembly of section \.text: > +[0-9a-f]+ <[^>]*> 7f6e 5d4c \.short 0x7f6e, 0x5d4c > + \.\.\. > diff --git a/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s > new file mode 100644 > index 00000000000..d4918f36857 > --- /dev/null > +++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s > @@ -0,0 +1,9 @@ > + .module mips64r3 > + .module micromips > +foo: > + .insn > + .short 0x7f6e, 0x5d4c > + > +# Force some (non-delay-slot) zero bytes, to make 'objdump' print ... > + .align 4, 0 > + .space 16 > diff --git a/binutils/testsuite/binutils-all/mips/mips.exp b/binutils/testsuite/binutils-all/mips/mips.exp > index 6a0ec25a06f..91bf3274592 100644 > --- a/binutils/testsuite/binutils-all/mips/mips.exp > +++ b/binutils/testsuite/binutils-all/mips/mips.exp > @@ -266,3 +266,7 @@ run_dump_test_n64 "global-local-symtab-sort-n64${tmips}" > run_dump_test_o32 "global-local-symtab-final-o32" useld > run_dump_test_n32 "global-local-symtab-final-n32" useld > run_dump_test_n64 "global-local-symtab-final-n64" useld > + > +run_dump_test_o32 "micromips-reserved-enc-o32" > +run_dump_test_n32 "micromips-reserved-enc-n32" > +run_dump_test_n64 "micromips-reserved-enc-n64" > diff --git a/opcodes/mips-dis.c b/opcodes/mips-dis.c > index 6a513cd8946..859d4e3806f 100644 > --- a/opcodes/mips-dis.c > +++ b/opcodes/mips-dis.c > @@ -2600,12 +2600,15 @@ print_insn_micromips (bfd_vma memaddr, struct disassemble_info *info) > } > } > > - if (length == 2) > - infprintf (is, dis_style_assembler_directive, ".short"); > - else > - infprintf (is, dis_style_assembler_directive, ".word"); > + infprintf (is, dis_style_assembler_directive, ".short"); > infprintf (is, dis_style_text, "\t"); > - infprintf (is, dis_style_immediate, "0x%x", insn); > + if (length != 2) > + { > + infprintf (is, dis_style_immediate, "0x%x", (insn >> 16) & 0xffff); > + infprintf (is, dis_style_text, ", "); > + } > + infprintf (is, dis_style_immediate, "0x%x", (insn & 0xffff)); > + > info->insn_type = dis_noninsn; > > return length;