From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11686 invoked by alias); 1 Jun 2010 22:04:56 -0000 Received: (qmail 11673 invoked by uid 22791); 1 Jun 2010 22:04:55 -0000 X-SWARE-Spam-Status: No, hits=-0.3 required=5.0 tests=AWL,BAYES_50,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,TW_BJ X-Spam-Check-By: sourceware.org Received: from mail-wy0-f169.google.com (HELO mail-wy0-f169.google.com) (74.125.82.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 01 Jun 2010 22:04:44 +0000 Received: by wyb39 with SMTP id 39so2366699wyb.0 for ; Tue, 01 Jun 2010 15:04:41 -0700 (PDT) Received: by 10.227.143.198 with SMTP id w6mr6545701wbu.34.1275429881056; Tue, 01 Jun 2010 15:04:41 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id y31sm53115759wby.16.2010.06.01.15.04.37 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 01 Jun 2010 15:04:38 -0700 (PDT) From: Richard Sandiford To: "Maciej W. Rozycki" Mail-Followup-To: "Maciej W. Rozycki" ,binutils@sourceware.org, Chao-ying Fu , Catherine Moore , "Joseph S. Myers" , Daniel Jacobowitz , rdsandiford@googlemail.com Cc: binutils@sourceware.org, Chao-ying Fu , Catherine Moore , "Joseph S. Myers" , Daniel Jacobowitz Subject: Re: [PATCH] MIPS: microMIPS and MCU ASE instruction set support References: <87y6fa9u3t.fsf@firetop.home> Date: Tue, 01 Jun 2010 22:04:00 -0000 In-Reply-To: (Maciej W. Rozycki's message of "Tue, 1 Jun 2010 15:19:24 +0100 (BST)") Message-ID: <87typmo1e6.fsf@firetop.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2010-06/txt/msg00018.txt.bz2 "Maciej W. Rozycki" writes: >> Generally looks good. It would have been better to submit the >> ACE, m14kc and microMIPS support as three separate changes though. >> Please can you do that for the follow-up? > > I have done it now; the next version will comprise microMIPS ASE changes > only (the MCU ASE adds to both the MIPS ISA and the microMIPS ASE, so it > should be applied on top of the microMIPS change). Thanks. Thanks too for all the issues that you've addressed already, and for the ones you said you'd sort out. Sounds like good progress! > Stricly speaking some changes, while related, can be split off too (e.g. > some MIPS16 or testsuite changes), so I'll look into separating them too > -- perhaps that'll make the thing rolling sooner. Yeah, thanks, that'd be a help if it's easy to do. It doesn't matter too much, though. Most of the MIPS16 changes were consistent with the microMIPS ones, so were easy to review as a unit. >> From a usability perspective, it's a shame that: >> >> .set micromips >> .ent foo >> foo: >> b 1f >> nop >> .end foo >> .ent bar >> bar: >> 1: nop >> .end bar >> >> disassembles as: >> >> 00000000 : >> 0: cfff b 0 >> 0: R_MICROMIPS_PC10_S1 .L11 >> 2: 0c00 nop >> 4: 0c00 nop >> >> 00000006 : >> 6: 0c00 nop >> >> leaving the poor user with no idea what .L11 is. > > Indeed. This is a general limitation of `objdump' it would seem. This > is no different to what you get with: > > $ cat b.s > .globl baz > .ent foo > foo: > b baz > nop > .end foo > .ent bar > baz: > bar: > 1: nop > .end bar > $ mips-sde-elf-objdump -dr b.o > > b.o: file format elf32-tradbigmips > > > Disassembly of section .text: > > 00000000 : > 0: 1000ffff b 0 > 0: R_MIPS_PC16 baz > 4: 00000000 nop > 8: 00000000 nop > > 0000000c : > c: 00000000 nop Well, it's a little different. The user has at least defined two named symbols in this case, so they have a good chance of knowing what "baz" means. In the microMIPS case we've invented a label and are using it in preference to the user-defined one. > I'd just recommend peeking at the symbol table (back to the first > program): > > $ mips-sde-elf-objdump -t b.o > > b.o: file format elf32-tradbigmips > > SYMBOL TABLE: > 00000000 l d .text 00000000 .text > 00000000 l d .data 00000000 .data > 00000000 l d .bss 00000000 .bss > 00000000 l d .reginfo 00000000 .reginfo > 00000000 l d .pdr 00000000 .pdr > 00000000 l F .text 00000006 0x80 foo > 00000006 l F .text 00000002 0x80 bar > 00000006 l .text 00000000 0x80 .L11 I suppose having a symbol with ^B in it is less than ideal too. AIUI that name was chosen specifically because it wasn't supposed to be written out. It would be especially confusing if the user or compiler had a ".L11" label (without the ^B). >> The following: >> >> .set micromips >> .ent foo >> foo: >> ld $10,0x1000($11) >> .end foo >> >> generates an assertion failure: >> >> Assertion failure in micromips_macro_build at gas/config/tc-mips.c line 19466. >> Please report this bug. >> >> on mipsisa64-elf with "-mips1 -mabi=32". > > I can't reproduce it, sorry: > [...] > Can you debug it and see what the relocation type is that's causing it? > I wonder if that might be related to the varargs issue you referring to > below and depend on the host architecture, hmm... Yeah, you're right, sorry. I forgot to mention that in the later reviews. This was with a x86_64-linux-gnu host and a botched attempt at working around the varags issue. (I probably just added -Wno-error or something, I can't remember now.) I switched to a 32-bit host for parts 2 and 3 of the review, and yeah, it doesn't reproduce there. >> > gas/ >> > 2010-05-18 Chao-ying Fu >> > Maciej W. Rozycki >> > Daniel Jacobowitz >> > >> > * config/tc-mips.h (mips_segment_info): Add one bit for >> > microMIPS. >> > * config/tc-mips.c >> >> How about having something like: >> >> #define OOD_TEXT_LABELS (mips_opts.mips16 || mips_opts.micromips) >> >> ? > > It sounds reasonable to me, except that condition is used in some other > contexts as well. I have made it HAVE_CODE_COMPRESSION thus and took the > opportunity to optimise code around mips16_micromips_mark_labels() too. > Finally I have renamed the function to mips_compressed_mark_labels() for > as the other sounds too complicated to me. All these changes sound good, thanks. >> > (append_insn): Handle microMIPS. >> >> + if (mips_opts.micromips) >> + { >> + if ((prev_pinfo2 & INSN2_BRANCH_DELAY_16BIT) >> + && !is_micromips_16bit_p (ip->insn_mo)) >> + as_warn (_("instruction with wrong size in a branch delay slot that" >> + " requires a 16-bit instruction")); >> + if ((prev_pinfo2 & INSN2_BRANCH_DELAY_32BIT) >> + && !is_micromips_32bit_p (ip->insn_mo)) >> + as_warn (_("instruction with wrong size in a branch delay slot that" >> + " requires a 32-bit instruction")); >> + } >> + >> >> Although not enforced as often as it should be, GAS convention is for >> errors to start with a capital letter. > > I'll be fixing these up as I come across them. If to be effective, then > upstream HEAD should be fixed up or otherwise people have no way not to > get confused. I didn't know of this rule for one. That shouldn't be a > lot effort, should it? Nope. It's just a question of time and priorities. ;-) > Some have to stay for now actually, because .l testsuite patterns are > commonly shared between standard MIPS and microMIPS tests, and should be > fixed separately. OK, that's fine. >> + if (pinfo & INSN_COP) >> + { >> + /* We don't keep enough information to sort these cases out. >> + The itbl support does keep this information however, although >> + we currently don't support itbl fprmats as part of the cop >> + instruction. May want to add this support in the future. */ >> + } >> >> Assert? > > Well, that's no different to the standard MIPS variant. OK, fair enough, but I suppose I was judging the new code on its merits as new code. Sticking to existing practice is an easier sell if the code that implements it is being reused rather than copied... >> +do_lsb: >> >> Not properly indented. A few other instances. > > Like the respective originals in mips_ip(). I have fixed up the new > labels, but upstream HEAD code should be adjusted the same way. Thanks. Yeah, sorting out the new code is all that's needed. As before, I was judging the code on its own merits rather than checking whether each dubious bit was new or from cut-&-paste. It'll be a while before I have chance to go through the update properly, but it looks good at first glance. Richard