| Message ID | alpine.DEB.1.10.1205081014470.18334@tp.orcam.me.uk |
|---|---|
| State | Accepted |
| Headers | show |
"Maciej W. Rozycki" <macro@codesourcery.com> writes: > gcc-mips16-gp-pseudo-loc.patch > Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.c > =================================================================== > --- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.c 2012-05-02 23:42:46.185566469 +0100 > +++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.c 2012-05-03 18:55:28.775580939 +0100 > @@ -2622,7 +2622,8 @@ mips16_gp_pseudo_reg (void) > scan = NEXT_INSN (scan); > > insn = gen_load_const_gp (cfun->machine->mips16_gp_pseudo_rtx); > - emit_insn_after (insn, scan); > + insn = emit_insn_after (insn, scan); > + INSN_LOCATOR (insn) = 0; > > pop_topmost_sequence (); > } An alternative would be to use prologue_locator, like ARM does. I'm not sure whether that's an improvement though, so the patch is OK as-is, thanks. Richard
On Tue, 8 May 2012, Richard Sandiford wrote: > > gcc-mips16-gp-pseudo-loc.patch > > Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.c > > =================================================================== > > --- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.c 2012-05-02 23:42:46.185566469 +0100 > > +++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.c 2012-05-03 18:55:28.775580939 +0100 > > @@ -2622,7 +2622,8 @@ mips16_gp_pseudo_reg (void) > > scan = NEXT_INSN (scan); > > > > insn = gen_load_const_gp (cfun->machine->mips16_gp_pseudo_rtx); > > - emit_insn_after (insn, scan); > > + insn = emit_insn_after (insn, scan); > > + INSN_LOCATOR (insn) = 0; > > > > pop_topmost_sequence (); > > } > > An alternative would be to use prologue_locator, like ARM does. Is this instruction guaranteed to be emitted once per function only? > I'm not sure whether that's an improvement though, so the patch > is OK as-is, thanks. Applied now, thanks. Maciej
"Maciej W. Rozycki" <macro@codesourcery.com> writes: > On Tue, 8 May 2012, Richard Sandiford wrote: >> > gcc-mips16-gp-pseudo-loc.patch >> > Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.c >> > =================================================================== >> > --- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.c 2012-05-02 23:42:46.185566469 +0100 >> > +++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.c 2012-05-03 18:55:28.775580939 +0100 >> > @@ -2622,7 +2622,8 @@ mips16_gp_pseudo_reg (void) >> > scan = NEXT_INSN (scan); >> > >> > insn = gen_load_const_gp (cfun->machine->mips16_gp_pseudo_rtx); >> > - emit_insn_after (insn, scan); >> > + insn = emit_insn_after (insn, scan); >> > + INSN_LOCATOR (insn) = 0; >> > >> > pop_topmost_sequence (); >> > } >> >> An alternative would be to use prologue_locator, like ARM does. > > Is this instruction guaranteed to be emitted once per function only? Yes.
Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.c =================================================================== --- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.c 2012-05-02 23:42:46.185566469 +0100 +++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.c 2012-05-03 18:55:28.775580939 +0100 @@ -2622,7 +2622,8 @@ mips16_gp_pseudo_reg (void) scan = NEXT_INSN (scan); insn = gen_load_const_gp (cfun->machine->mips16_gp_pseudo_rtx); - emit_insn_after (insn, scan); + insn = emit_insn_after (insn, scan); + INSN_LOCATOR (insn) = 0; pop_topmost_sequence (); }
Hi, I have been investigating some failures in the MIPS16 GDB test suite that appeared to me that was caused by some weird out-of place line information. For example we get this in gdb.base/break.exp, where a breakpoint is set at main and the beginning of the function (after removing some "decorations") looks like this: int main (int argc, char **argv, char **envp) { if (argc == 12345) { /* an unlikely value < 2^16, in case uninited */ /* set breakpoint 6 here */ fprintf (stderr, "usage: factorial <number>\n"); return 1; } [...] The test case quite reasonably expects the breakpoint to hit at: if (argc == 12345) { [...] which indeed happens with standard MIPS testing (at -O0). However with MIPS16 testing (also at -O0) the breakpoint instead hits here: fprintf (stderr, "usage: factorial <number>\n"); which of course scores as a test case failure and would confuse any human being in a real debug session too. So what happens in this case turned out to be this: [...] .text .align 2 .globl main .LFB1 = . .loc 1 88 0 .cfi_startproc .set mips16 .ent main .type main, @function main: .frame $17,8,$31 # vars= 0, regs= 2/0, args= 16, gp= 0 .mask 0x80020000,-4 .fmask 0x00000000,0 save $4-$6,24,$17,$31 # 85 *mips16e_save_restore [length = 4] .cfi_def_cfa_offset 24 .cfi_offset 31, -4 .cfi_offset 17, -8 addiu $17,$sp,16 # 87 *addsi3_mips16/2 [length = 2] .cfi_def_cfa 17, 8 .loc 1 94 0 move $2,$28 # 12 *movsi_mips16/6 [length = 2] .loc 1 93 0 lw $3,8($17) # 69 *movsi_mips16/8 [length = 2] move $24,$3 # 8 *movsi_mips16/2 [length = 2] move $3,$24 # 70 *movsi_mips16/3 [length = 2] cmpi $3,12345 # 9 *mips.md:2835/2 [length = 4] btnez .L4 # 10 *branch_equalitysi_mips16/2 [length = 4] .loc 1 94 0 lw $3,%gprel(_impure_ptr)($2) # 71 *movsi_mips16/8 [length = 4] move $24,$3 # 13 *movsi_mips16/2 [length = 2] move $2,$24 # 72 *movsi_mips16/3 [length = 2] lw $2,12($2) # 73 *movsi_mips16/8 [length = 2] move $24,$2 # 14 *movsi_mips16/2 [length = 2] lw $4,.L6 # 15 *movsi_mips16/8 [length = 4] li $5,1 # 16 *movsi_mips16/4 [length = 2] li $6,27 # 17 *movsi_mips16/4 [length = 2] move $7,$24 # 18 *movsi_mips16/3 [length = 2] jal fwrite # 19 call_value_internal/2 [length = 6] .loc 1 95 0 li $3,1 # 74 *movsi_mips16/4 [length = 2] move $24,$3 # 20 *movsi_mips16/2 [length = 2] b .L5 # 65 *jump_mips16 [length = 6] .L4: .loc 1 97 0 [...] -- notice the ".loc 1 94 0" directive just after the prologue, spanning just a single instruction that copies $gp to a directly-accessible register, followed by ".loc 1 93 0" that corresponds to the first actual line of main. The conditional block then follows, marked with another ".loc 1 94 0" annotation. I have tracked it down to mips16_gp_pseudo_reg -- an instruction is generated there to access the real $gp and moved backwards as far as possible (IIUC). I think this instruction is not really associated with anything specific line 94 does as far as the high-level language is concerned and is merely an ABI implementation detail. It's also going to be reused for any other accesses to GP throughout as long as the auxiliary register is live. I think it should really be treated as a part of the prologue or similarly to the "lw $gp, GPSLOT($sp)" o32 subroutine call GP restoration step. I propose therefore to remove line annotation associated with this instruction so that it's merged with the prologue or any adjacent source line so as not to confuse the GDB test suite and, more importantly, the user. As a result of this change the extra ".loc 1 94 0" directive at the beginning of main is not produced anymore. This change fixes 32 regressions in the GDB test suite for the MIPS16 multilibs (either endianness) and the mips-sde-elf target: PASS: gdb.base/break.exp: Temporary breakpoint info PASS: gdb.base/break.exp: breakpoint info PASS: gdb.base/break.exp: run until function breakpoint PASS: gdb.base/callfuncs.exp: next to t_double_values PASS: gdb.base/callfuncs.exp: next to t_structs_c PASS: gdb.base/condbreak.exp: breakpoint info PASS: gdb.base/define.exp: use hook-stop command PASS: gdb.base/define.exp: use user command: nextwhere PASS: gdb.base/hbreak2.exp: hardware breakpoint insertion PASS: gdb.base/hbreak2.exp: run until function breakpoint PASS: gdb.base/maint.exp: maint info breakpoints PASS: gdb.base/pointers.exp: and post-increment PASS: gdb.base/pointers.exp: and pre-decrement PASS: gdb.base/pointers.exp: continuing after dummy() PASS: gdb.base/pointers.exp: pointer assignment PASS: gdb.base/pointers.exp: pointer assignment [#2] PASS: gdb.base/pointers.exp: print array element through pointer PASS: gdb.base/pointers.exp: print array element through pointer #2 PASS: gdb.base/pointers.exp: print object pointed to PASS: gdb.base/pointers.exp: print object pointed to #2 PASS: gdb.base/pointers.exp: print through ptr to ptr PASS: gdb.base/sepdebug.exp: Temporary breakpoint info PASS: gdb.base/sepdebug.exp: breakpoint info PASS: gdb.base/sepdebug.exp: run until function breakpoint PASS: gdb.base/watchpoint.exp: gloabl_ptr_ptr next PASS: gdb.base/watchpoint.exp: global_ptr next PASS: gdb.base/watchpoint.exp: next over buffer set PASS: gdb.base/watchpoint.exp: next over global_ptr_ptr buffer set PASS: gdb.base/watchpoint.exp: next over global_ptr_ptr init PASS: gdb.base/watchpoint.exp: next over global_ptr_ptr pointer advance PASS: gdb.base/watchpoint.exp: next over ptr init PASS: gdb.cp/shadow.exp: Print class x shadowing global x It also fixes two G++ failures for same: PASS: g++.dg/debug/dwarf2/redeclaration-1.C scan-assembler-times [^\n\r]*\\(DIE[^\n\r]*DW_TAG_lexical_block\\)[\n\r]{1,2}[^\n\r]*DW_AT_low_pc[\n\r]{1,2}[^\n\r]*DW_AT_high_pc[\n\r]{1,2}[^\n\r]*\\(DIE [^\n\r]*DW_TAG_variable\\)[\n\r]{1,2}[^\n\r]*DW_AT_name 2 PASS: g++.dg/debug/dwarf2/redeclaration-1.C scan-assembler-times [^\n\r]*\\(DIE[^\n\r]*DW_TAG_lexical_block\\)[\n\r]{1,2}[^\n\r]*DW_AT_low_pc[\n\r]{1,2}[^\n\r]*DW_AT_high_pc[\n\r]{1,2}[^\n\r]*\\(DIE [^\n\r]*DW_TAG_variable\\)[\n\r]{1,2}[^\n\r]*DW_AT_name 2 [#2] Additionally I have regression-tested it with the gcc, g++, libstdc++ and gdb test suites for the mips-sde-elf target, MIPS32 and MIPS16 multilibs, both endiannesses each. I tested the little-endian MIPS64 n64 multilib as well, with the gcc, g++ and libstdc++ test suites (gdb or big endianness currently untested due to a limitation in my test environment, sorry). OK to apply? 2012-05-08 Maciej W. Rozycki <macro@codesourcery.com> gcc/ * config/mips/mips.c (mips16_gp_pseudo_reg): Remove line information from the instruction produced. Maciej gcc-mips16-gp-pseudo-loc.patch