MIPS16: Remove DWARF-2 location information from GP accesses
diff mbox

Message ID alpine.DEB.1.10.1205081014470.18334@tp.orcam.me.uk
State Accepted
Headers show

Commit Message

Maciej W. Rozycki May 8, 2012, 10:04 a.m. UTC
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

Comments

Richard Sandiford May 8, 2012, 5:48 p.m. UTC | #1
"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
Maciej W. Rozycki May 9, 2012, 9:44 a.m. UTC | #2
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
Richard Sandiford May 9, 2012, 9:47 a.m. UTC | #3
"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.

Patch
diff mbox

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 ();
     }