From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26935 invoked by alias); 4 Jul 2011 19:35:23 -0000 Received: (qmail 26527 invoked by uid 22791); 4 Jul 2011 19:35:22 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 04 Jul 2011 19:35:04 +0000 Received: (qmail 24189 invoked from network); 4 Jul 2011 19:35:03 -0000 Received: from unknown (HELO tp.orcam.me.uk) (macro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 4 Jul 2011 19:35:03 -0000 Date: Mon, 04 Jul 2011 20:30:00 -0000 From: "Maciej W. Rozycki" To: Richard Sandiford cc: binutils@sourceware.org Subject: Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code In-Reply-To: <87boxdcf2b.fsf@firetop.home> Message-ID: References: <87boxg5nyo.fsf@firetop.home> <87boxdcf2b.fsf@firetop.home> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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: 2011-07/txt/msg00043.txt.bz2 On Sat, 2 Jul 2011, Richard Sandiford wrote: > > Hmm, does it? I did verify it both on a GNU/Linux and an ELF target. It > > still passes for me, except that: > > > > + Advance PC by 24 to 0x64 > > > > is required instead as with my intended-to-be-sent-originally change. > > Likewise: > > > > + Advance PC by 23 to 0x40 > > > > for the MIPS16 dump. Which target is failing for you? Perhaps the cause > > was merely the outdated version of the change, sigh... > > Ah, could be. It was mips64-elf. Maybe I'd got confused and the > original didn't pass for GNU/Linux after all. Sorry about that. I have checked mips64-elf to be sure and this test case passes in its updated form (there are some unrelated failures for this target that shouldn't happen though). > > I have regenerated and revalidated the change now and I'm ready to push > > it, but I'd prefer the final DWARF-2 instructions to be matched exactly. > > I made the effort to make the source alignment-agnostic and I'd prefer to > > keep the final offsets to be verified literally so that any unexpected > > discrepancies are caught. > > Agreed. I'm glad to hear this for once. ;) > > 2011-07-02 Maciej W. Rozycki > > > > gas/ > > * config/tc-mips.c (append_insn): Make sure DWARF-2 location > > information is properly adjusted for branches that get swapped. > > > > gas/testsuite/ > > * gas/mips/loc-swap.d: New test case for DWARF-2 location with > > branch swapping. > > * gas/mips/loc-swap-dis.d: Likewise. > > * gas/mips/mips16\@loc-swap.d: Likewise, MIPS16 version. > > * gas/mips/mips16\@loc-swap-dis.d: Likewise. > > * gas/mips/loc-swap.s: Source for the new tests. > > * gas/mips/mips.exp: Run the new tests. > > OK. Applied now, thanks. BTW, I've been wondering about this construct: return mask & ~0; you've introduced in a couple of places. The masking operation seems a no-op to me -- given that both mask and the function's return type are defined as unsigned int. Am I missing anything? Also why "can not" instead of "cannot"? My understanding of the subtleties of English is this essentially means we "are allowed not to swap" rather than we "are not allowed to swap". The usual disclaimer applies of course. ;) Finally I think the change below is needed as well. We shouldn't be relying on the meaning of FP_D for MIPS16 code as opcode flags are assumed to have a different meaning in the MIPS16 table. Granted, the location of the FP_D bit hasn't been assigned to anything (yet), so it's guaranteed to be zero and the check is harmless, but I think we better avoided this kind of overloading. Do you agree? Alternatively, given that there's no FP support in the MIPS16 mode, the two functions could simply return straight away in that case and then let the rest of the bodies to stay outside a conditional block. 2011-07-04 Maciej W. Rozycki gas/ * config/tc-mips.c (fpr_read_mask, fpr_write_mask): Avoid the FP_D check in MIPS16 mode. Maciej binutils-gas-mips-fp-d.diff Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c =================================================================== --- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c 2011-07-02 19:30:04.000000000 +0100 +++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c 2011-07-02 19:32:22.000000000 +0100 @@ -2519,7 +2519,7 @@ fpr_read_mask (const struct mips_cl_insn } /* Conservatively treat all operands to an FP_D instruction are doubles. (This is overly pessimistic for things like cvt.d.s.) */ - if (HAVE_32BIT_FPRS && (pinfo & FP_D)) + if (!mips_opts.mips16 && HAVE_32BIT_FPRS && (pinfo & FP_D)) mask |= mask << 1; return mask; } @@ -2548,7 +2548,7 @@ fpr_write_mask (const struct mips_cl_ins } /* Conservatively treat all operands to an FP_D instruction are doubles. (This is overly pessimistic for things like cvt.s.d.) */ - if (HAVE_32BIT_FPRS && (pinfo & FP_D)) + if (!mips_opts.mips16 && HAVE_32BIT_FPRS && (pinfo & FP_D)) mask |= mask << 1; return mask; }