Re: [PATCH] MIPS: microMIPS ASE support

[Richard Sandiford <rdsandiford at googlemail dot com>]

Hi Maciej,

Thanks for the huge effort in updating this patch.  Aside from the
gas macro handling (see below) it's looking much better, and I don't
think there are many issues blocking it going in.  I'll try to find
time to read over the patch properly in the next week or so.

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> > > > 	(A_BFD_RELOC_HI16_S, A_BFD_RELOC_HI16, A_BFD_RELOC_LO16): New
>> > > > 	relocation wrapper macros.
>> > > > 	(A_BFD_RELOC_GPREL16): Likewise.
>> > > > 	(A_BFD_RELOC_MIPS_GOT16, A_BFD_RELOC_MIPS_GOT_HI16): Likewise.
>> > > > 	(A_BFD_RELOC_MIPS_GOT_LO16, A_BFD_RELOC_MIPS_HIGHEST): Likewise.
>> > > > 	(A_BFD_RELOC_MIPS_HIGHER, A_BFD_RELOC_MIPS_GOT_DISP): Likewise.
>> > > > 	(A_BFD_RELOC_MIPS_GOT_PAGE, A_BFD_RELOC_MIPS_GOT_OFST): 
>> > Likewise.
>> > > > 	(A_BFD_RELOC_MIPS_SUB, A_BFD_RELOC_MIPS_JALR): Likewise.
>> > > 
>> > > Did you consider doing the translation from non-microMIPS to
>> > > microMIPS in the macro_* functions, rather than in their callers?
>> > > I fear it'll be too easy to accidentally forget to use 
>> > A_BFD_* in future.
>> > 
>> >  Agreed, I didn't like the new macros from the very 
>> > beginning.  Chao-ying 
>> > -- any thoughts?
>> 
>>   I am fine.  But the new method may be bigger than the A_BFD* method.
>
>  I have placed the translation in macro_map_reloc() now.  That incurs an 
> O(n) performance penalty because all relocations in the microMIPS mode 
> have to be iterated over the table of mappings; regrettably BFD_RELOC_* 
> definitions are sparse so an O(1) lookup table cannot be used.  By keeping 
> it sorted some time can be saved, but that's still O(n).  This could be 
> reduced to O(log n) with a binary search, but I fear with the limited 
> number of relocs handled the overhead would kill the benefit.

TBH, I think a case statement would be fine here.  It's then up to
the compiler to pick the best implementation.

>> > 	(MICROMIPS_TARGET, MICROMIPS_TARGET_LABEL): New macros.
>> > 	(micromips_add_number_label): New function.
>> 
>> +/* For microMIPS macros, we need to generate a local number label
>> +   as the target of branches.  */
>> +#define MICROMIPS_TARGET	"2147483647f"
>> +#define MICROMIPS_TARGET_LABEL	2147483647
>> +
>> +static void
>> +micromips_add_number_label (void)
>> +{
>> +  symbolS *s;
>> +  fb_label_instance_inc (MICROMIPS_TARGET_LABEL);
>> +  s = colon (fb_label_name (MICROMIPS_TARGET_LABEL, 0));
>> +  S_SET_OTHER (s, ELF_ST_SET_MICROMIPS (S_GET_OTHER (s)));
>> +}
>> +
>> 
>> Ugh, this is a bit hackish.  There's nothing stopping a user using
>> 2147483647f themselves.
>
>  This is now handled by micromips_label_name(), micromips_label_expr(), 
> micromips_label_inc() and micromips_add_label() in a manner similar to 
> what dollar_label_name() and fb_label_name(), etc. do, except that the 
> special character used in symbols generated is ^_, avoiding a clash.

Thanks, this looks better at first glance.

