RE: [Patch] Gas support for MIPS Compact EH

["Moore, Catherine" <Catherine_Moore@mentor.com>]

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Wednesday, March 19, 2014 7:46 PM
> To: Moore, Catherine
> Cc: Schmidt, Bernd; binutils@sourceware.org
> Subject: Re: [Patch] Gas support for MIPS Compact EH
> 
> "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> > I've been working on getting ".word X-." to work in the assembler.  It looks
> like I can produce the relocation, but I'm seeing failures for  the lui-1.s and lui-
> 2.s tests.
> > I've added these two definitions to tc-mips.h:
> >
> > +#define DIFF_EXPR_OK
> > +#define md_register_arithmetic 0
> >
> > The first thing is that I'm not sure if the setting for md_register_arithmetic
> is correct.  The MIPS port currently uses the default of 1 (that's got to be
> wrong).
> > There is interaction between DIFF_EXPR_OK, md_register_arithmetic and
> the FORCE_RELOC macros that is confusing.
> >
> > This definition of TC_FORCE_RELOCATION_SUB_LOCAL (write.c) needs to
> have md_register_arithmetic defined to zero, when DIFF_EXPR_OK is set.
> >
> > #ifndef TC_FORCE_RELOCATION_SUB_LOCAL
> > #ifdef DIFF_EXPR_OK
> > #define TC_FORCE_RELOCATION_SUB_LOCAL(FIX, SEG) \
> >   (!md_register_arithmetic && (SEG) == reg_section) #else #define
> > TC_FORCE_RELOCATION_SUB_LOCAL(FIX, SEG) 1 #endif #endif
> >
> > Needs to have md_register_arithmetic set to zero if DIFF_EXPR_OK is set.
> 
> I think we should leave md_register_arithmetic as it is.  The
> "(SEG) == reg_section" means that the special handling only applies to
> "register minus something" expressions.
> 
> > Assuming that defining md_register_arithmetic to zero is the correct thing
> to do, then the error message in md_pcrel_from looks wrong:
> >
> > long
> > md_pcrel_from (fixS *fixP)
> > {
> >   valueT addr = fixP->fx_where + fixP->fx_frag->fr_address;
> >   switch (fixP->fx_r_type)
> >     {
> >     case BFD_RELOC_MICROMIPS_7_PCREL_S1:
> >     case BFD_RELOC_MICROMIPS_10_PCREL_S1:
> >       /* Return the address of the delay slot.  */
> >       return addr + 2;
> >
> >     case BFD_RELOC_MICROMIPS_16_PCREL_S1:
> >     case BFD_RELOC_MICROMIPS_JMP:
> >     case BFD_RELOC_16_PCREL_S2:
> >     case BFD_RELOC_MIPS_JMP:
> >       /* Return the address of the delay slot.  */
> >       return addr + 4;
> >
> >     case BFD_RELOC_32_PCREL:
> >       return addr;
> >
> >     default:
> >       /* We have no relocation type for PC relative MIPS16 instructions.  */
> >       if (fixP->fx_addsy && S_GET_SEGMENT (fixP->fx_addsy) != now_seg)
> >         as_bad_where (fixP->fx_file, fixP->fx_line,
> >                       _("PC relative MIPS16 instruction references"
> >                         " a different section"));
> >       return addr;
> >     }
> > }
> >
> >
> > fixup_segment now treats many relocs as "possibly pc-relative" and waits
> for md_apply_fix to make the call.
> > Should the default be to return addr for any reloc that's encountered and
> try to process errors later?  I don't have any state on how a MIPS16 reloc
> ends up here.
> 
> The change comes from:
> 
>     https://www.sourceware.org/ml/binutils/2006-11/msg00029.html
> 
> but I'm not sure which case it was handling.
> 
> I agree it'd make sense to check this elsewhere.  I think the best place would
> be...
> 
> > Then there is an assert in md_apply_fix that assumes a pc relative reloc is
> one of   a handful of relocs.  The fx_pcrel field needs to be reset for
> relocations that aren't pc-relative.  Is md_apply_fix the correct place to do
> that?
> 
> ...instead of this assert.
> 
> > Then the test cases in question.   This is lui-1.s:
> >
> >         .text
> > foo:
> >         lui     $2, -1
> >         lui     $2, 65536
> >         lui     $2, 0x10000000000000000
> >         lui     $2, $3
> >         lui     $2, ($3)
> >         lui     $2, 0+$3  <<--- this statement
> >         lui     $2, (($3))
> >
> > Old error message:
> > lui-1.s:10: Error: register value used as expression `lui $2,0+$3'
> >
> > New error message:
> > None, for the entire test case, but a new file with just the offending line
> causes a assertion failure in bfd_elf32_swap_symbol_out.  (That's wrong, I
> need to track it down).
> 
> Yeah, this is a consequence of changing md_register_arithmetic.
> The old behaviour was right.
> 
> > Then lui-2.s:
> >
> >         .text
> > foo:
> >         lui     $2, bar - foo
> >         lui     $2, baz - bar
> >         lui     $2, foo - baz
> >         lui     $2, bar / baz
> >
> > Should any of these be supported now?
> >
> > Old error messages:
> > lui-2.s:10: Error: invalid operands (*UND* and *UND* sections) for `/'
> > lui-2.s:7: Error: can't resolve `bar' {*UND* section} - `foo' {.text
> > section}
> > lui-2.s:8: Error: can't resolve `baz' {*UND* section} - `bar' {*UND*
> > section}
> > lui-2.s:9: Error: can't resolve `.text' {.text section} - `baz' {*UND*
> > section}
> >
> > New error messages:
> > lui-2.s:10: Error: invalid operands (*UND* and *UND* sections) for `/'
> > lui-2.s:8: Error: can't resolve `baz' {*UND* section} - `bar' {*UND*
> > section}
> > lui-2.s:9: Error: can't resolve `.text' {.text section} - `baz' {*UND*
> > section}
> >
> > Notice that lui $2 bar-foo seems to be accepted.  Should it be?
> 
> No, the old behaviour was right here too, since we don't have a PC-relative
> form of R_MIPS_LO16.  The new error you mentioned should catch it.
> 
> How about the patch below?  Tested on various MIPSy targets against the
> binutils testsuite, but nothing beyond that so far.
> 

