This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH 19/20] MIPS/GAS: Only set the MIPS16 flag if code produced


Hi Richard,

 Following our earlier correspondence:

On Mon, 26 Jul 2010, Richard Sandiford wrote:

> Message-ID: <876302kqvu.fsf@firetop.home>

> >> @@ -14813,6 +16230,8 @@ mips_elf_final_processing (void)
> >>       file_ase_mt is true.  */
> >>    if (file_ase_mips16)
> >>      elf_elfheader (stdoutput)->e_flags |= EF_MIPS_ARCH_ASE_M16;
> >> +  if (file_ase_micromips)
> >> +    elf_elfheader (stdoutput)->e_flags |= EF_MIPS_ARCH_ASE_MICROMIPS;
> >>  #if 0 /* XXX FIXME */
> >>    if (file_ase_mips3d)
> >>      elf_elfheader (stdoutput)->e_flags |= ???;
> >> 
> >> Do you really only want this flag to be set if -mmicromips was passed
> >> on the command line?  (Yes, the same concern applies to MIPS16 and MDMX.)
> >
> >  Fixing it up is easy, but I'm not sure what the original intent of these 
> > flags has been.  Do they mean:
> >
> > 1. I make use of the FOO feature and I assert it is available?
> >
> > 2. I make use of the FOO feature, but I may go and figure out if it's 
> >    available?
> >
> > 3. I make use of the FOO feature and want to prevent linking with any
> >    incompatible ones?
> >
> > Once we've determined the answer we may modify code accordingly or leave 
> > it as it is.  It looks to me the current approach matches #1 and the ELF 
> > program loader may refuse to execute the program if it sees a flag for a 
> > feature that is not supported by the processor to run on.  Any .set 
> > fragments within the program are guarded appropriately and hence they do 
> > not need to set the flag (as it would be for #2).  Then #3 is sort of 
> > tangential to the two others, but it does matter in this context, hence I 
> > mentioned it (i.e. if #3 was true, then I'd be much happier with #3 & #1 
> > than #3 & #2; otherwise I don't have a strong preference between #1 and 
> > #2).
> >
> >  Current arrangement is similar to that of file_mips_isa even though the 
> > ISA can be overridden by .set too and I think it makes sense to keep all 
> > these flags consistent.
> 
> I think MIPS16 and microMIPS are fundamentally different from the ISA,
> and even from MDMX.  IMO, it's reasonable to assume that, if the user is
> taking a MIPS III target as their base system, that they'll assemble
> with a MIPS III ISA flag.  I know others disagree, and think that such
> things should always be specified by pseudo-ops.  Even if you're of that
> opinion, though, it's easy in principle to set the flags to the minimum
> ISA required by the assembly source.
> 
> MIPS16 and microMIPS are different because they represent an operating
> mode that the processor can switch in and out of.  The fact that the
> assembly source contains both MIPS16 and non-MIPS16 code isn't really a
> good indicator that the code is designed to cope with non-MIPS16-capable
> systems.
> 
> We could live without deciding between #1, #2 and #3 until now because
> MIPS16 was the only feature like this.  Now that we've got two
> incompatible code-compression methods, I think it's more important.
> 
> I too would like to hear other opinions.  I'm just not convinced by
> the "it's similar to ISA selection" argument.  More below.
[...]
> >> -  /* If this is an odd-valued function symbol, assume it's a MIPS16 one.  */
> >> +  /* If this is an odd-valued function symbol, assume it's a MIPS16
> >> +     or microMIPS one.  */
> >>    if (ELF_ST_TYPE (elfsym->internal_elf_sym.st_info) == STT_FUNC
> >>        && (asym->value & 1) != 0)
> >>      {
> >>        asym->value--;
> >> -      elfsym->internal_elf_sym.st_other
> >> -	= ELF_ST_SET_MIPS16 (elfsym->internal_elf_sym.st_other);
> >> +      if (elf_elfheader (abfd)->e_flags & EF_MIPS_ARCH_ASE_MICROMIPS)
> >> +	elfsym->internal_elf_sym.st_other
> >> +	  = ELF_ST_SET_MICROMIPS (elfsym->internal_elf_sym.st_other);
> >> +      else
> >> +	elfsym->internal_elf_sym.st_other
> >> +	  = ELF_ST_SET_MIPS16 (elfsym->internal_elf_sym.st_other);
> >>      }
> >>  }
> >>  
> >> 
> >> So a file can't mix MIPS16 and microMIPS code?  We should probably
> >> detect that explicitly.  I'd like a clear statement of what the
> >> interoperability restrictions are.
> >> 
> >> This goes back to the question of when EF_MIPS_ARCH_ASE_MICROMIPS
> >> should be set (see previous reviews).
> >
> >  Background information first -- you can't have a piece of MIPS hardware, 
> > either real or simulated, with the MIPS16 ASE and the microMIPS ASE 
> > implemented both at a time.  This is a design assumption that allowed the 
> > ISA bit to be overloaded.
> >
> >  That obviously does not mean -- in principle -- that you can't have an 
> > executable (or one plus a mixture of shared libraries) where some 
> > functions are MIPS16 code and some are microMIPS code, either of which 
> > only called once the presence of the respective ASE has been determined.  
> > While a valid configuration this is also a rare exotic corner case -- I 
> > could imagine a piece of generic, processor-independent console/bootstrap 
> > firmware like this for example, but little beyond that.
> >
> >  We had a discussion about it and the conclusion was from the user's 
> > perspective the most beneficial configuration is one where mixing MIPS16 
> > and microMIPS code within a single executable is forbidden by default.  
> > The reason is a build-time error is always better than a run-time one 
> > (especially when the device has been placed out there in the field 
> > already; hopefully not an Ariane 5 rocket) and the case where mixing 
> > MIPS16 and microMIPS code would almost always happen is when the user 
> > picked the wrong library by mistake.  It is therefore most productive to 
> > fail at this point rather than later.
> 
> I agree.  In that case, though, I think it's important that we go for #1
> in your list above, otherwise this check doesn't really count for much.
> I think having a way of forcing interpretation #2 fits into the same
> category as...
> 
> >  A future enhancement could add assembler and linker switches to override 
> > this default and mix the two ASEs in a single executable. [FIXME]
> 
> ...this, which I agree is best left for future work.

and noting no further comments from anyone else despite your request 
here's my proposal to modify the MIPS16 ASE ELF file flag's semantics such 
that it is only set (by GAS) if actual MIPS16 code has been generated.  
That is with this change it is not enough to pass "-mips16" to GAS or emit 
data like this:

	.set	mips16
	.word	0

-- there actually has to be at least one instruction generated, e.g.:

	.set	mips16
	nop

 I have adjusted the existing test case and added a new one for proper 
coverage.  I have modified the test framework to allow one to add a 
leading "!" to a regexp to request inverse matching; I have found no 
native way to do this with TCL's regexp syntax.  There are currently no 
test cases with a leading "!" in any regexp, so this change should be safe 
without any further adjustments to the test suite, e.g. for other targets.  

2010-12-02  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* config/tc-mips.c (file_ase_mips16): Adjust comment.
	(append_insn): Update file_ase_mips16.
	(mips_after_parse_args): Don't set file_ase_mips16 here.

	gas/testsuite/
	* gas/mips/elf_ase_mips16.d: Update test for new MIPS16 ASE flag
	semantics.
	* gas/mips/elf_ase_mips16-1.d: New test.
	* gas/mips/nop.s: Source for the new test.
	* gas/mips/mips.exp: Run the new test.

	binutils/testsuite/
	* lib/binutils-common.exp (regexp_diff): Implement inverse 
	matching, requested by `!'.

 OK to apply?

  Maciej

binutils-gas-mips16-ase.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2010-12-01 21:05:58.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2010-12-01 21:05:59.000000000 +0000
@@ -282,8 +282,7 @@ unsigned long mips_cprmask[4];
 /* MIPS ISA we are using for this output file.  */
 static int file_mips_isa = ISA_UNKNOWN;
 
-/* True if -mips16 was passed or implied by arguments passed on the
-   command line (e.g., by -march).  */
+/* True if any MIPS16 code was produced.  */
 static int file_ase_mips16;
 
 #define ISA_SUPPORTS_MIPS16E (mips_opts.isa == ISA_MIPS32		\
@@ -2814,6 +2813,8 @@ append_insn (struct mips_cl_insn *ip, ex
   /* Mark instruction labels in mips16 mode.  */
   mips16_mark_labels ();
 
+  file_ase_mips16 |= mips_opts.mips16;
+
   prev_pinfo = history[0].insn_mo->pinfo;
   pinfo = ip->insn_mo->pinfo;
 
@@ -12018,7 +12019,6 @@ mips_after_parse_args (void)
 	     mips_cpu_info_from_isa (mips_opts.isa)->name);
 
   file_mips_isa = mips_opts.isa;
-  file_ase_mips16 = mips_opts.mips16;
   file_ase_mips3d = mips_opts.ase_mips3d;
   file_ase_mdmx = mips_opts.ase_mdmx;
   file_ase_smartmips = mips_opts.ase_smartmips;
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/elf_ase_mips16-1.d
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/elf_ase_mips16-1.d	2010-12-01 21:05:59.000000000 +0000
@@ -0,0 +1,8 @@
+# name: ELF MIPS16 ASE markings 1
+# source: nop.s
+# objdump: -p
+# as: -32 -mips16
+
+.*:.*file format.*mips.*
+private flags = [0-9a-f]*[4-7c-f]......: .*[[,]mips16[],].*
+
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/elf_ase_mips16.d
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/elf_ase_mips16.d	2010-12-01 21:05:40.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/elf_ase_mips16.d	2010-12-01 21:05:59.000000000 +0000
@@ -1,8 +1,8 @@
-# name: ELF MIPS16 ASE markings
+# name: ELF MIPS16 ASE markings 0
 # source: empty.s
 # objdump: -p
 # as: -32 -mips16
 
 .*:.*file format.*mips.*
-private flags = [0-9a-f]*[4-7c-f]......: .*[[,]mips16[],].*
+!private flags = .{1,8}: .*[[,]mips16[],].*
 
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/nop.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/nop.s	2010-12-01 21:05:59.000000000 +0000
@@ -0,0 +1 @@
+	nop
Index: binutils-fsf-trunk-quilt/binutils/testsuite/lib/binutils-common.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/binutils/testsuite/lib/binutils-common.exp	2010-12-01 21:05:40.000000000 +0000
+++ binutils-fsf-trunk-quilt/binutils/testsuite/lib/binutils-common.exp	2010-12-01 21:05:59.000000000 +0000
@@ -167,7 +167,9 @@ proc is_elf64 { binary_file } {
 #    REGEXP
 #        Skip all lines in FILE_1 until the first that matches REGEXP.
 #
-# Other # lines are comments.  Skip empty lines in both files.
+# Other # lines are comments.  Regexp lines starting with the `!' character
+# specify inverse matching (use `\!' for literal matching against a leading
+# `!').  Skip empty lines in both files.
 #
 # The first optional argument is a list of regexp substitutions of the form:
 #
@@ -235,12 +237,15 @@ proc regexp_diff { file_1 file_2 args } 
 		    set diff_pass 1
 		    break
 		}
