* [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code
@ 2011-02-23 13:39 Maciej W. Rozycki
2011-06-29 21:26 ` Richard Sandiford
0 siblings, 1 reply; 11+ messages in thread
From: Maciej W. Rozycki @ 2011-02-23 13:39 UTC (permalink / raw)
To: binutils; +Cc: Richard Sandiford
Hi,
We have a long-standing problem with branch swapping in MIPS16 code.
Consider the following real piece, extracted from step-test.c in the GDB
test suite:
sw $2,36($17)
.loc 1 45 0
jal callee
As you can see there's DWARF-2 line information associated with the
procedure call instruction. As the SW instruction is 16-bit, GAS decides
to reorder it into the JAL's delay slot. The resulting binary code looks
like this:
60: 1800 0000 jal 0 <callee>
60: R_MIPS16_26 callee
64: d940 sw v0,0(s1)
but line #45 is recorded at its instruction's original location before the
swapping took place i.e. 0x62. When such a program is being debugged and
a breakpoint is requested at line #45, then the debugger faithfully places
it at 0x62, targetting the second half of the JAL instruction and when
control reaches this address, then the breakpoint is either missed (if
hardware breakpoints are used, either explicitly or by the debug stub,
because no address match happens) or the program goes astray (if software
breakpoints are used, because JAL's immediate argument has been corrupted
by the breakpoint instruction).
Many years ago I developed a simple workaround for this issue that
disabled branch swapping for MIPS16 code altogether. Now when I got back
to it because of the microMIPS effort I have decided the old workaround is
too lame to keep it alive any longer. With some shuffling of code within
append_insn() I have now made the function produce DWARF-2 line
information modified such that if a branch is swapped, then the
corresponding location record is updated to point to the new location of
the branch.
This change has been successfully regression-tested for the mips-sde-elf
and mips-linux-gnu targets and also fixes several mips-linux-gnu MIPS16
GDB test suite failures, e.g.:
(gdb) PASS: gdb.base/auxv.exp: tbreak 64
continue
Continuing.
-Program received signal SIGSEGV, Segmentation fault.
-0x0043a294 in ?? ()
-(gdb) FAIL: gdb.base/auxv.exp: continue
+Temporary breakpoint 2, func2 (x=20) at [...]/gdb/testsuite/gdb.base/auxv.c:64
+64 ABORT;
+(gdb) PASS: gdb.base/auxv.exp: continue
info auxv
16 AT_HWCAP Machine-dependent CPU capability hints 0x0
6 AT_PAGESZ System page size 4096
Notice that (0x0043a294 >> 2) & 0xffff is 0xe8a5 which is the encoding of
the MIPS16 BREAK 5 instruction.
Some GDB regressions from the simple workaround still remain, because
some test cases rely on stopping between a given instruction and the
following branch. That unfortunately may never happen if the two
instructions have been swapped, no matter what we do about line
information, so these failures have to stay for the time being as
shortcomings of the respective test cases. Or actually whenever the -g
compilation option is used, then GCC should either use the noreorder
assembly mode and schedule delay slots itself such that line boundaries
are preserved or otherwise supply -O1 to GAS to override its default of
-O2 and disable branch swapping, at least at GCC's -O0 optimisation level
(this has a potential drawback of the toolchain producing different code
depending on whether -g has or hasn't been specified, so perhaps -O1
should be passed to GAS unconditionally whenever -O0 has been selected for
GCC, hmm...).
While at it, I think this DWARF-2 line information is not correctly
recorded for code generated as a result of fix_loongson2f_jump() being
triggered. I think the call to fix_loongson2f_jump() should be moved to
after the call to dwarf2_emit_insn(). This observation applies regardless
of the change considered here.
2011-02-23 Maciej W. Rozycki <macro@codesourcery.com>
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 to apply?
Maciej
binutils-gas-mips-branch-swap-dwarf2.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c 2011-02-22 16:49:48.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c 2011-02-22 21:59:08.000000000 +0000
@@ -2814,9 +2814,10 @@ append_insn (struct mips_cl_insn *ip, ex
{
unsigned long prev_pinfo, pinfo;
unsigned long prev_pinfo2, pinfo2;
- relax_stateT prev_insn_frag_type = 0;
bfd_boolean relaxed_branch = FALSE;
segment_info_type *si = seg_info (now_seg);
+ bfd_boolean branch_swap = FALSE;
+ int branch_disp = 0;
if (mips_fix_loongson2f)
fix_loongson2f (ip);
@@ -2905,18 +2906,219 @@ append_insn (struct mips_cl_insn *ip, ex
}
}
+ /* Update the register mask information. */
+ if (! mips_opts.mips16)
+ {
+ if ((pinfo & INSN_WRITE_GPR_D) || (pinfo2 & INSN2_READ_GPR_D))
+ mips_gprmask |= 1 << EXTRACT_OPERAND (RD, *ip);
+ if ((pinfo & (INSN_WRITE_GPR_T | INSN_READ_GPR_T)) != 0)
+ mips_gprmask |= 1 << EXTRACT_OPERAND (RT, *ip);
+ if (pinfo & INSN_READ_GPR_S)
+ mips_gprmask |= 1 << EXTRACT_OPERAND (RS, *ip);
+ if (pinfo & INSN_WRITE_GPR_31)
+ mips_gprmask |= 1 << RA;
+ if (pinfo2 & (INSN2_WRITE_GPR_Z | INSN2_READ_GPR_Z))
+ mips_gprmask |= 1 << EXTRACT_OPERAND (RZ, *ip);
+ if (pinfo & INSN_WRITE_FPR_D)
+ mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FD, *ip);
+ if ((pinfo & (INSN_WRITE_FPR_S | INSN_READ_FPR_S)) != 0)
+ mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FS, *ip);
+ if ((pinfo & (INSN_WRITE_FPR_T | INSN_READ_FPR_T)) != 0)
+ mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FT, *ip);
+ if ((pinfo & INSN_READ_FPR_R) != 0)
+ mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FR, *ip);
+ if (pinfo2 & (INSN2_WRITE_FPR_Z | INSN2_READ_FPR_Z))
+ mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FZ, *ip);
+ 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. */
+ }
+ /* Never set the bit for $0, which is always zero. */
+ mips_gprmask &= ~1 << 0;
+ }
+ else
+ {
+ if (pinfo & (MIPS16_INSN_WRITE_X | MIPS16_INSN_READ_X))
+ mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (RX, *ip);
+ if (pinfo & (MIPS16_INSN_WRITE_Y | MIPS16_INSN_READ_Y))
+ mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (RY, *ip);
+ if (pinfo & MIPS16_INSN_WRITE_Z)
+ mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (RZ, *ip);
+ if (pinfo & (MIPS16_INSN_WRITE_T | MIPS16_INSN_READ_T))
+ mips_gprmask |= 1 << TREG;
+ if (pinfo & (MIPS16_INSN_WRITE_SP | MIPS16_INSN_READ_SP))
+ mips_gprmask |= 1 << SP;
+ if (pinfo & (MIPS16_INSN_WRITE_31 | MIPS16_INSN_READ_31))
+ mips_gprmask |= 1 << RA;
+ if (pinfo & MIPS16_INSN_WRITE_GPR_Y)
+ mips_gprmask |= 1 << MIPS16OP_EXTRACT_REG32R (ip->insn_opcode);
+ if (pinfo & MIPS16_INSN_READ_Z)
+ mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (MOVE32Z, *ip);
+ if (pinfo & MIPS16_INSN_READ_GPR_X)
+ mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (REGR32, *ip);
+ }
+
+ /* Filling the branch delay slot is more complex. We try to switch
+ the branch with the previous instruction, which we can do if the
+ previous instruction does not set up a condition that the branch
+ tests and if the branch is not itself the target of any branch.
+ We don't yet optimize branch-likely instructions though.*/
+ if (mips_relax.sequence != 2
+ && !mips_opts.noreorder
+ && ((pinfo & INSN_UNCOND_BRANCH_DELAY)
+ || (pinfo & INSN_COND_BRANCH_DELAY))
+ && mips_optimize >= 2
+ /* If we have seen .set volatile or .set nomove, don't optimize. */
+ && mips_opts.nomove == 0
+ /* We can't swap if the previous instruction's position is fixed. */
+ && !history[0].fixed_p
+ /* If the previous previous insn was in a .set noreorder, we can't
+ swap. Actually, the MIPS assembler will swap in this situation.
+ However, gcc configured -with-gnu-as will generate code like:
+
+ .set noreorder
+ lw $4,XXX
+ .set reorder
+ INSN
+ bne $4,$0,foo
+
+ in which we cannot swap the bne and INSN. If gcc is not
+ configured -with-gnu-as, it does not output the .set pseudo-ops. */
+ && !history[1].noreorder_p
+ /* If the branch is itself the target of a branch, we cannot swap.
+ We cheat on this; all we check for is whether there is a label
+ on this instruction. If there are any branches to anything other
+ than a label, users must use .set noreorder. */
+ && si->label_list == NULL
+ /* If the previous instruction is in a variant frag other than this
+ branch's one, we cannot do the swap. This does not apply to
+ MIPS16 code, which uses variant frags for different purposes. */
+ && (mips_opts.mips16 || history[0].frag->fr_type != rs_machine_dependent)
+ /* Check for conflicts between the branch and the instructions
+ before the candidate delay slot. */
+ && nops_for_insn (history + 1, ip) == 0
+ /* Check for conflicts between the swapped sequence and the target
+ of the branch. */
+ && nops_for_sequence (2, history + 1, ip, history) == 0
+ /* We do not swap with a trap instruction, since it complicates
+ trap handlers to have the trap instruction be in a delay slot. */
+ && !(prev_pinfo & INSN_TRAP)
+ /* If the branch reads a register that the previous instruction
+ sets, we cannot swap. */
+ && (mips_opts.mips16
+ || !(prev_pinfo & INSN_WRITE_GPR_T)
+ || !insn_uses_reg (ip, EXTRACT_OPERAND (RT, history[0]),
+ MIPS_GR_REG))
+ && (mips_opts.mips16
+ || !(prev_pinfo & INSN_WRITE_GPR_D)
+ || !insn_uses_reg (ip, EXTRACT_OPERAND (RD, history[0]),
+ MIPS_GR_REG))
+ && (mips_opts.mips16
+ || !(prev_pinfo2 & INSN2_WRITE_GPR_Z)
+ || !insn_uses_reg (ip, EXTRACT_OPERAND (RZ, history[0]),
+ MIPS_GR_REG))
+ && (!mips_opts.mips16
+ || !(prev_pinfo & MIPS16_INSN_WRITE_X)
+ || !insn_uses_reg (ip, MIPS16_EXTRACT_OPERAND (RX, history[0]),
+ MIPS16_REG))
+ && (!mips_opts.mips16
+ || !(prev_pinfo & MIPS16_INSN_WRITE_Y)
+ || !insn_uses_reg (ip, MIPS16_EXTRACT_OPERAND (RY, history[0]),
+ MIPS16_REG))
+ && (!mips_opts.mips16
+ || !(prev_pinfo & MIPS16_INSN_WRITE_Z)
+ || !insn_uses_reg (ip, MIPS16_EXTRACT_OPERAND (RZ, history[0]),
+ MIPS16_REG))
+ && (!mips_opts.mips16
+ || !(prev_pinfo & MIPS16_INSN_WRITE_T)
+ || !insn_uses_reg (ip, TREG, MIPS_GR_REG))
+ && (!mips_opts.mips16
+ || !(prev_pinfo & MIPS16_INSN_WRITE_31)
+ || !insn_uses_reg (ip, RA, MIPS_GR_REG))
+ && (!mips_opts.mips16
+ || !(prev_pinfo & MIPS16_INSN_WRITE_GPR_Y)
+ || !insn_uses_reg (ip,
+ MIPS16OP_EXTRACT_REG32R (history[0].insn_opcode),
+ MIPS_GR_REG))
+ /* If the branch writes a register that the previous instruction
+ sets, we cannot swap (we know that branches write only to RD
+ or to $31). */
+ && (mips_opts.mips16
+ || !(prev_pinfo & INSN_WRITE_GPR_T)
+ || ((!(pinfo & INSN_WRITE_GPR_D)
+ || (EXTRACT_OPERAND (RT, history[0])
+ != EXTRACT_OPERAND (RD, *ip)))
+ && (!(pinfo & INSN_WRITE_GPR_31)
+ || EXTRACT_OPERAND (RT, history[0]) != RA)))
+ && (mips_opts.mips16
+ || !(prev_pinfo & INSN_WRITE_GPR_D)
+ || ((!(pinfo & INSN_WRITE_GPR_D)
+ || (EXTRACT_OPERAND (RD, history[0])
+ != EXTRACT_OPERAND (RD, *ip)))
+ && (!(pinfo & INSN_WRITE_GPR_31)
+ || EXTRACT_OPERAND (RD, history[0]) != RA)))
+ && (!mips_opts.mips16
+ || !(pinfo & MIPS16_INSN_WRITE_31)
+ || (!(prev_pinfo & MIPS16_INSN_WRITE_31)
+ && (!(prev_pinfo & MIPS16_INSN_WRITE_GPR_Y)
+ || MIPS16OP_EXTRACT_REG32R (history[0].insn_opcode) != RA)))
+ /* If the branch writes a register that the previous instruction
+ reads, we cannot swap (we know that branches only write to RD
+ or to $31). */
+ && (mips_opts.mips16
+ || !(pinfo & INSN_WRITE_GPR_D)
+ || !insn_uses_reg (&history[0], EXTRACT_OPERAND (RD, *ip),
+ MIPS_GR_REG))
+ && (mips_opts.mips16
+ || !(pinfo & INSN_WRITE_GPR_31)
+ || !insn_uses_reg (&history[0], RA, MIPS_GR_REG))
+ && (!mips_opts.mips16
+ || !(pinfo & MIPS16_INSN_WRITE_31)
+ || !insn_uses_reg (&history[0], RA, MIPS_GR_REG))
+ /* If one instruction sets a condition code and the other one uses
+ a condition code, we cannot swap. */
+ && !((pinfo & INSN_READ_COND_CODE)
+ && (prev_pinfo & INSN_WRITE_COND_CODE))
+ && !((pinfo & INSN_WRITE_COND_CODE)
+ && (prev_pinfo & INSN_READ_COND_CODE))
+ /* If the previous instruction uses the PC, we cannot swap. */
+ && !(mips_opts.mips16 && (prev_pinfo & MIPS16_INSN_READ_PC))
+ /* If the previous instruction had a fixup in mips16 mode, we
+ cannot swap. This normally means that the previous
+ instruction was a 4 byte branch anyhow. */
+ && !(mips_opts.mips16 && history[0].fixp[0])
+ /* If the previous instruction is a sync, sync.l, or sync.p,
+ we cannot swap. */
+ && !(prev_pinfo & INSN_SYNC)
+ /* If the previous instruction is an ERET or DERET, avoid the swap. */
+ && history[0].insn_opcode != INSN_ERET
+ && history[0].insn_opcode != INSN_DERET)
+ {
+ /* It looks like we can actually do the swap. */
+ branch_disp = insn_length (history);
+ branch_swap = TRUE;
+ }
+
#ifdef OBJ_ELF
/* The value passed to dwarf2_emit_insn is the distance between
the beginning of the current instruction and the address that
- should be recorded in the debug tables. For MIPS16 debug info
- we want to use ISA-encoded addresses, so we pass -1 for an
- address higher by one than the current. */
- dwarf2_emit_insn (mips_opts.mips16 ? -1 : 0);
-#endif
+ should be recorded in the debug tables. This is normally the
+ current address.
- /* Record the frag type before frag_var. */
- if (history[0].frag)
- prev_insn_frag_type = history[0].frag->fr_type;
+ For MIPS16 debug info we want to use ISA-encoded addresses,
+ so we use -1 for an address higher by one than the current one.
+
+ If the instruction produced is a branch that we will swap with
+ the preceding instruction, then we add the displacement by which
+ the branch will be moved backwards. This is more appropriate
+ and for MIPS16 code also prevents a debugger from placing a
+ breakpoint in the middle of the branch (and corrupting code if
+ software breakpoints are used). */
+ dwarf2_emit_insn ((mips_opts.mips16 ? -1 : 0) + branch_disp);
+#endif
if (address_expr
&& *reloc_type == BFD_RELOC_16_PCREL_S2
@@ -3166,234 +3368,14 @@ append_insn (struct mips_cl_insn *ip, ex
}
install_insn (ip);
- /* Update the register mask information. */
- if (! mips_opts.mips16)
- {
- if ((pinfo & INSN_WRITE_GPR_D) || (pinfo2 & INSN2_READ_GPR_D))
- mips_gprmask |= 1 << EXTRACT_OPERAND (RD, *ip);
- if ((pinfo & (INSN_WRITE_GPR_T | INSN_READ_GPR_T)) != 0)
- mips_gprmask |= 1 << EXTRACT_OPERAND (RT, *ip);
- if (pinfo & INSN_READ_GPR_S)
- mips_gprmask |= 1 << EXTRACT_OPERAND (RS, *ip);
- if (pinfo & INSN_WRITE_GPR_31)
- mips_gprmask |= 1 << RA;
- if (pinfo2 & (INSN2_WRITE_GPR_Z | INSN2_READ_GPR_Z))
- mips_gprmask |= 1 << EXTRACT_OPERAND (RZ, *ip);
- if (pinfo & INSN_WRITE_FPR_D)
- mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FD, *ip);
- if ((pinfo & (INSN_WRITE_FPR_S | INSN_READ_FPR_S)) != 0)
- mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FS, *ip);
- if ((pinfo & (INSN_WRITE_FPR_T | INSN_READ_FPR_T)) != 0)
- mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FT, *ip);
- if ((pinfo & INSN_READ_FPR_R) != 0)
- mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FR, *ip);
- if (pinfo2 & (INSN2_WRITE_FPR_Z | INSN2_READ_FPR_Z))
- mips_cprmask[1] |= 1 << EXTRACT_OPERAND (FZ, *ip);
- 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. */
- }
- /* Never set the bit for $0, which is always zero. */
- mips_gprmask &= ~1 << 0;
- }
- else
- {
- if (pinfo & (MIPS16_INSN_WRITE_X | MIPS16_INSN_READ_X))
- mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (RX, *ip);
- if (pinfo & (MIPS16_INSN_WRITE_Y | MIPS16_INSN_READ_Y))
- mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (RY, *ip);
- if (pinfo & MIPS16_INSN_WRITE_Z)
- mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (RZ, *ip);
- if (pinfo & (MIPS16_INSN_WRITE_T | MIPS16_INSN_READ_T))
- mips_gprmask |= 1 << TREG;
- if (pinfo & (MIPS16_INSN_WRITE_SP | MIPS16_INSN_READ_SP))
- mips_gprmask |= 1 << SP;
- if (pinfo & (MIPS16_INSN_WRITE_31 | MIPS16_INSN_READ_31))
- mips_gprmask |= 1 << RA;
- if (pinfo & MIPS16_INSN_WRITE_GPR_Y)
- mips_gprmask |= 1 << MIPS16OP_EXTRACT_REG32R (ip->insn_opcode);
- if (pinfo & MIPS16_INSN_READ_Z)
- mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (MOVE32Z, *ip);
- if (pinfo & MIPS16_INSN_READ_GPR_X)
- mips_gprmask |= 1 << MIPS16_EXTRACT_OPERAND (REGR32, *ip);
- }
-
if (mips_relax.sequence != 2 && !mips_opts.noreorder)
{
- /* Filling the branch delay slot is more complex. We try to
- switch the branch with the previous instruction, which we can
- do if the previous instruction does not set up a condition
- that the branch tests and if the branch is not itself the
- target of any branch. */
if ((pinfo & INSN_UNCOND_BRANCH_DELAY)
|| (pinfo & INSN_COND_BRANCH_DELAY))
{
- if (mips_optimize < 2
- /* If we have seen .set volatile or .set nomove, don't
- optimize. */
- || mips_opts.nomove != 0
- /* We can't swap if the previous instruction's position
- is fixed. */
- || history[0].fixed_p
- /* If the previous previous insn was in a .set
- noreorder, we can't swap. Actually, the MIPS
- assembler will swap in this situation. However, gcc
- configured -with-gnu-as will generate code like
- .set noreorder
- lw $4,XXX
- .set reorder
- INSN
- bne $4,$0,foo
- in which we can not swap the bne and INSN. If gcc is
- not configured -with-gnu-as, it does not output the
- .set pseudo-ops. */
- || history[1].noreorder_p
- /* If the branch is itself the target of a branch, we
- can not swap. We cheat on this; all we check for is
- whether there is a label on this instruction. If
- there are any branches to anything other than a
- label, users must use .set noreorder. */
- || si->label_list != NULL
- /* If the previous instruction is in a variant frag
- other than this branch's one, we cannot do the swap.
- This does not apply to the mips16, which uses variant
- frags for different purposes. */
- || (! mips_opts.mips16
- && prev_insn_frag_type == rs_machine_dependent)
- /* Check for conflicts between the branch and the instructions
- before the candidate delay slot. */
- || nops_for_insn (history + 1, ip) > 0
- /* Check for conflicts between the swapped sequence and the
- target of the branch. */
- || nops_for_sequence (2, history + 1, ip, history) > 0
- /* We do not swap with a trap instruction, since it
- complicates trap handlers to have the trap
- instruction be in a delay slot. */
- || (prev_pinfo & INSN_TRAP)
- /* If the branch reads a register that the previous
- instruction sets, we can not swap. */
- || (! mips_opts.mips16
- && (prev_pinfo & INSN_WRITE_GPR_T)
- && insn_uses_reg (ip, EXTRACT_OPERAND (RT, history[0]),
- MIPS_GR_REG))
- || (! mips_opts.mips16
- && (prev_pinfo & INSN_WRITE_GPR_D)
- && insn_uses_reg (ip, EXTRACT_OPERAND (RD, history[0]),
- MIPS_GR_REG))
- || (! mips_opts.mips16
- && (prev_pinfo2 & INSN2_WRITE_GPR_Z)
- && insn_uses_reg (ip, EXTRACT_OPERAND (RZ, history[0]),
- MIPS_GR_REG))
- || (mips_opts.mips16
- && (((prev_pinfo & MIPS16_INSN_WRITE_X)
- && (insn_uses_reg
- (ip, MIPS16_EXTRACT_OPERAND (RX, history[0]),
- MIPS16_REG)))
- || ((prev_pinfo & MIPS16_INSN_WRITE_Y)
- && (insn_uses_reg
- (ip, MIPS16_EXTRACT_OPERAND (RY, history[0]),
- MIPS16_REG)))
- || ((prev_pinfo & MIPS16_INSN_WRITE_Z)
- && (insn_uses_reg
- (ip, MIPS16_EXTRACT_OPERAND (RZ, history[0]),
- MIPS16_REG)))
- || ((prev_pinfo & MIPS16_INSN_WRITE_T)
- && insn_uses_reg (ip, TREG, MIPS_GR_REG))
- || ((prev_pinfo & MIPS16_INSN_WRITE_31)
- && insn_uses_reg (ip, RA, MIPS_GR_REG))
- || ((prev_pinfo & MIPS16_INSN_WRITE_GPR_Y)
- && insn_uses_reg (ip,
- MIPS16OP_EXTRACT_REG32R
- (history[0].insn_opcode),
- MIPS_GR_REG))))
- /* If the branch writes a register that the previous
- instruction sets, we can not swap (we know that
- branches write only to RD or to $31). */
- || (! mips_opts.mips16
- && (prev_pinfo & INSN_WRITE_GPR_T)
- && (((pinfo & INSN_WRITE_GPR_D)
- && (EXTRACT_OPERAND (RT, history[0])
- == EXTRACT_OPERAND (RD, *ip)))
- || ((pinfo & INSN_WRITE_GPR_31)
- && EXTRACT_OPERAND (RT, history[0]) == RA)))
- || (! mips_opts.mips16
- && (prev_pinfo & INSN_WRITE_GPR_D)
- && (((pinfo & INSN_WRITE_GPR_D)
- && (EXTRACT_OPERAND (RD, history[0])
- == EXTRACT_OPERAND (RD, *ip)))
- || ((pinfo & INSN_WRITE_GPR_31)
- && EXTRACT_OPERAND (RD, history[0]) == RA)))
- || (mips_opts.mips16
- && (pinfo & MIPS16_INSN_WRITE_31)
- && ((prev_pinfo & MIPS16_INSN_WRITE_31)
- || ((prev_pinfo & MIPS16_INSN_WRITE_GPR_Y)
- && (MIPS16OP_EXTRACT_REG32R (history[0].insn_opcode)
- == RA))))
- /* If the branch writes a register that the previous
- instruction reads, we can not swap (we know that
- branches only write to RD or to $31). */
- || (! mips_opts.mips16
- && (pinfo & INSN_WRITE_GPR_D)
- && insn_uses_reg (&history[0],
- EXTRACT_OPERAND (RD, *ip),
- MIPS_GR_REG))
- || (! mips_opts.mips16
- && (pinfo & INSN_WRITE_GPR_31)
- && insn_uses_reg (&history[0], RA, MIPS_GR_REG))
- || (mips_opts.mips16
- && (pinfo & MIPS16_INSN_WRITE_31)
- && insn_uses_reg (&history[0], RA, MIPS_GR_REG))
- /* If one instruction sets a condition code and the
- other one uses a condition code, we can not swap. */
- || ((pinfo & INSN_READ_COND_CODE)
- && (prev_pinfo & INSN_WRITE_COND_CODE))
- || ((pinfo & INSN_WRITE_COND_CODE)
- && (prev_pinfo & INSN_READ_COND_CODE))
- /* If the previous instruction uses the PC, we can not
- swap. */
- || (mips_opts.mips16
- && (prev_pinfo & MIPS16_INSN_READ_PC))
- /* If the previous instruction had a fixup in mips16
- mode, we can not swap. This normally means that the
- previous instruction was a 4 byte branch anyhow. */
- || (mips_opts.mips16 && history[0].fixp[0])
- /* If the previous instruction is a sync, sync.l, or
- sync.p, we can not swap. */
- || (prev_pinfo & INSN_SYNC)
- /* If the previous instruction is an ERET or
- DERET, avoid the swap. */
- || (history[0].insn_opcode == INSN_ERET)
- || (history[0].insn_opcode == INSN_DERET))
- {
- if (mips_opts.mips16
- && (pinfo & INSN_UNCOND_BRANCH_DELAY)
- && (pinfo & (MIPS16_INSN_READ_X | MIPS16_INSN_READ_31))
- && ISA_SUPPORTS_MIPS16E)
- {
- /* Convert MIPS16 jr/jalr into a "compact" jump. */
- ip->insn_opcode |= 0x0080;
- install_insn (ip);
- insert_into_history (0, 1, ip);
- }
- else
- {
- /* We could do even better for unconditional branches to
- portions of this object file; we could pick up the
- instruction at the destination, put it in the delay
- slot, and bump the destination address. */
- insert_into_history (0, 1, ip);
- emit_nop ();
- }
-
- if (mips_relax.sequence)
- mips_relax.sizes[mips_relax.sequence - 1] += 4;
- }
- else
+ if (branch_swap)
{
- /* It looks like we can actually do the swap. */
+ /* Do the swap now. */
struct mips_cl_insn delay = history[0];
if (mips_opts.mips16)
{
@@ -3421,6 +3403,31 @@ append_insn (struct mips_cl_insn *ip, ex
delay.fixed_p = 1;
insert_into_history (0, 1, &delay);
}
+ else
+ {
+ if (mips_opts.mips16
+ && (pinfo & INSN_UNCOND_BRANCH_DELAY)
+ && (pinfo & (MIPS16_INSN_READ_X | MIPS16_INSN_READ_31))
+ && ISA_SUPPORTS_MIPS16E)
+ {
+ /* Convert MIPS16 jr/jalr into a "compact" jump. */
+ ip->insn_opcode |= 0x0080;
+ install_insn (ip);
+ insert_into_history (0, 1, ip);
+ }
+ else
+ {
+ /* We could do even better for unconditional branches to
+ portions of this object file; we could pick up the
+ instruction at the destination, put it in the delay
+ slot, and bump the destination address. */
+ insert_into_history (0, 1, ip);
+ emit_nop ();
+ }
+
+ if (mips_relax.sequence)
+ mips_relax.sizes[mips_relax.sequence - 1] += 4;
+ }
/* If that was an unconditional branch, forget the previous
insn information. */
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap.d 2011-02-23 00:17:05.000000000 +0000
@@ -0,0 +1,61 @@
+#PROG: readelf
+#readelf: -wl
+#name: MIPS DWARF-2 location information with branch swapping
+#as: -32
+#source: loc-swap.s
+
+# Verify that DWARF-2 location information for instructions reordered
+# into a branch delay slot is updated to point to the branch instead.
+
+Raw dump of debug contents of section \.debug_line:
+
+ Offset: 0x0
+ Length: 67
+ DWARF Version: 2
+ Prologue Length: 33
+ Minimum Instruction Length: 1
+ Initial value of 'is_stmt': 1
+ Line Base: -5
+ Line Range: 14
+ Opcode Base: 13
+
+ Opcodes:
+ Opcode 1 has 0 args
+ Opcode 2 has 1 args
+ Opcode 3 has 1 args
+ Opcode 4 has 1 args
+ Opcode 5 has 1 args
+ Opcode 6 has 0 args
+ Opcode 7 has 0 args
+ Opcode 8 has 0 args
+ Opcode 9 has 1 args
+ Opcode 10 has 0 args
+ Opcode 11 has 0 args
+ Opcode 12 has 1 args
+
+ The Directory Table is empty\.
+
+ The File Name Table:
+ Entry Dir Time Size Name
+ 1 0 0 0 loc-swap\.s
+
+ Line Number Statements:
+ Extended opcode 2: set Address to 0x0
+ Special opcode 11: advance Address by 0 to 0x0 and Line by 6 to 7
+ Special opcode 63: advance Address by 4 to 0x4 and Line by 2 to 9
+ Special opcode 120: advance Address by 8 to 0xc and Line by 3 to 12
+ Special opcode 7: advance Address by 0 to 0xc and Line by 2 to 14
+ Special opcode 120: advance Address by 8 to 0x14 and Line by 3 to 17
+ Special opcode 7: advance Address by 0 to 0x14 and Line by 2 to 19
+ Special opcode 120: advance Address by 8 to 0x1c and Line by 3 to 22
+ Special opcode 63: advance Address by 4 to 0x20 and Line by 2 to 24
+ Special opcode 120: advance Address by 8 to 0x28 and Line by 3 to 27
+ Special opcode 63: advance Address by 4 to 0x2c and Line by 2 to 29
+ Special opcode 120: advance Address by 8 to 0x34 and Line by 3 to 32
+ Special opcode 63: advance Address by 4 to 0x38 and Line by 2 to 34
+ Special opcode 120: advance Address by 8 to 0x40 and Line by 3 to 37
+ Special opcode 7: advance Address by 0 to 0x40 and Line by 2 to 39
+ Special opcode 120: advance Address by 8 to 0x48 and Line by 3 to 42
+ Special opcode 63: advance Address by 4 to 0x4c and Line by 2 to 44
+ Advance PC by 16 to 0x5c
+ Extended opcode 1: End of Sequence
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap.s
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap.s 2011-02-23 00:52:51.000000000 +0000
@@ -0,0 +1,48 @@
+# Source file to test DWARF-2 location information with branch swapping.
+
+ .file 1 "loc-swap.s"
+ .text
+foo:
+ .loc 1 7
+ move $4, $16
+ .loc 1 9
+ jr $4
+
+ .loc 1 12
+ move $31, $16
+ .loc 1 14
+ jr $4
+
+ .loc 1 17
+ move $4, $16
+ .loc 1 19
+ jr $31
+
+ .loc 1 22
+ move $31, $16
+ .loc 1 24
+ jr $31
+
+ .loc 1 27
+ move $4, $16
+ .loc 1 29
+ jalr $4
+
+ .loc 1 32
+ move $31, $16
+ .loc 1 34
+ jalr $4
+
+ .loc 1 37
+ move $4, $16
+ .loc 1 39
+ jal bar
+
+ .loc 1 42
+ move $31, $16
+ .loc 1 44
+ jal bar
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 2
+ .space 16
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp 2011-02-22 02:17:20.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp 2011-02-23 00:24:25.000000000 +0000
@@ -831,6 +831,10 @@ if { [istarget mips*-*-vxworks*] } {
run_dump_test "jalr2"
run_dump_test_arches "aent" [mips_arch_list_matching mips1]
+
+ run_dump_test_arches "loc-swap" [mips_arch_list_all]
+ run_dump_test_arches "loc-swap-dis" \
+ [mips_arch_list_all]
}
if $has_newabi {
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@loc-swap-dis.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@loc-swap-dis.d 2011-02-22 23:51:06.000000000 +0000
@@ -0,0 +1,35 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS DWARF-2 location information with branch swapping disassembly
+#as: -32
+#source: loc-swap.s
+
+# Check branch swapping with DWARF-2 location information (MIPS16).
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> ec00 jr a0
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> ec00 jr a0
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> e820 jr ra
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> e820 jr ra
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> ec40 jalr a0
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> ec40 jalr a0
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 1800 0000 jal 0+0000 <foo>
+[ ]*[0-9a-f]+: R_MIPS16_26 bar
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> 1800 0000 jal 0+0000 <foo>
+[ ]*[0-9a-f]+: R_MIPS16_26 bar
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 6500 nop
+ \.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@loc-swap.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@loc-swap.d 2011-02-23 00:17:57.000000000 +0000
@@ -0,0 +1,61 @@
+#PROG: readelf
+#readelf: -wl
+#name: MIPS DWARF-2 location information with branch swapping
+#as: -32
+#source: loc-swap.s
+
+# Verify that DWARF-2 location information for instructions reordered
+# into a branch delay slot is updated to point to the branch instead.
+
+Raw dump of debug contents of section \.debug_line:
+
+ Offset: 0x0
+ Length: 67
+ DWARF Version: 2
+ Prologue Length: 33
+ Minimum Instruction Length: 1
+ Initial value of 'is_stmt': 1
+ Line Base: -5
+ Line Range: 14
+ Opcode Base: 13
+
+ Opcodes:
+ Opcode 1 has 0 args
+ Opcode 2 has 1 args
+ Opcode 3 has 1 args
+ Opcode 4 has 1 args
+ Opcode 5 has 1 args
+ Opcode 6 has 0 args
+ Opcode 7 has 0 args
+ Opcode 8 has 0 args
+ Opcode 9 has 1 args
+ Opcode 10 has 0 args
+ Opcode 11 has 0 args
+ Opcode 12 has 1 args
+
+ The Directory Table is empty\.
+
+ The File Name Table:
+ Entry Dir Time Size Name
+ 1 0 0 0 loc-swap\.s
+
+ Line Number Statements:
+ Extended opcode 2: set Address to 0x1
+ Special opcode 11: advance Address by 0 to 0x1 and Line by 6 to 7
+ Special opcode 35: advance Address by 2 to 0x3 and Line by 2 to 9
+ Special opcode 64: advance Address by 4 to 0x7 and Line by 3 to 12
+ Special opcode 7: advance Address by 0 to 0x7 and Line by 2 to 14
+ Special opcode 64: advance Address by 4 to 0xb and Line by 3 to 17
+ Special opcode 7: advance Address by 0 to 0xb and Line by 2 to 19
+ Special opcode 64: advance Address by 4 to 0xf and Line by 3 to 22
+ Special opcode 35: advance Address by 2 to 0x11 and Line by 2 to 24
+ Special opcode 64: advance Address by 4 to 0x15 and Line by 3 to 27
+ Special opcode 35: advance Address by 2 to 0x17 and Line by 2 to 29
+ Special opcode 64: advance Address by 4 to 0x1b and Line by 3 to 32
+ Special opcode 35: advance Address by 2 to 0x1d and Line by 2 to 34
+ Special opcode 64: advance Address by 4 to 0x21 and Line by 3 to 37
+ Special opcode 7: advance Address by 0 to 0x21 and Line by 2 to 39
+ Special opcode 92: advance Address by 6 to 0x27 and Line by 3 to 42
+ Special opcode 35: advance Address by 2 to 0x29 and Line by 2 to 44
+ Advance PC by 15 to 0x38
+ Extended opcode 1: End of Sequence
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code
2011-02-23 13:39 [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code Maciej W. Rozycki
@ 2011-06-29 21:26 ` Richard Sandiford
2011-07-02 1:20 ` Maciej W. Rozycki
0 siblings, 1 reply; 11+ messages in thread
From: Richard Sandiford @ 2011-06-29 21:26 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> 2011-02-23 Maciej W. Rozycki <macro@codesourcery.com>
>
> 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.
Sorry, I'd somehow got the idea that this patch applied on top of
the microMIPS changes. I now realise that it's the other way around.
I've updated the patch after the changes I made today, as below.
I think this is OK to commit. However:
- loc-swap-dis.d was missing from the patch
- this:
> + Advance PC by 16 to 0x5c
depends on the target alignment of .text; it passes for GNU/Linux
targets but fails on ELF ones. I think we should stub out the
exact numbers, since they're incidental to the test.
I've included the second change in the patch. If you want to
apply it along with loc-swap-dis.d, please feel free. Otherwise
send loc-swap-dis.d to me and I'll test and commit it.
Richard
gas/
2011-06-29 Maciej W. Rozycki <macro@codesourcery.com>
* config/tc-mips.c (append_insn: Make sure DWARF-2 location
information is properly adjusted for branches that get swapped.
gas/testsuite/
2011-06-29 Maciej W. Rozycki <macro@codesourcery.com>
* 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.
Index: gas/config/tc-mips.c
===================================================================
--- gas/config/tc-mips.c 2011-06-29 22:05:03.000000000 +0100
+++ gas/config/tc-mips.c 2011-06-29 22:06:15.000000000 +0100
@@ -3454,10 +3454,20 @@ append_insn (struct mips_cl_insn *ip, ex
#ifdef OBJ_ELF
/* The value passed to dwarf2_emit_insn is the distance between
the beginning of the current instruction and the address that
- should be recorded in the debug tables. For MIPS16 debug info
- we want to use ISA-encoded addresses, so we pass -1 for an
- address higher by one than the current. */
- dwarf2_emit_insn (mips_opts.mips16 ? -1 : 0);
+ should be recorded in the debug tables. This is normally the
+ current address.
+
+ For MIPS16 debug info we want to use ISA-encoded addresses,
+ so we use -1 for an address higher by one than the current one.
+
+ If the instruction produced is a branch that we will swap with
+ the preceding instruction, then we add the displacement by which
+ the branch will be moved backwards. This is more appropriate
+ and for MIPS16 code also prevents a debugger from placing a
+ breakpoint in the middle of the branch (and corrupting code if
+ software breakpoints are used). */
+ dwarf2_emit_insn ((mips_opts.mips16 ? -1 : 0)
+ + (method == APPEND_SWAP ? insn_length (history) : 0));
#endif
if (address_expr
Index: gas/testsuite/gas/mips/loc-swap.d
===================================================================
--- /dev/null 2011-06-29 20:49:09.402506167 +0100
+++ gas/testsuite/gas/mips/loc-swap.d 2011-06-29 22:16:56.000000000 +0100
@@ -0,0 +1,61 @@
+#PROG: readelf
+#readelf: -wl
+#name: MIPS DWARF-2 location information with branch swapping
+#as: -32
+#source: loc-swap.s
+
+# Verify that DWARF-2 location information for instructions reordered
+# into a branch delay slot is updated to point to the branch instead.
+
+Raw dump of debug contents of section \.debug_line:
+
+ Offset: 0x0
+ Length: 67
+ DWARF Version: 2
+ Prologue Length: 33
+ Minimum Instruction Length: 1
+ Initial value of 'is_stmt': 1
+ Line Base: -5
+ Line Range: 14
+ Opcode Base: 13
+
+ Opcodes:
+ Opcode 1 has 0 args
+ Opcode 2 has 1 args
+ Opcode 3 has 1 args
+ Opcode 4 has 1 args
+ Opcode 5 has 1 args
+ Opcode 6 has 0 args
+ Opcode 7 has 0 args
+ Opcode 8 has 0 args
+ Opcode 9 has 1 args
+ Opcode 10 has 0 args
+ Opcode 11 has 0 args
+ Opcode 12 has 1 args
+
+ The Directory Table is empty\.
+
+ The File Name Table:
+ Entry Dir Time Size Name
+ 1 0 0 0 loc-swap\.s
+
+ Line Number Statements:
+ Extended opcode 2: set Address to 0x0
+ Special opcode 11: advance Address by 0 to 0x0 and Line by 6 to 7
+ Special opcode 63: advance Address by 4 to 0x4 and Line by 2 to 9
+ Special opcode 120: advance Address by 8 to 0xc and Line by 3 to 12
+ Special opcode 7: advance Address by 0 to 0xc and Line by 2 to 14
+ Special opcode 120: advance Address by 8 to 0x14 and Line by 3 to 17
+ Special opcode 7: advance Address by 0 to 0x14 and Line by 2 to 19
+ Special opcode 120: advance Address by 8 to 0x1c and Line by 3 to 22
+ Special opcode 63: advance Address by 4 to 0x20 and Line by 2 to 24
+ Special opcode 120: advance Address by 8 to 0x28 and Line by 3 to 27
+ Special opcode 63: advance Address by 4 to 0x2c and Line by 2 to 29
+ Special opcode 120: advance Address by 8 to 0x34 and Line by 3 to 32
+ Special opcode 63: advance Address by 4 to 0x38 and Line by 2 to 34
+ Special opcode 120: advance Address by 8 to 0x40 and Line by 3 to 37
+ Special opcode 7: advance Address by 0 to 0x40 and Line by 2 to 39
+ Special opcode 120: advance Address by 8 to 0x48 and Line by 3 to 42
+ Special opcode 63: advance Address by 4 to 0x4c and Line by 2 to 44
+ Advance PC by .* to 0x.*
+ Extended opcode 1: End of Sequence
Index: gas/testsuite/gas/mips/mips16@loc-swap.d
===================================================================
--- /dev/null 2011-06-29 20:49:09.402506167 +0100
+++ gas/testsuite/gas/mips/mips16@loc-swap.d 2011-06-29 22:18:41.000000000 +0100
@@ -0,0 +1,61 @@
+#PROG: readelf
+#readelf: -wl
+#name: MIPS DWARF-2 location information with branch swapping
+#as: -32
+#source: loc-swap.s
+
+# Verify that DWARF-2 location information for instructions reordered
+# into a branch delay slot is updated to point to the branch instead.
+
+Raw dump of debug contents of section \.debug_line:
+
+ Offset: 0x0
+ Length: 67
+ DWARF Version: 2
+ Prologue Length: 33
+ Minimum Instruction Length: 1
+ Initial value of 'is_stmt': 1
+ Line Base: -5
+ Line Range: 14
+ Opcode Base: 13
+
+ Opcodes:
+ Opcode 1 has 0 args
+ Opcode 2 has 1 args
+ Opcode 3 has 1 args
+ Opcode 4 has 1 args
+ Opcode 5 has 1 args
+ Opcode 6 has 0 args
+ Opcode 7 has 0 args
+ Opcode 8 has 0 args
+ Opcode 9 has 1 args
+ Opcode 10 has 0 args
+ Opcode 11 has 0 args
+ Opcode 12 has 1 args
+
+ The Directory Table is empty\.
+
+ The File Name Table:
+ Entry Dir Time Size Name
+ 1 0 0 0 loc-swap\.s
+
+ Line Number Statements:
+ Extended opcode 2: set Address to 0x1
+ Special opcode 11: advance Address by 0 to 0x1 and Line by 6 to 7
+ Special opcode 35: advance Address by 2 to 0x3 and Line by 2 to 9
+ Special opcode 64: advance Address by 4 to 0x7 and Line by 3 to 12
+ Special opcode 7: advance Address by 0 to 0x7 and Line by 2 to 14
+ Special opcode 64: advance Address by 4 to 0xb and Line by 3 to 17
+ Special opcode 7: advance Address by 0 to 0xb and Line by 2 to 19
+ Special opcode 64: advance Address by 4 to 0xf and Line by 3 to 22
+ Special opcode 35: advance Address by 2 to 0x11 and Line by 2 to 24
+ Special opcode 64: advance Address by 4 to 0x15 and Line by 3 to 27
+ Special opcode 35: advance Address by 2 to 0x17 and Line by 2 to 29
+ Special opcode 64: advance Address by 4 to 0x1b and Line by 3 to 32
+ Special opcode 35: advance Address by 2 to 0x1d and Line by 2 to 34
+ Special opcode 64: advance Address by 4 to 0x21 and Line by 3 to 37
+ Special opcode 7: advance Address by 0 to 0x21 and Line by 2 to 39
+ Special opcode 92: advance Address by 6 to 0x27 and Line by 3 to 42
+ Special opcode 35: advance Address by 2 to 0x29 and Line by 2 to 44
+ Advance PC by .* to 0x.*
+ Extended opcode 1: End of Sequence
Index: gas/testsuite/gas/mips/mips16@loc-swap-dis.d
===================================================================
--- /dev/null 2011-06-29 20:49:09.402506167 +0100
+++ gas/testsuite/gas/mips/mips16@loc-swap-dis.d 2011-06-29 22:06:15.000000000 +0100
@@ -0,0 +1,35 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS DWARF-2 location information with branch swapping disassembly
+#as: -32
+#source: loc-swap.s
+
+# Check branch swapping with DWARF-2 location information (MIPS16).
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> ec00 jr a0
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> ec00 jr a0
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> e820 jr ra
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> e820 jr ra
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> ec40 jalr a0
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> ec40 jalr a0
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 1800 0000 jal 0+0000 <foo>
+[ ]*[0-9a-f]+: R_MIPS16_26 bar
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> 1800 0000 jal 0+0000 <foo>
+[ ]*[0-9a-f]+: R_MIPS16_26 bar
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 6500 nop
+ \.\.\.
Index: gas/testsuite/gas/mips/loc-swap.s
===================================================================
--- /dev/null 2011-06-29 20:49:09.402506167 +0100
+++ gas/testsuite/gas/mips/loc-swap.s 2011-06-29 22:17:43.000000000 +0100
@@ -0,0 +1,48 @@
+# Source file to test DWARF-2 location information with branch swapping.
+
+ .file 1 "loc-swap.s"
+ .text
+foo:
+ .loc 1 7
+ move $4, $16
+ .loc 1 9
+ jr $4
+
+ .loc 1 12
+ move $31, $16
+ .loc 1 14
+ jr $4
+
+ .loc 1 17
+ move $4, $16
+ .loc 1 19
+ jr $31
+
+ .loc 1 22
+ move $31, $16
+ .loc 1 24
+ jr $31
+
+ .loc 1 27
+ move $4, $16
+ .loc 1 29
+ jalr $4
+
+ .loc 1 32
+ move $31, $16
+ .loc 1 34
+ jalr $4
+
+ .loc 1 37
+ move $4, $16
+ .loc 1 39
+ jal bar
+
+ .loc 1 42
+ move $31, $16
+ .loc 1 44
+ jal bar
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 2
+ .space 16
Index: gas/testsuite/gas/mips/mips.exp
===================================================================
--- gas/testsuite/gas/mips/mips.exp 2011-06-29 21:38:31.000000000 +0100
+++ gas/testsuite/gas/mips/mips.exp 2011-06-29 22:06:15.000000000 +0100
@@ -854,6 +854,10 @@ if { [istarget mips*-*-vxworks*] } {
[mips_arch_list_matching mips1]
run_dump_test_arches "branch-misc-4-64" \
[mips_arch_list_matching mips3]
+
+ run_dump_test_arches "loc-swap" [mips_arch_list_all]
+ run_dump_test_arches "loc-swap-dis" \
+ [mips_arch_list_all]
}
if $has_newabi {
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code
2011-06-29 21:26 ` Richard Sandiford
@ 2011-07-02 1:20 ` Maciej W. Rozycki
2011-07-02 7:33 ` Richard Sandiford
0 siblings, 1 reply; 11+ messages in thread
From: Maciej W. Rozycki @ 2011-07-02 1:20 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
On Wed, 29 Jun 2011, Richard Sandiford wrote:
> "Maciej W. Rozycki" <macro@codesourcery.com> writes:
> > 2011-02-23 Maciej W. Rozycki <macro@codesourcery.com>
> >
> > 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.
>
> Sorry, I'd somehow got the idea that this patch applied on top of
> the microMIPS changes. I now realise that it's the other way around.
Well, I've got some remains of sanity still in place. ;)
Speaking of which, I plan to get back to the microMIPS changes, well...
sometime. Hopefully this month. Sorry about the delays.
> I've updated the patch after the changes I made today, as below.
> I think this is OK to commit. However:
>
> - loc-swap-dis.d was missing from the patch
Oops, sorry about that. I sent an outdated version it would seem as my
current one did have this file included.
> - this:
>
> > + Advance PC by 16 to 0x5c
>
> depends on the target alignment of .text; it passes for GNU/Linux
> targets but fails on ELF ones. I think we should stub out the
> exact numbers, since they're incidental to the test.
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...
> I've included the second change in the patch. If you want to
> apply it along with loc-swap-dis.d, please feel free. Otherwise
> send loc-swap-dis.d to me and I'll test and commit it.
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.
Here's a version of the change I'd like to push. If that still fails on
any of your pet targets, then I'll be happy to reintroduce the wildcard
match. Otherwise I'll insist on keeping it as is.
And BTW, thanks a lot for the register mask, etc. rewrites! They make
this piece of code much easier to follow.
2011-07-02 Maciej W. Rozycki <macro@codesourcery.com>
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.
Maciej
binutils-gas-mips-branch-swap-dwarf2.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c 2011-07-02 00:40:03.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c 2011-07-02 00:53:52.000000000 +0100
@@ -3454,10 +3454,20 @@ append_insn (struct mips_cl_insn *ip, ex
#ifdef OBJ_ELF
/* The value passed to dwarf2_emit_insn is the distance between
the beginning of the current instruction and the address that
- should be recorded in the debug tables. For MIPS16 debug info
- we want to use ISA-encoded addresses, so we pass -1 for an
- address higher by one than the current. */
- dwarf2_emit_insn (mips_opts.mips16 ? -1 : 0);
+ should be recorded in the debug tables. This is normally the
+ current address.
+
+ For MIPS16 debug info we want to use ISA-encoded addresses,
+ so we use -1 for an address higher by one than the current one.
+
+ If the instruction produced is a branch that we will swap with
+ the preceding instruction, then we add the displacement by which
+ the branch will be moved backwards. This is more appropriate
+ and for MIPS16 code also prevents a debugger from placing a
+ breakpoint in the middle of the branch (and corrupting code if
+ software breakpoints are used). */
+ dwarf2_emit_insn ((mips_opts.mips16 ? -1 : 0)
+ + (method == APPEND_SWAP ? insn_length (history) : 0));
#endif
if (address_expr
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap.d 2011-07-02 00:53:52.000000000 +0100
@@ -0,0 +1,61 @@
+#PROG: readelf
+#readelf: -wl
+#name: MIPS DWARF-2 location information with branch swapping
+#as: -32
+#source: loc-swap.s
+
+# Verify that DWARF-2 location information for instructions reordered
+# into a branch delay slot is updated to point to the branch instead.
+
+Raw dump of debug contents of section \.debug_line:
+
+ Offset: 0x0
+ Length: 67
+ DWARF Version: 2
+ Prologue Length: 33
+ Minimum Instruction Length: 1
+ Initial value of 'is_stmt': 1
+ Line Base: -5
+ Line Range: 14
+ Opcode Base: 13
+
+ Opcodes:
+ Opcode 1 has 0 args
+ Opcode 2 has 1 args
+ Opcode 3 has 1 args
+ Opcode 4 has 1 args
+ Opcode 5 has 1 args
+ Opcode 6 has 0 args
+ Opcode 7 has 0 args
+ Opcode 8 has 0 args
+ Opcode 9 has 1 args
+ Opcode 10 has 0 args
+ Opcode 11 has 0 args
+ Opcode 12 has 1 args
+
+ The Directory Table is empty\.
+
+ The File Name Table:
+ Entry Dir Time Size Name
+ 1 0 0 0 loc-swap\.s
+
+ Line Number Statements:
+ Extended opcode 2: set Address to 0x0
+ Special opcode 11: advance Address by 0 to 0x0 and Line by 6 to 7
+ Special opcode 63: advance Address by 4 to 0x4 and Line by 2 to 9
+ Special opcode 120: advance Address by 8 to 0xc and Line by 3 to 12
+ Special opcode 7: advance Address by 0 to 0xc and Line by 2 to 14
+ Special opcode 120: advance Address by 8 to 0x14 and Line by 3 to 17
+ Special opcode 7: advance Address by 0 to 0x14 and Line by 2 to 19
+ Special opcode 120: advance Address by 8 to 0x1c and Line by 3 to 22
+ Special opcode 63: advance Address by 4 to 0x20 and Line by 2 to 24
+ Special opcode 120: advance Address by 8 to 0x28 and Line by 3 to 27
+ Special opcode 63: advance Address by 4 to 0x2c and Line by 2 to 29
+ Special opcode 120: advance Address by 8 to 0x34 and Line by 3 to 32
+ Special opcode 63: advance Address by 4 to 0x38 and Line by 2 to 34
+ Special opcode 120: advance Address by 8 to 0x40 and Line by 3 to 37
+ Special opcode 7: advance Address by 0 to 0x40 and Line by 2 to 39
+ Special opcode 120: advance Address by 8 to 0x48 and Line by 3 to 42
+ Special opcode 63: advance Address by 4 to 0x4c and Line by 2 to 44
+ Advance PC by 24 to 0x64
+ Extended opcode 1: End of Sequence
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap.s
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap.s 2011-07-02 00:53:52.000000000 +0100
@@ -0,0 +1,48 @@
+# Source file to test DWARF-2 location information with branch swapping.
+
+ .file 1 "loc-swap.s"
+ .text
+foo:
+ .loc 1 7
+ move $4, $16
+ .loc 1 9
+ jr $4
+
+ .loc 1 12
+ move $31, $16
+ .loc 1 14
+ jr $4
+
+ .loc 1 17
+ move $4, $16
+ .loc 1 19
+ jr $31
+
+ .loc 1 22
+ move $31, $16
+ .loc 1 24
+ jr $31
+
+ .loc 1 27
+ move $4, $16
+ .loc 1 29
+ jalr $4
+
+ .loc 1 32
+ move $31, $16
+ .loc 1 34
+ jalr $4
+
+ .loc 1 37
+ move $4, $16
+ .loc 1 39
+ jal bar
+
+ .loc 1 42
+ move $31, $16
+ .loc 1 44
+ jal bar
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 2
+ .space 16
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp 2011-07-02 00:52:11.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp 2011-07-02 00:53:52.000000000 +0100
@@ -854,6 +854,10 @@ if { [istarget mips*-*-vxworks*] } {
[mips_arch_list_matching mips1]
run_dump_test_arches "branch-misc-4-64" \
[mips_arch_list_matching mips3]
+
+ run_dump_test_arches "loc-swap" [mips_arch_list_all]
+ run_dump_test_arches "loc-swap-dis" \
+ [mips_arch_list_all]
}
if $has_newabi {
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@loc-swap-dis.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@loc-swap-dis.d 2011-07-02 00:53:52.000000000 +0100
@@ -0,0 +1,35 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS DWARF-2 location information with branch swapping disassembly
+#as: -32
+#source: loc-swap.s
+
+# Check branch swapping with DWARF-2 location information (MIPS16).
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> ec00 jr a0
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> ec00 jr a0
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> e820 jr ra
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> e820 jr ra
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> ec40 jalr a0
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> ec40 jalr a0
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 1800 0000 jal 0+0000 <foo>
+[ ]*[0-9a-f]+: R_MIPS16_26 bar
+[0-9a-f]+ <[^>]*> 6790 move a0,s0
+[0-9a-f]+ <[^>]*> 65f8 move ra,s0
+[0-9a-f]+ <[^>]*> 1800 0000 jal 0+0000 <foo>
+[ ]*[0-9a-f]+: R_MIPS16_26 bar
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> 6500 nop
+ \.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@loc-swap.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@loc-swap.d 2011-07-02 00:53:52.000000000 +0100
@@ -0,0 +1,61 @@
+#PROG: readelf
+#readelf: -wl
+#name: MIPS DWARF-2 location information with branch swapping
+#as: -32
+#source: loc-swap.s
+
+# Verify that DWARF-2 location information for instructions reordered
+# into a branch delay slot is updated to point to the branch instead.
+
+Raw dump of debug contents of section \.debug_line:
+
+ Offset: 0x0
+ Length: 67
+ DWARF Version: 2
+ Prologue Length: 33
+ Minimum Instruction Length: 1
+ Initial value of 'is_stmt': 1
+ Line Base: -5
+ Line Range: 14
+ Opcode Base: 13
+
+ Opcodes:
+ Opcode 1 has 0 args
+ Opcode 2 has 1 args
+ Opcode 3 has 1 args
+ Opcode 4 has 1 args
+ Opcode 5 has 1 args
+ Opcode 6 has 0 args
+ Opcode 7 has 0 args
+ Opcode 8 has 0 args
+ Opcode 9 has 1 args
+ Opcode 10 has 0 args
+ Opcode 11 has 0 args
+ Opcode 12 has 1 args
+
+ The Directory Table is empty\.
+
+ The File Name Table:
+ Entry Dir Time Size Name
+ 1 0 0 0 loc-swap\.s
+
+ Line Number Statements:
+ Extended opcode 2: set Address to 0x1
+ Special opcode 11: advance Address by 0 to 0x1 and Line by 6 to 7
+ Special opcode 35: advance Address by 2 to 0x3 and Line by 2 to 9
+ Special opcode 64: advance Address by 4 to 0x7 and Line by 3 to 12
+ Special opcode 7: advance Address by 0 to 0x7 and Line by 2 to 14
+ Special opcode 64: advance Address by 4 to 0xb and Line by 3 to 17
+ Special opcode 7: advance Address by 0 to 0xb and Line by 2 to 19
+ Special opcode 64: advance Address by 4 to 0xf and Line by 3 to 22
+ Special opcode 35: advance Address by 2 to 0x11 and Line by 2 to 24
+ Special opcode 64: advance Address by 4 to 0x15 and Line by 3 to 27
+ Special opcode 35: advance Address by 2 to 0x17 and Line by 2 to 29
+ Special opcode 64: advance Address by 4 to 0x1b and Line by 3 to 32
+ Special opcode 35: advance Address by 2 to 0x1d and Line by 2 to 34
+ Special opcode 64: advance Address by 4 to 0x21 and Line by 3 to 37
+ Special opcode 7: advance Address by 0 to 0x21 and Line by 2 to 39
+ Special opcode 92: advance Address by 6 to 0x27 and Line by 3 to 42
+ Special opcode 35: advance Address by 2 to 0x29 and Line by 2 to 44
+ Advance PC by 23 to 0x40
+ Extended opcode 1: End of Sequence
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap-dis.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/loc-swap-dis.d 2011-07-02 00:56:31.000000000 +0100
@@ -0,0 +1,34 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS DWARF-2 location information with branch swapping disassembly
+#as: -32
+#source: loc-swap.s
+
+# Check branch swapping with DWARF-2 location information.
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 02002021 move a0,s0
+[0-9a-f]+ <[^>]*> 00800008 jr a0
+[0-9a-f]+ <[^>]*> 00000000 nop
+[0-9a-f]+ <[^>]*> 00800008 jr a0
+[0-9a-f]+ <[^>]*> 0200f821 move ra,s0
+[0-9a-f]+ <[^>]*> 03e00008 jr ra
+[0-9a-f]+ <[^>]*> 02002021 move a0,s0
+[0-9a-f]+ <[^>]*> 0200f821 move ra,s0
+[0-9a-f]+ <[^>]*> 03e00008 jr ra
+[0-9a-f]+ <[^>]*> 00000000 nop
+[0-9a-f]+ <[^>]*> 02002021 move a0,s0
+[0-9a-f]+ <[^>]*> 0080f809 jalr a0
+[0-9a-f]+ <[^>]*> 00000000 nop
+[0-9a-f]+ <[^>]*> 0200f821 move ra,s0
+[0-9a-f]+ <[^>]*> 0080f809 jalr a0
+[0-9a-f]+ <[^>]*> 00000000 nop
+[0-9a-f]+ <[^>]*> 0c000000 jal 0+0000 <foo>
+[ ]*[0-9a-f]+: R_MIPS_26 bar
+[0-9a-f]+ <[^>]*> 02002021 move a0,s0
+[0-9a-f]+ <[^>]*> 0200f821 move ra,s0
+[0-9a-f]+ <[^>]*> 0c000000 jal 0+0000 <foo>
+[ ]*[0-9a-f]+: R_MIPS_26 bar
+[0-9a-f]+ <[^>]*> 00000000 nop
+ \.\.\.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code
2011-07-02 1:20 ` Maciej W. Rozycki
@ 2011-07-02 7:33 ` Richard Sandiford
2011-07-04 20:30 ` Maciej W. Rozycki
0 siblings, 1 reply; 11+ messages in thread
From: Richard Sandiford @ 2011-07-02 7:33 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> - this:
>>
>> > + Advance PC by 16 to 0x5c
>>
>> depends on the target alignment of .text; it passes for GNU/Linux
>> targets but fails on ELF ones. I think we should stub out the
>> exact numbers, since they're incidental to the test.
>
> 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 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.
> 2011-07-02 Maciej W. Rozycki <macro@codesourcery.com>
>
> 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.
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code
2011-07-02 7:33 ` Richard Sandiford
@ 2011-07-04 20:30 ` Maciej W. Rozycki
2011-07-04 22:20 ` Richard Sandiford
0 siblings, 1 reply; 11+ messages in thread
From: Maciej W. Rozycki @ 2011-07-04 20:30 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
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 <macro@codesourcery.com>
> >
> > 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 <macro@codesourcery.com>
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;
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code
2011-07-04 20:30 ` Maciej W. Rozycki
@ 2011-07-04 22:20 ` Richard Sandiford
2011-07-05 0:28 ` Maciej W. Rozycki
0 siblings, 1 reply; 11+ messages in thread
From: Richard Sandiford @ 2011-07-04 22:20 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> BTW, I've been wondering about this construct:
>
> return mask & ~0;
>
> you've introduced in a couple of places.
Oops. Fixed below.
> 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. ;)
Those comments were already there. I just moved them around.
> 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,
Yes. I checked that before writing it this way. It's guaranteed to be
harmless anyway, because "until" MIPS16 gets hardware FP support, we'd
never have any set bits.
I structured the code to be consistent with the GPR case. If you feel
strongly about it though, feel free to move the conditions inside the
existing "!mips_opts.mips16" check.
Richard
gas/
* config/tc-mips.c (gpr_read_mask, gpr_write_mask): Fix handling
of register 0.
Index: gas/config/tc-mips.c
===================================================================
--- gas/config/tc-mips.c 2011-07-04 21:00:54.000000000 +0100
+++ gas/config/tc-mips.c 2011-07-04 21:01:13.000000000 +0100
@@ -2450,7 +2450,8 @@ gpr_read_mask (const struct mips_cl_insn
if (pinfo2 & INSN2_READ_GPR_Z)
mask |= 1 << EXTRACT_OPERAND (RZ, *ip);
}
- return mask & ~0;
+ /* Don't include register 0. */
+ return mask & ~1;
}
/* Return the mask of core registers that IP writes. */
@@ -2492,7 +2493,8 @@ gpr_write_mask (const struct mips_cl_ins
if (pinfo2 & INSN2_WRITE_GPR_Z)
mask |= 1 << EXTRACT_OPERAND (RZ, *ip);
}
- return mask & ~0;
+ /* Don't include register 0. */
+ return mask & ~1;
}
/* Return the mask of floating-point registers that IP reads. */
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code
2011-07-04 22:20 ` Richard Sandiford
@ 2011-07-05 0:28 ` Maciej W. Rozycki
2011-07-05 11:29 ` Richard Sandiford
2011-08-03 14:55 ` [ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode Maciej W. Rozycki
0 siblings, 2 replies; 11+ messages in thread
From: Maciej W. Rozycki @ 2011-07-05 0:28 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
On Mon, 4 Jul 2011, Richard Sandiford wrote:
> > BTW, I've been wondering about this construct:
> >
> > return mask & ~0;
> >
> > you've introduced in a couple of places.
>
> Oops. Fixed below.
Hmm, makes sense indeed, thanks. Except that it didn't pop up in testing
which is worrisome and means that all this branch swapping stuff is not
really terribly well covered. I think we should cook up something more
systematic than the couple of scattered cases we already have.
> > 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. ;)
>
> Those comments were already there. I just moved them around.
Fair enough. I got a notion of being otherwise, but now that I have
double-checked you are right. Sorry about the confusion.
In that case though I'm tempted to adjust the comments I'll be tweaking
anyway with the microMIPS changes, if you don't mind that is.
> > 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,
>
> Yes. I checked that before writing it this way. It's guaranteed to be
> harmless anyway, because "until" MIPS16 gets hardware FP support, we'd
> never have any set bits.
Actually, I was more worried about this bit being reused for something
else as a distinct MIPS16_* flag and then hitting this condition
inadvertently. There is a place in append_insn() using the FPR mask
unconditionally even in the MIPS16 mode (quite rightfully IMO). Granted,
it's only used for the calculation of .fmask, that is less of importance
now that we have DWARF-2, but still.
How about making a suitable comment in include/opcode/mips.h, preferably
following all the other MIPS16_* flags, that the bit position used for the
FP_D flag is already reserved for MIPS16 code too? There's a terse
description already there covering the shared flags. I think a change
like below should be enough.
I think INSN_ISA3 should be moved to the beginning and made clearly
distinct. It took me a while to notice it's there. I have decided these
changes are too trivial to be kept separate, so I've made them both at
once.
> I structured the code to be consistent with the GPR case. If you feel
> strongly about it though, feel free to move the conditions inside the
> existing "!mips_opts.mips16" check.
I just feel no confidence that the notion of such an assumption will
stand for the next half a dozen years or so. The chance for the MIPS16
ASE being extended any further is probably slim, but the lifespan of
binutils is long enough to be careful about any assumptions and about
long-term maintenance of any piece of code. We seem to keep discovering
obscure oddities all the time despite quite a strict maintenance regime.
As long as include/opcode/mips.h is updated accordingly I don't insist on
my previous change though.
Maciej
2011-07-04 Maciej W. Rozycki <macro@codesourcery.com>
include/opcode/
* mips.h: Document the use of FP_D in MIPS16 mode. Adjust the
order of flags documented.
Index: include/opcode/mips.h
===================================================================
RCS file: /cvs/src/src/include/opcode/mips.h,v
retrieving revision 1.73
diff -u -p -r1.73 mips.h
--- include/opcode/mips.h 28 Feb 2011 16:06:51 -0000 1.73
+++ include/opcode/mips.h 4 Jul 2011 22:57:01 -0000
@@ -1132,6 +1132,9 @@ extern int bfd_mips_num_opcodes;
/* The following flags have the same value for the mips16 opcode
table:
+
+ INSN_ISA3
+
INSN_UNCOND_BRANCH_DELAY
INSN_COND_BRANCH_DELAY
INSN_COND_BRANCH_LIKELY (never used)
@@ -1140,7 +1143,7 @@ extern int bfd_mips_num_opcodes;
INSN_WRITE_HI
INSN_WRITE_LO
INSN_TRAP
- INSN_ISA3
+ FP_D (never used)
*/
extern const struct mips_opcode mips16_opcodes[];
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code
2011-07-05 0:28 ` Maciej W. Rozycki
@ 2011-07-05 11:29 ` Richard Sandiford
2011-08-03 14:55 ` [ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode Maciej W. Rozycki
1 sibling, 0 replies; 11+ messages in thread
From: Richard Sandiford @ 2011-07-05 11:29 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> 2011-07-04 Maciej W. Rozycki <macro@codesourcery.com>
>
> include/opcode/
> * mips.h: Document the use of FP_D in MIPS16 mode. Adjust the
> order of flags documented.
OK.
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* [ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode
2011-07-05 0:28 ` Maciej W. Rozycki
2011-07-05 11:29 ` Richard Sandiford
@ 2011-08-03 14:55 ` Maciej W. Rozycki
2011-08-03 19:47 ` Richard Sandiford
1 sibling, 1 reply; 11+ messages in thread
From: Maciej W. Rozycki @ 2011-08-03 14:55 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
Hi Richard,
Where are we with the change below?
Maciej
On Mon, 4 Jul 2011, Maciej W. Rozycki wrote:
> On Mon, 4 Jul 2011, Richard Sandiford wrote:
>
> > > BTW, I've been wondering about this construct:
> > >
> > > return mask & ~0;
> > >
> > > you've introduced in a couple of places.
> >
> > Oops. Fixed below.
>
> Hmm, makes sense indeed, thanks. Except that it didn't pop up in testing
> which is worrisome and means that all this branch swapping stuff is not
> really terribly well covered. I think we should cook up something more
> systematic than the couple of scattered cases we already have.
>
> > > 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. ;)
> >
> > Those comments were already there. I just moved them around.
>
> Fair enough. I got a notion of being otherwise, but now that I have
> double-checked you are right. Sorry about the confusion.
>
> In that case though I'm tempted to adjust the comments I'll be tweaking
> anyway with the microMIPS changes, if you don't mind that is.
>
> > > 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,
> >
> > Yes. I checked that before writing it this way. It's guaranteed to be
> > harmless anyway, because "until" MIPS16 gets hardware FP support, we'd
> > never have any set bits.
>
> Actually, I was more worried about this bit being reused for something
> else as a distinct MIPS16_* flag and then hitting this condition
> inadvertently. There is a place in append_insn() using the FPR mask
> unconditionally even in the MIPS16 mode (quite rightfully IMO). Granted,
> it's only used for the calculation of .fmask, that is less of importance
> now that we have DWARF-2, but still.
>
> How about making a suitable comment in include/opcode/mips.h, preferably
> following all the other MIPS16_* flags, that the bit position used for the
> FP_D flag is already reserved for MIPS16 code too? There's a terse
> description already there covering the shared flags. I think a change
> like below should be enough.
>
> I think INSN_ISA3 should be moved to the beginning and made clearly
> distinct. It took me a while to notice it's there. I have decided these
> changes are too trivial to be kept separate, so I've made them both at
> once.
>
> > I structured the code to be consistent with the GPR case. If you feel
> > strongly about it though, feel free to move the conditions inside the
> > existing "!mips_opts.mips16" check.
>
> I just feel no confidence that the notion of such an assumption will
> stand for the next half a dozen years or so. The chance for the MIPS16
> ASE being extended any further is probably slim, but the lifespan of
> binutils is long enough to be careful about any assumptions and about
> long-term maintenance of any piece of code. We seem to keep discovering
> obscure oddities all the time despite quite a strict maintenance regime.
>
> As long as include/opcode/mips.h is updated accordingly I don't insist on
> my previous change though.
>
> Maciej
>
> 2011-07-04 Maciej W. Rozycki <macro@codesourcery.com>
>
> include/opcode/
> * mips.h: Document the use of FP_D in MIPS16 mode. Adjust the
> order of flags documented.
>
> Index: include/opcode/mips.h
> ===================================================================
> RCS file: /cvs/src/src/include/opcode/mips.h,v
> retrieving revision 1.73
> diff -u -p -r1.73 mips.h
> --- include/opcode/mips.h 28 Feb 2011 16:06:51 -0000 1.73
> +++ include/opcode/mips.h 4 Jul 2011 22:57:01 -0000
> @@ -1132,6 +1132,9 @@ extern int bfd_mips_num_opcodes;
>
> /* The following flags have the same value for the mips16 opcode
> table:
> +
> + INSN_ISA3
> +
> INSN_UNCOND_BRANCH_DELAY
> INSN_COND_BRANCH_DELAY
> INSN_COND_BRANCH_LIKELY (never used)
> @@ -1140,7 +1143,7 @@ extern int bfd_mips_num_opcodes;
> INSN_WRITE_HI
> INSN_WRITE_LO
> INSN_TRAP
> - INSN_ISA3
> + FP_D (never used)
> */
>
> extern const struct mips_opcode mips16_opcodes[];
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode
2011-08-03 14:55 ` [ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode Maciej W. Rozycki
@ 2011-08-03 19:47 ` Richard Sandiford
2011-08-03 21:13 ` Maciej W. Rozycki
0 siblings, 1 reply; 11+ messages in thread
From: Richard Sandiford @ 2011-08-03 19:47 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> Hi Richard,
>
> Where are we with the change below?
I approved it here:
http://sourceware.org/ml/binutils/2011-07/msg00049.html
so please go ahead and commit.
Thanks,
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode
2011-08-03 19:47 ` Richard Sandiford
@ 2011-08-03 21:13 ` Maciej W. Rozycki
0 siblings, 0 replies; 11+ messages in thread
From: Maciej W. Rozycki @ 2011-08-03 21:13 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
On Wed, 3 Aug 2011, Richard Sandiford wrote:
> > Where are we with the change below?
>
> I approved it here:
>
> http://sourceware.org/ml/binutils/2011-07/msg00049.html
>
> so please go ahead and commit.
Hmm, no idea why I missed it (or forgot, or whatever...) -- sorry about
the confusion. Applied now, thanks.
Maciej
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-08-03 21:13 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-23 13:39 [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code Maciej W. Rozycki
2011-06-29 21:26 ` Richard Sandiford
2011-07-02 1:20 ` Maciej W. Rozycki
2011-07-02 7:33 ` Richard Sandiford
2011-07-04 20:30 ` Maciej W. Rozycki
2011-07-04 22:20 ` Richard Sandiford
2011-07-05 0:28 ` Maciej W. Rozycki
2011-07-05 11:29 ` Richard Sandiford
2011-08-03 14:55 ` [ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode Maciej W. Rozycki
2011-08-03 19:47 ` Richard Sandiford
2011-08-03 21:13 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).