Yes, this patch looks good.  I lightly tested it and it appears to DTRT.
Thanks for helping to sort this out.
Catherine


> 
> gas/
> 	* config/tc-mips.h (DIFF_EXPR_OK, CFI_DIFF_EXPR_OK): Define.
> 	* config/tc-mips.c (md_pcrel_from): Remove error message.
> 	(md_apply_fix): Convert PC-relative BFD_RELOC_32s to
> 	BFD_RELOC_32_PCREL.  Report a specific error message for
> unhandled
> 	PC-relative expressions.  Handle BFD_RELOC_8.
> 
> gas/testsuite/
> 	* gas/all/gas.exp: Remove XFAIL of forward.d for MIPS.
> 	* gas/mips/pcrel-1.s, gas/mips/pcrel-1.d, gas/mips/pcrel-2.s,
> 	gas/mips/pcrel-2.d, gas/mips/pcrel-3.s, gas/mips/pcrel-3.l,
> 	gas/mips/pcrel-4.s, gas/mips/pcrel-4-32.d, gas/mips/pcrel-4-n32.d,
> 	gas/mips/pcrel-4-64.d: New tests.
> 	* gas/mips/mips.exp: Run them.
> 	* gas/mips/lui-2.l: Tweak error message for line 7.
> 
> ld/testsuite/
> 	* ld-elf/merge.d: Remove MIPS XFAIL.
> 
> Index: gas/config/tc-mips.h
> ==========================================================
> =========
> --- gas/config/tc-mips.h	2014-03-19 22:44:18.584854477 +0000
> +++ gas/config/tc-mips.h	2014-03-19 23:43:50.695861764 +0000
> @@ -189,4 +189,9 @@ extern int tc_mips_regname_to_dw2regnum
> #define DWARF2_DEFAULT_RETURN_COLUMN 31  #define
> DWARF2_CIE_DATA_ALIGNMENT (-4)
> 
> +#define DIFF_EXPR_OK
> +/* We define DIFF_EXPR_OK because of R_MIPS_PC32, but we have no
> +   64-bit form for n64 CFIs.  */
> +#define CFI_DIFF_EXPR_OK 0
> +
>  #endif /* TC_MIPS */
> Index: gas/config/tc-mips.c
> ==========================================================
> =========
> --- gas/config/tc-mips.c	2014-03-19 22:44:18.584854477 +0000
> +++ gas/config/tc-mips.c	2014-03-19 23:12:27.340621113 +0000
> @@ -14057,15 +14057,7 @@ md_pcrel_from (fixS *fixP)
>        /* Return the address of the delay slot.  */
>        return addr + 4;
> 
> -    case BFD_RELOC_32_PCREL:
> -      return addr;
> -
>      default:
> -      /* We have no relocation type for PC relative MIPS16 instructions.  */
> -      if (fixP->fx_addsy && S_GET_SEGMENT (fixP->fx_addsy) != now_seg)
> -	as_bad_where (fixP->fx_file, fixP->fx_line,
> -		      _("PC relative MIPS16 instruction references"
> -			" a different section"));
>        return addr;
>      }
>  }
> @@ -14262,13 +14254,38 @@ md_apply_fix (fixS *fixP, valueT *valP,
>    unsigned long insn;
>    reloc_howto_type *howto;
> 
> -  /* We ignore generic BFD relocations we don't know about.  */
> -  howto = bfd_reloc_type_lookup (stdoutput, fixP->fx_r_type);
> -  if (! howto)
> -    return;
> +  if (fixP->fx_pcrel)
> +    switch (fixP->fx_r_type)
> +      {
> +      case BFD_RELOC_16_PCREL_S2:
> +      case BFD_RELOC_MICROMIPS_7_PCREL_S1:
> +      case BFD_RELOC_MICROMIPS_10_PCREL_S1:
> +      case BFD_RELOC_MICROMIPS_16_PCREL_S1:
> +      case BFD_RELOC_32_PCREL:
> +	break;
> +
> +      case BFD_RELOC_32:
> +	fixP->fx_r_type = BFD_RELOC_32_PCREL;
> +	break;
> +
> +      default:
> +	as_bad_where (fixP->fx_file, fixP->fx_line,
> +		      _("PC-relative reference to a different section"));
> +	break;
> +      }
> +
> +  /* Handle BFD_RELOC_8, since it's easy.  Punt on other bfd relocations
> +     that have no MIPS ELF equivalent.  */
> +  if (fixP->fx_r_type != BFD_RELOC_8)
> +    {
> +      howto = bfd_reloc_type_lookup (stdoutput, fixP->fx_r_type);
> +      if (!howto)
> +	return;
> +    }
> 
>    gas_assert (fixP->fx_size == 2
>  	      || fixP->fx_size == 4
> +	      || fixP->fx_r_type == BFD_RELOC_8
>  	      || fixP->fx_r_type == BFD_RELOC_16
>  	      || fixP->fx_r_type == BFD_RELOC_64
>  	      || fixP->fx_r_type == BFD_RELOC_CTOR @@ -14280,12 +14297,6
> @@ md_apply_fix (fixS *fixP, valueT *valP,
> 
>    buf = fixP->fx_frag->fr_literal + fixP->fx_where;
> 
> -  gas_assert (!fixP->fx_pcrel || fixP->fx_r_type ==
> BFD_RELOC_16_PCREL_S2
> -	      || fixP->fx_r_type == BFD_RELOC_MICROMIPS_7_PCREL_S1
> -	      || fixP->fx_r_type == BFD_RELOC_MICROMIPS_10_PCREL_S1
> -	      || fixP->fx_r_type == BFD_RELOC_MICROMIPS_16_PCREL_S1
> -	      || fixP->fx_r_type == BFD_RELOC_32_PCREL);
> -
>    /* Don't treat parts of a composite relocation as done.  There are two
>       reasons for this:
> 
> @@ -14435,6 +14446,7 @@ md_apply_fix (fixS *fixP, valueT *valP,
>      case BFD_RELOC_32:
>      case BFD_RELOC_32_PCREL:
>      case BFD_RELOC_16:
> +    case BFD_RELOC_8:
>        /* If we are deleting this reloc entry, we must fill in the
>  	 value now.  This can happen if we have a .word which is not
>  	 resolved when it appears but is later defined.  */
> Index: gas/testsuite/gas/all/gas.exp
> ==========================================================
> =========
> --- gas/testsuite/gas/all/gas.exp	2014-03-19 21:26:29.344209431 +0000
> +++ gas/testsuite/gas/all/gas.exp	2014-03-19 23:28:58.492008472 +0000
> @@ -99,7 +99,7 @@ case $target_triplet in {
>      default {
>  	# Some targets don't manage to resolve BFD_RELOC_8 for constants.
>  	setup_xfail "alpha*-*-*" "*c30*-*-*" "*c4x*-*-*" \
> -	    "d\[13\]0v*-*-*" "i860-*-*" "mips*-*-*" \
> +	    "d\[13\]0v*-*-*" "i860-*-*" \
>  	    "nds32*-*-*" "pdp11-*-*" "xtensa*-*-*"
>  	run_dump_test forward
>      }
> Index: gas/testsuite/gas/mips/pcrel-1.s
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-1.s	2014-03-19 23:12:27.341621122 +0000
> @@ -0,0 +1,13 @@
> +	.text
> +	.ent	func
> +func:
> +	lui	$4,%hi(foo-.)
> +	addiu	$4,%lo(foo-.)
> +	.end	func
> +
> +	.space	0x8008
> +
> +	.ent	foo
> +foo:
> +	nop
> +	.end	foo
> Index: gas/testsuite/gas/mips/pcrel-1.d
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-1.d	2014-03-19 23:12:27.341621122 +0000
> @@ -0,0 +1,14 @@
> +#objdump: -dr
> +#name: Locally-resolvable PC-relative code references
> +
> +.*:     file format .*
> +
> +Disassembly of section .text:
> +
> +00000000 <func>:
> +       0:	3c040001 	lui	a0,0x1
> +       4:	2484800c 	addiu	a0,a0,-32756
> +	...
> +
> +00008010 <foo>:
> +#pass
> Index: gas/testsuite/gas/mips/pcrel-2.s
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-2.s	2014-03-19 23:12:27.342621131 +0000
> @@ -0,0 +1,7 @@
> +	.data
> +	.byte	0xff
> +	.byte	frob-.
> +	.half	frob-.
> +	.word	frob-.
> +	.quad	frob-.
> +frob:
> Index: gas/testsuite/gas/mips/pcrel-2.d
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-2.d	2014-03-19 23:12:27.342621131 +0000
> @@ -0,0 +1,8 @@
> +#objdump: -s
> +#name: Locally-resolvable PC-relative data references
> +#as: -EB
> +
> +#...
> +Contents of section \.data:
> + 0000 ff0f000e 0000000c 00000000 00000008  .* #pass
> Index: gas/testsuite/gas/mips/pcrel-3.s
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-3.s	2014-03-19 23:12:27.342621131 +0000
> @@ -0,0 +1,11 @@
> +	.text
> +	.ent	func
> +func:
> +	lui	$4,%hi(foo-.)
> +	addiu	$4,%lo(foo-.)
> +	lw	$4,%got(foo-.)($gp)
> +	.end	func
> +
> +	.byte	foo-.
> +	.half	foo-.
> +	.quad	foo-.
> Index: gas/testsuite/gas/mips/pcrel-3.l
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-3.l	2014-03-19 23:12:27.342621131 +0000
> @@ -0,0 +1,7 @@
> +.*: Assembler messages:
> +.*:4: Error: PC-relative reference to a different section
> +.*:5: Error: PC-relative reference to a different section
> +.*:6: Error: PC-relative reference to a different section
> +.*:9: Error: PC-relative reference to a different section
> +.*:10: Error: PC-relative reference to a different section
> +.*:11: Error: PC-relative reference to a different section
> Index: gas/testsuite/gas/mips/pcrel-4.s
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-4.s	2014-03-19 23:12:27.343621139 +0000
> @@ -0,0 +1,6 @@
> +	.data
> +	.word	foo-.
> +	.word	foo-(.-4)
> +	.word	foo+8-.
> +	.word	foo-.-16
> +
> Index: gas/testsuite/gas/mips/pcrel-4-32.d
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-4-32.d	2014-03-19
> 23:12:27.342621131 +0000
> @@ -0,0 +1,18 @@
> +#objdump: -sr
> +#name: Valid cross-section PC-relative references (o32)
> +#as: -32 -EB
> +#source: pcrel-4.s
> +
> +.*:     file format .*
> +
> +RELOCATION RECORDS FOR \[\.data\]:
> +OFFSET   TYPE              VALUE
> +00000000 R_MIPS_PC32       foo
> +00000004 R_MIPS_PC32       foo
> +00000008 R_MIPS_PC32       foo
> +0000000c R_MIPS_PC32       foo
> +
> +#...
> +Contents of section \.data:
> + 0000 00000000 00000004 00000008 fffffff0  ................
> +#pass
> Index: gas/testsuite/gas/mips/pcrel-4-n32.d
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-4-n32.d	2014-03-19
> 23:26:47.946909763 +0000
> @@ -0,0 +1,13 @@
> +#objdump: -r
> +#name: Valid cross-section PC-relative references (n32)
> +#as: -n32 -mips3
> +#source: pcrel-4.s
> +
> +.*:     file format .*
> +
> +RELOCATION RECORDS FOR \[\.data\]:
> +OFFSET   TYPE              VALUE
> +00000000 R_MIPS_PC32       foo
> +00000004 R_MIPS_PC32       foo\+0x00000004
> +00000008 R_MIPS_PC32       foo\+0x00000008
> +0000000c R_MIPS_PC32       foo-0x00000010
> Index: gas/testsuite/gas/mips/pcrel-4-64.d
> ==========================================================
> =========
> --- /dev/null	2014-03-15 14:12:32.004596490 +0000
> +++ gas/testsuite/gas/mips/pcrel-4-64.d	2014-03-19
> 23:26:58.591999264 +0000
> @@ -0,0 +1,21 @@
> +#objdump: -r
> +#name: Valid cross-section PC-relative references (n64)
> +#as: -64 -mips3
> +#source: pcrel-4.s
> +
> +.*:     file format .*
> +
> +RELOCATION RECORDS FOR \[\.data\]:
> +OFFSET           TYPE              VALUE
> +0+000 R_MIPS_PC32       foo
> +0+000 R_MIPS_NONE       \*ABS\*
> +0+000 R_MIPS_NONE       \*ABS\*
> +0+004 R_MIPS_PC32       foo\+0x0+004
> +0+004 R_MIPS_NONE       \*ABS\*\+0x0+004
> +0+004 R_MIPS_NONE       \*ABS\*\+0x0+004
> +0+008 R_MIPS_PC32       foo\+0x0+008
> +0+008 R_MIPS_NONE       \*ABS\*\+0x0+008
> +0+008 R_MIPS_NONE       \*ABS\*\+0x0+008
> +0+00c R_MIPS_PC32       foo-0x0+010
> +0+00c R_MIPS_NONE       \*ABS\*-0x0+010
> +0+00c R_MIPS_NONE       \*ABS\*-0x0+010
> Index: gas/testsuite/gas/mips/mips.exp
> ==========================================================
> =========
> --- gas/testsuite/gas/mips/mips.exp	2014-03-19 22:44:18.584854477 +0000
> +++ gas/testsuite/gas/mips/mips.exp	2014-03-19 23:12:27.341621122 +0000
> @@ -1169,4 +1169,13 @@ if { [istarget mips*-*-vxworks*] } {
>      run_dump_test_arches "msa64"	[mips_arch_list_matching mips64r2]
>      run_dump_test_arches "msa-relax"	[mips_arch_list_matching mips32r2]
>      run_dump_test_arches "msa-branch"	[mips_arch_list_matching
> mips32r2]
> +
> +    run_dump_test "pcrel-1"
> +    run_dump_test "pcrel-2"
> +    run_list_test "pcrel-3" "" "Invalid cross-section PC-relative references"
> +    run_dump_test "pcrel-4-32"
> +    if $has_newabi {
> +	run_dump_test "pcrel-4-n32"
> +	run_dump_test "pcrel-4-64"
> +    }
>  }
> Index: gas/testsuite/gas/mips/lui-2.l
> ==========================================================
> =========
> --- gas/testsuite/gas/mips/lui-2.l	2014-03-19 22:44:18.584854477 +0000
> +++ gas/testsuite/gas/mips/lui-2.l	2014-03-19 23:12:27.343621139 +0000
> @@ -1,5 +1,5 @@
>  .*\.s: Assembler messages:
>  .*\.s:10: Error: invalid operands \(\*UND\* and \*UND\* sections\) for `/'
> -.*\.s:7: Error: can't resolve `bar' {\*UND\* section} - `foo' {\.text section}
> +.*\.s:7: Error: PC-relative reference to a different section
>  .*\.s:8: Error: can't resolve `baz' {\*UND\* section} - `bar' {\*UND\* section}
>  .*\.s:9: Error: can't resolve `\.text' {\.text section} - `baz' {\*UND\* section}
> Index: ld/testsuite/ld-elf/merge.d
> ==========================================================
> =========
> --- ld/testsuite/ld-elf/merge.d	2013-11-13 21:08:01.200582091 +0000
> +++ ld/testsuite/ld-elf/merge.d	2014-03-19 23:30:33.635810673 +0000
> @@ -4,7 +4,7 @@
>  #xfail: "arc-*-*" "avr-*-*" "bfin-*-*" "cr16-*-*" "cris*-*-*" "crx-*-*" "d10v-
> *-*" "d30v-*-*"
>  #xfail: "dlx-*-*" "fr30-*-*" "frv-*-*" "hppa*64*-*-*" "h8300-*-*" "score-*-
> *"
>  #xfail: "i370-*-*" "i860-*-*" "i960-*-*" "ip2k-*-*" "iq2000-*-*" "lm32-*-*"
> -#xfail: "mcore-*-*" "mn102*-*-*" "mips*-*-*" "ms1-*-*" "mep-*-*"
> +#xfail: "mcore-*-*" "mn102*-*-*" "ms1-*-*" "mep-*-*"
>  #xfail: "or32-*-*" "pj-*-*" "sparc*-*-*" "tic6x-*-*" "vax-*-*" "xstormy16-*-
> *"
>  #xfail: "xtensa*-*-*" "metag-*-*"
>