>  I have rewritten macro_end() entirely now.  I have changed the approach 
> such that if multiple instructions are emitted into a branch delay slot 
> and the first of these instructions is out of size, then two warnings are 
> produced.
>
>  The rationale is these are different classes of warnings -- the delay 
> slot size mismatch is fatal if the call is ever returned from.  Multiple 
> instructions may or may not be a problem depending on the actual piece of 
> code and author's intentions.  We had a discussion about it a few years 
> ago. ;)
>
>  To complete these changes I have modified the change to append_insn() 
> such that no warning is produced for a delay slot size mismatch if called 
> for a macro expansion as it would get in the way, especially in the case 
> of relaxation.  The API of this function had to be modified in a trivial 
> way and all the callers adjusted accordingly.

I'll have to think about this a bit.  And dig out that conversation ;-)

>> >> From a usability perspective, it's a shame that:
>> >> 
>> >> 	.set	micromips
>> >> 	.ent	foo
>> >> foo:
>> >> 	b	1f
>> >> 	nop
>> >> 	.end	foo
>> >> 	.ent	bar
>> >> bar:
>> >> 1:	nop
>> >> 	.end	bar
>> >> 
>> >> disassembles as:
>> >> 
>> >> 00000000 <foo>:
>> >>    0:   cfff            b       0 <foo>
>> >>                         0: R_MICROMIPS_PC10_S1  .L11
>> >>    2:   0c00            nop
>> >>    4:   0c00            nop
>> >> 
>> >> 00000006 <bar>:
>> >>    6:   0c00            nop
>> >> 
>> >> leaving the poor user with no idea what .L11 is.
>> >
>> >  Indeed.  This is a general limitation of `objdump' it would seem.  This 
>> > is no different to what you get with:
>> >
>> > $ cat b.s
>> > 	.globl	baz
>> > 	.ent	foo
>> > foo:
>> > 	b	baz
>> > 	nop
>> > 	.end	foo
>> > 	.ent	bar
>> > baz:
>> > bar:
>> > 1:	nop
>> > 	.end	bar
>> > $ mips-sde-elf-objdump -dr b.o
>> >
>> > b.o:     file format elf32-tradbigmips
>> >
>> >
>> > Disassembly of section .text:
>> >
>> > 00000000 <foo>:
>> >    0:	1000ffff 	b	0 <foo>
>> > 			0: R_MIPS_PC16	baz
>> >    4:	00000000 	nop
>> >    8:	00000000 	nop
>> >
>> > 0000000c <bar>:
>> >    c:	00000000 	nop
>> 
>> Well, it's a little different.  The user has at least defined two
>> named symbols in this case, so they have a good chance of knowing
>> what "baz" means.  In the microMIPS case we've invented a label
>> and are using it in preference to the user-defined one.
>
>  That's unfortunate, indeed.  [FIXME]

Have you looked into the work involved, before this gets dismissed
entirely as future work?

>> > I'd just recommend peeking at the symbol table (back to the first 
>> > program):
>> >
>> > $ mips-sde-elf-objdump -t b.o
>> >
>> > b.o:     file format elf32-tradbigmips
>> >
>> > SYMBOL TABLE:
>> > 00000000 l    d  .text	00000000 .text
>> > 00000000 l    d  .data	00000000 .data
>> > 00000000 l    d  .bss	00000000 .bss
>> > 00000000 l    d  .reginfo	00000000 .reginfo
>> > 00000000 l    d  .pdr	00000000 .pdr
>> > 00000000 l     F .text	00000006 0x80 foo
>> > 00000006 l     F .text	00000002 0x80 bar
>> > 00000006 l       .text	00000000 0x80 .L1^B1
>> 
>> I suppose having a symbol with ^B in it is less than ideal too.
>> AIUI that name was chosen specifically because it wasn't supposed
>> to be written out.
>> 
>> It would be especially confusing if the user or compiler had a ".L11"
>> label (without the ^B).
>
>  Actually this is a tough problem -- we need to emit a generated symbol 
> that does not clash with anything the user or GCC may have put in the 
> source.  Any ideas?  How is it done in other ports if anywhere?

None off-hand.  Seggestions from others very welcome.

I'll try to think of something when I've got more time.  For avoidance
of doubt, I think this does need to be fixed.

>> @@ -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.

>> micromips_ip obviously started life as a cut-&-paste of mips_ip, and it
>> would have been nice to factor some code out.  At least split out the
>> block beginning:
>> 
>> +	    case 'F':
>> +	    case 'L':
>> +	    case 'f':
>> +	    case 'l':
>> +	      {
>> 
>> which is identical between the two, and far too subtle to copy wholesale.
>> There may be other good opportunities too.
>
>  Next time, I'm afraid. [FIXME]

Next time == next iteration?  If so, that's fine, but I think this
has to be fixed for the patch to be acceptable.

>> The same cut-&-paste concerns apply to micromips_macro, which obviously
>> started out as a copy of macro().  I realise there are special cases
>> for micromips (such as the DADDI assymmetry and the lack of
>> branch-likely instructions, to name only a few), but most of the
>> basic decisions are the same.
>> 
>> As it stands, we have a new 3524 line function, of which I imagine at
>> least 90% is shared with macro().  I really think the new microMIPS
>> macro handling should be integrated into macro() instead.  Don't be
>> afraid of factoring out code from macro() if it makes it easier to
>> integrate the microMIPS code.
>
>  Can we make it a follow-up patch sometime in the future?  I'm not afraid 
> of factoring code out of anywhere (I'm speaking of myself only of course), 
> but at this point the effort is about the same as writing this whole stuff 
> from scratch. :( [FIXME]

Sorry, I know it's a pain, but I just don't think the patch can go
in as it stands.   The fact that the amount of work is so daunting
means that (even with the best of intentions, which I realise yours
are), that follow-up is unlikely to happen.  In the meantime we'll
have a 3524-line function (haven't recalculated ;-)) that has to be
kept in sync with another even bigger function (or pair of functions),
and it just won't be obvious whether the divergences are deliberate
microMIPS differences or whether they're cases where the two functions
haven't been updated in the same way.

I remember how painful it was replacing the old macro relaxation
code so that it could support arbitrary alternatives.  This sort
of thing would double that pain.

And it's not just (or even principally) about cut-&-paste phobia.
microMIPS has been deliberately designed to provide a great deal
of source-level backwards compatibility, so it's no surprise that
the two modes require very similar code, and very similar decisions.
I think having a single function handling both is actually significantly
clearer, because it calls out which differences are deliberate,
hopefully with a helpful bit of commentary where necessary.

As things stand, it's already a bit of an archaeology exercise trying
to decide what the differences actually are.

>  Overall I'm a bit concerned about all this linker relaxation stuff -- it 
> breaks -falign-jumps, -falign-labels and -falign-loops which may have a 
> severe performance penalty and should therefore be enabled conditionally 
> only, preferably where all the three options are set to 1 (meaning that 
> would be naturally implied by -Os).  A linker option would be required an 
> set appropriately by the GCC driver. [FIXME]

Yeah, I agree a bit more rigour would be nice here.  Without it, though,
I think it's generally OK to assume that anyone using MIPS16 or microMIPS
is more of the -Os rather than -O2 mindset, so I agree this is at most a
FIXME.

>> Did you actually test this with n64, say with a gcc bootstrap?  Same
>> comment goes for elfn32-mips.c.
>> 
>> Why only do the linker relaxation for elf32-mips.c (o32, o64 & EABI)?
>> Why not for n32 and n64 too?
>
>  The answer to all the questions is negative.  There is no 64-bit 
> microMIPS hardware available at the moment.  The best bet might be QEMU -- 
> NathanF, have you implemented any of the 64-bit instructions in QEMU?  
> Otherwise there's probably no way do do any of 64-bit microMIPS testing 
> beside what's done in binutils.  And no library work of any kind has 
> started for 64-bit microMIPS support AFAIK.  Hence there has been little 
> incentive to attempt anything but rudimentary support for 64-bit ABIs.

OK, in that case, never mind about testing 64-bit ;-)  However...

>  I envisage all the relaxation stuff to be either moved over to 
> elfxx-mips.c or copied to elfn32-mips.c and elf64-mips.c, as applicable, 
> once we have real 64-bit support.

...I still think this stuff should start life in elfxx-mips.c, even if
it's only used by elf32-mips.c at first.

>> That said, why split these up?  bfd_{get,put}_32
>> don't require aligned addresses.
>
>  I think it's easier and more readable this way as with bfd_{get,put}_32() 
> you'd have to halfword-swap the bit patterns produced based on the target 
> endianness.

Hmm, good point ;-)

>> @@ -5127,12 +5200,26 @@ mips_elf_calculate_relocation (bfd *abfd
>>  	      + h->la25_stub->stub_section->output_offset
>>  	      + h->la25_stub->offset);
>>  
>> +  /* Make sure MIPS16 and microMIPS are not used together.  */
>> +  if ((r_type == R_MIPS16_26 && target_is_micromips_code_p)
>> +      || (r_type == R_MICROMIPS_26_S1 && target_is_16_bit_code_p))
>> +   {
>> +      (*_bfd_error_handler)
>> +	(_("MIPS16 and microMIPS functions cannot call each other"));
>> +      return bfd_reloc_notsupported;
>> +   }
>> 
>> Should this be extended to check for branches too?
>
>  That would be a useful improvement I suppose, but MIPS16 code doesn't 
> care either (you can't use a branch to switch the ISA mode).  Overall I 
> think it's a corner case -- branches to external symbols are a rarity. 
> [FIXME]

But it is (or feels like) something that people have asked about on many
occasions, before we decided to ignore the botched ABI definition of
R_MIPS_PC16.

If it was a lot of work, I'd be sympathetic, but I think this is
something that's easily done, and handling one pair of relocs without
the others seems inconsistent.

>  I think R_MIPS_26 should be used for microMIPS JALX.  I don't like the 
> idea of overloading R_MICROMIPS_26_S1 this way, especially given another 
> relocation already fits.  What do you think?  It is too late to fix the 
> ABI probably though; I'm thinking what the consequences might be...

TBH, I quite like having a different reloc for the microMIPS version.
I agree it's probably moot though; the ABI has already been set.

>> Use of 0xfffffffc isn't portable for 64-bit addresses;
>> use "-4" or "~(bfd_vma) 3" instead.
>
>  I'm nasty enough to use (foo | 3) ^ 3 and avoid type width and promotion 
> dependencies altogether in such cases and hope for GCC to be smart and do 
> the right thing and combine the two ops.

Hmm, well... TBH, I don't really see that as an improvement over the
other two -- C is C -- but I'll let it pass ;-)

>> -  /* 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.

>> +  else if ((insn & 0x1c00) != 0x0400
>> +	   && (insn & 0x1c00) != 0x0800
>> +	   && (insn & 0x1c00) != 0x0c00)
>> +    {
>> +      /* This is a 32-bit microMIPS instruction.  */
>> +      higher = insn;
>> +
>> +      status = (*info->read_memory_func) (memaddr + 2, buffer, 2, info);
>> +      if (status != 0)
>> +	{
>> +	  (*info->fprintf_func) (info->stream, "micromips 0x%x",
>> +				 (unsigned int) higher);
>> +	  (*info->memory_error_func) (status, memaddr + 2, info);
>> +	  return -1;
>> +	}
>> 
>> Why print "micromips " here but not in the 48-byte case?
>
>  It looks to me it's been modelled after the MIPS16 version where "extend 
> 0x..." is printed in the case of a read error.  I'm not sure if either 
> makes sense (.byte foo or suchlike that some other ports output in such a 
> case looks more reasonable to me), but at least it is consistent.

Hmm, and MIPS16 doesn't print a prefix for unknown 4-byte ops.
OK, in that case, I agree the patch is right for consistency.
I agree with you about .byte being better in principle though,
as a possible future enhancement.

Richard