+		set negated [expr { [string index $line_b 0] eq "!" }]
+		set line_bx [string range $line_b $negated end]
+		set n [expr { $negated ? "! " : "" }]
 		# Substitute on the reference.
 		foreach {name value} $ref_subst {
-		    regsub -- $name $line_b $value line_b
+		    regsub -- $name $line_bx $value line_bx
 		}
-		verbose "looking for \"^$line_b$\"" 3
-		while { ![regexp "^$line_b$" "$line_a"] } {
+		verbose "looking for $n\"^$line_bx$\"" 3
+		while { [expr [regexp "^$line_bx$" "$line_a"] == $negated] } {
 		    verbose "skipping    \"$line_a\"" 3
 		    if { [gets $file_a line_a] == $eof } {
 			set end_1 1
@@ -270,14 +275,18 @@ proc regexp_diff { file_1 file_2 args } 
 	    set differences 1
 	    break
 	} else {
+	    set negated [expr { [string index $line_b 0] eq "!" }]
+	    set line_bx [string range $line_b $negated end]
+	    set n [expr { $negated ? "! " : "" }]
+	    set s [expr { $negated ? "  " : "" }]
 	    # Substitute on the reference.
 	    foreach {name value} $ref_subst {
-		regsub -- $name $line_b $value line_b
+		regsub -- $name $line_bx $value line_bx
 	    }
-	    verbose "regexp \"^$line_b$\"\nline   \"$line_a\"" 3
-	    if { ![regexp "^$line_b$" "$line_a"] } {
+	    verbose "regexp $n\"^$line_bx$\"\nline   \"$line_a\"" 3
+	    if { [expr [regexp "^$line_bx$" "$line_a"] == $negated] } {
 		send_log "regexp_diff match failure\n"
-		send_log "regexp \"^$line_b$\"\nline   \"$line_a\"\n"
+		send_log "regexp $n\"^$line_bx$\"\nline   $s\"$line_a\"\n"
 		verbose "regexp_diff match failure\n" 3
 		set differences 1
 	    }
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp	2010-12-01 21:05:48.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp	2010-12-01 21:05:59.000000000 +0000
@@ -744,7 +744,10 @@ if { [istarget mips*-*-vxworks*] } {
 	run_dump_test "elf_arch_mips64r2"
 
 	# Verify that ASE markings are handled properly.
-	if { !$no_mips16 } { run_dump_test "elf_ase_mips16" }
+	if { !$no_mips16 } {
+	    run_dump_test "elf_ase_mips16"
+	    run_dump_test "elf_ase_mips16-1"
+	}
 
  	run_dump_test "mips-gp32-fp32-pic"
  	run_dump_test "mips-gp32-fp64-pic"


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]