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]

Re: [PATCH] MIPS: microMIPS ASE support


Thanks for the updates.  I'll try to go through the patches in the next
couple of weeks (maybe later this weekend).

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> "Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> >
>> >  Well, if we have code like this:
>> >
>> > 	branch	...
>> > 	 LUI	...
>> > 	insn	[...]
>> >
>> > (where for the purpose of this consideration BRANCH may also be a jump) 
>> > then LUI cannot be entirely deleted and INSN moved into the slot of BRANCH 
>> > no matter if INSN is a branch or an computational instruction.  All we can 
>> > do in this case is to see if there is a corresponding BRANCHC instruction 
>> > and use it to swap BRANCH with and then delete the LUI if so, or otherwise 
>> > shrink the LUI to a 16-bit NOP if BRANCH permits or can be swapped with 
>> > BRANCHS to permit a 16-bit delay-slot instruction.  If neither is 
>> > possible, then the LUI is merely substituted with a 32-bit NOP (although 
>> > the effect is purely cosmetical in this case; perhaps we should just back 
>> > out).
>> 
>> Yeah, I see your point.  I was thinking that the code claims to "know"
>> that the LUI and "insn" are both part of the same load address.  So if
>> the branch was taken, the target of the LUI ought to be dead.  However,
>> I agree that (even though the code does seem to assume that to some extent)
>> the assumption is wrong.
>> 
>> E.g. you could have:
>> 
>> 	beqz	$2,1f
>> 	lui	$4,%hi(foo)	<-- A
>> 
>> 	addiu	$4,$4,%lo(foo)	<-- B
>> 	...
>> 	jr      $31
>> 2:	...
>> 	lui	$4,%hi(foo)	<-- C
>> 	...
>> 1:	addiu   $4,$4,%lo(foo)	<-- D
>> 
>> In this case, the LO16 reloc for D might follow the HI16 reloc for C,
>> and the LO16 reloc for B might follow the HI16 reloc for A.  AIUI, we'd
>> consider relaxing A/B but not C/D.  In this case, turning A into a NOP
>> is wrong, because $4 is still live at D.  If you agree then...
>> 
>> >  Also with the recent update to LUI relaxation code I think we should 
>> > simply disallow the optimisation if a LUI is in a delay slot of an 
>> > unconditional branch -- we have no way to verify the corresponding LO16 
>> > reloc really belongs to this LUI instruction in that case.  This will let 
>> > us simplify code (which has become a little bit hairy by now IMO) a little 
>> > bit I would guess.  [FIXME]
>> 
>> ...maybe it would be simpler to drop the optimisation if the LUI is any
>> kind of delay slot.  I think this would simply the code, and I don't think
>> we'd then need to check for branch relocs.  We'd just have *_norel-like
>> functions (although not called that any more) to check for every kind
>> of branch.
>
>  I have implemented these changes now, dropping the unsafe part of 
> optimisation for the scenario you have listed.  I still have two concerns 
> about this optimisation, but the optional nature of linker relaxation 
> makes them reasonably unimportant IMO:
>
> 1. The resulting change of alignment may cause the linker produce bad code 
>    or abort the process if microMIPS and standard MIPS code is mixed in 
>    one object file and the latter turns out to become unaligned, e.g.
>
> 	.set	micromips
> 	.set	noreorder
> 	.align	2
> 	.globl	foo
> 	.ent	foo
> foo:
> 	beqz32	$4, 0f
> 	 nop16
> 0:
> 	jalx	bar
> 	nop
> 	.end	foo
>
> 	.set	nomicromips
> 	.align	2
> 	.globl	bar
> 	.ent	bar
> bar:
> 	nop
> 	.end	bar
>
>    The piece above will fail to link, because BEQZ will be converted to 
>    a BEQZC and the 16-bit NOP from its delay slot taken out.  As a result 
>    bar() will become misaligned and the JALX will not be allowed.  If 
>    there was no JALX, then linking might succeed if there were only 
>    indirect calls to bar(), but the function would not be properly aligned 
>    for standard MIPS execution.
>
> 2. Broken code is produced for cases like this:
>
> 	.set	noreorder
> 	lui	$4, 0x1234
> 	lui	$2, %hi(foo)
> 	bnez	$3, 0f
> 	 addiu	$2, %lo(foo)
> 	...
>
> 	lui	$4, %hi(foo)
> 0:
> 	jal	bar
> 	 addiu	$4, %lo(foo)
>
>    where the resulting code:
>
> 	.set	noreorder
> 	lui	$4, 0x1234
> 	bnez	$3, 0f
> 	 addiu	$2, $pc, foo - .
> 	...
>
> 0:
> 	jal	bar
> 	 addiu	$4, $pc, foo - .
>
>    obviously not being the same.  Such usage of HI16/LO16 relocations is 
>    non-standard, but not disallowed.  OTOH searching the symbol tables for 
>    the label (we could disable this relaxation if there's one at the
>    instruction a LO16 relocation is against) is expensive.
>
>  What do you think?  [FIXME]

I agree that these are minor.  At least with (1) we'll get an error
rather than silent wrong code, and like you say, if someone hits that error,
they can simply stop using relaxation.

I think it's fair for us to say that (2) is disallowed if you want to
use linker optimisations.

>> > +  /* Whether we are assembling for the mipsMIPS processor.  0 if we are
>> > +     not, 1 if we are, and -1 if the value has not been initialized.
>> > +     Changed by `.set micromips' and `.set nomicromips', and the -mmicromips
>> > +     and -mno-micromips command line options, and the default CPU.  */
>> > +  int micromips;
>> 
>> Blind cut-&-paste.  "microMIPS ASE".
>> 
>> Let's leave the -1 case and:
>> 
>> > +/* Return true if the given CPU supports microMIPS.  */
>> > +#define CPU_HAS_MICROMIPS(cpu)	0
>> 
>> out.  I think the CPU_HAS_MIPS16 stuff is derived from the original LSI
>> TinyRisc support and wouldn't be used for ASEs.
>
>  The microMIPS ASE provides for processors that do not support the 
> standard MIPS instruction set.  These I think should default to the 
> microMIPS mode.  I suspect someone will eventually implement such a 
> processor as since we've got this code implemented here already I'd like 
> to leave it as a placeholder.  I think it's not much of a burden, is it?

Oh no, it wasn't any sense of burden that bothered me.  It was more
the talk of "the microMIPS processor".  "The MIPS16 processor" made
sense when the support was first added, but plain "microMIPS" makes
more sense here.  (Or, reading further on, I suppose you won't agree.
Something other than "processor" though, if you want to treat
"microMIPS" as an adjective.)

I just thought that, if this was dead code, we might as well just
remove it rather than quibble about wording.  Given what you say about
microMIPS-only processors being possible though, please just change the
comment instead.

>> > +#define RELAX_MICROMIPS_ENCODE(type, is_16bit, uncond, link, toofar)	\
>> > +  (0x40000000							\
>> > +   | ((type) & 0xff)						\
>> > +   | ((is_16bit) ? 0x100 : 0)					\
>> > +   | ((uncond) ? 0x200 : 0)					\
>> > +   | ((link) ? 0x400 : 0)					\
>> > +   | ((toofar) ? 0x800 : 0))
>> > +#define RELAX_MICROMIPS_P(i) (((i) & 0xc0000000) == 0x40000000)
>> > +#define RELAX_MICROMIPS_TYPE(i) ((i) & 0xff)
>> > +#define RELAX_MICROMIPS_USER_16BIT(i) (((i) & 0x100) != 0)
>> > +#define RELAX_MICROMIPS_UNCOND(i) (((i) & 0x200) != 0)
>> > +#define RELAX_MICROMIPS_LINK(i) (((i) & 0x400) != 0)
>> > +#define RELAX_MICROMIPS_TOOFAR(i) (((i) & 0x800) != 0)
>> > +#define RELAX_MICROMIPS_MARK_TOOFAR(i) ((i) | 0x800)
>> > +#define RELAX_MICROMIPS_CLEAR_TOOFAR(i) ((i) & ~0x800)
>> 
>> Is there a need to create variant frags when the user has explicitly
>> specified the instruction size?  I wouldn't have expected any relaxation
>> to be necessary in that case, and it looks like the relaxation code does
>> indeed return 2 whenever USER_16BIT is true.
>
>  I suspect this has been copied over from MIPS16 code.  
> RELAX_MIPS16_USER_SMALL seems to be used in a similar fashion.  Do you 
> happen to know for sure why it has been implemented this way for MIPS16 
> assembly?

Nope :-)

> My suspicion is we want to keep the relocation until the final 
> relaxation so that if the final value turns out to fit afterwards (but not 
> until then) in the forced-truncated immediate field of the instruction 
> nothing is lost.

But that's true of all fixups, and should already be handled correctly.

E.g. if you had microMIPS code embedded in a larger MIPS function, you
might have normal MIPS branches that cross relaxable microMIPS instructions.
The same consideration would apply then, even if branch relaxation
wasn't enabled.

>> > +#define RELAX_MICROMIPS_EXTENDED(i) (((i) & 0x10000) != 0)
>> > +#define RELAX_MICROMIPS_MARK_EXTENDED(i) ((i) | 0x10000)
>> > +#define RELAX_MICROMIPS_CLEAR_EXTENDED(i) ((i) & ~0x10000)
>> 
>> Any particular reason why 0x10000 rather than 0x1000, which seems
>> to be the first unused bit?  I would prefer to pack the used bits
>> together so that it's easier to tell what's left.
>
>  These weren't used anywhere.  I have discarded these macros.

Thanks, missed that.

>> > +	      /* For microMIPS, disable reordering.  */
>> > +	      || mips_opts.micromips
>> 
>> You should say whether this is for simplicity or by specification.
>> Either way, a bit more detail would be welcome.  E.g. something like:
>> 
>> 	      /* microMIPS assembly language does not allow the assembler
>> 	      	 to reorder instructions, even in .set reorder mode.
>> 		 Delay slots are always filled with nops when .set reorder
>> 		 is in effect.  */
>> 
>> (adjusted as appropriate if my guess is wrong).
>
>  I believe the concerns are the same as with MIPS16 code -- so far we have 
> failed to develop means to update DWARF-2 records accordingly and if a 
> 32-bit branch/jump is swapped with a 16-bit delay-slot instruction (or 
> vice versa as it's permitted in microMIPS code, though not in MIPS16 one) 
> then substandard debugging experience results from software breakpoints 
> placed mid-through an instruction.
>
>  So as much as we'd love to reorder we really can't without fixing GAS 
> elsewhere.
>
>  Hmm, it looks like the piece of code to disable MIPS16 reordering has 
> never made its way upstream.  It should, unless we have a volunteer to fix 
> GAS immediately, so while holding my breath and hoping that people won't 
> fight over this challenge I extracted this piece now and updated this 
> change accordingly.  It makes no sense to keep the two pieces separate.

Thanks, the new version is much more descriptive.

>> >  	case 'i':
>> >  	case 'j':
>> >  	  macro_read_relocs (&args, r);
>> > -	  gas_assert (*r == BFD_RELOC_GPREL16
>> > +	  gas_assert (mips_opts.micromips
>> > +		      || *r == BFD_RELOC_GPREL16
>> >  		      || *r == BFD_RELOC_MIPS_HIGHER
>> >  		      || *r == BFD_RELOC_HI16_S
>> >  		      || *r == BFD_RELOC_LO16
>> >  		      || *r == BFD_RELOC_MIPS_GOT_OFST);
>> > +	  gas_assert (!mips_opts.micromips
>> > +		      || *r == BFD_RELOC_MICROMIPS_GPREL16
>> > +		      || *r == BFD_RELOC_MICROMIPS_HIGHER
>> > +		      || *r == BFD_RELOC_MICROMIPS_HI16_S
>> > +		      || *r == BFD_RELOC_MICROMIPS_LO16
>> > +		      || *r == BFD_RELOC_MICROMIPS_GOT_OFST);
>> 
>> Let's move the macro_read_relocs stuff inside append_insn rather than
>> leaving the conversion to the callers.  You could then make append_insn
>> keep a record of the original (non-microMIPS) reloc_type[0] too,
>> which would simply some of the logic.  E.g. these changes would
>> no longer be needed:
>> 
>> > -	  /* Tag symbols that have a R_MIPS16_26 relocation against them.  */
>> > -	  if (reloc_type[0] == BFD_RELOC_MIPS16_JMP
>> > +	  /* Tag symbols that have a R_MIPS16_26 or R_MICROMIPS_26_S1
>> > +	     relocation against them.  */
>> > +	  if ((reloc_type[0] == BFD_RELOC_MIPS16_JMP
>> > +	       || reloc_type[0] == BFD_RELOC_MICROMIPS_JMP)
>> >  	      && ip->fixp[0]->fx_addsy)
>> >  	    *symbol_get_tc (ip->fixp[0]->fx_addsy) = 1;
>
>  This actually would -- note that's BFD_RELOC_MIPS16_JMP and not 
> BFD_RELOC_MIPS_JMP there.

Oops, yes.

>  BTW, do you happen to know what the issue about BFD_RELOC_MIPS_SCN_DISP 
> is?  We refer to it in a couple of places throughout GAS, but never 
> actually generate it.  I realise BFD may have to handle the reloc for 
> compatibility with other tools, but GAS?

In a word, no.

>> > -	  macro_build (NULL, "jalr", "d,s", dreg, sreg);
>> > +	  s = (!mips_opts.micromips || (mips_opts.noreorder && !cprestore)
>> > +	       ? "jalr" : "jalrs");
>> > +	  if (mips_opts.micromips && dreg == RA)
>> > +	    macro_build (NULL, s, "mj", sreg);
>> > +	  else
>> > +	    macro_build (NULL, s, JALR_FMT, dreg, sreg);
>> 
>> Since we can use JALRS for mips_opts.noreorder && cprestore, I suppose
>> the cprestore nop:
>> 
>>  		  if (mips_opts.noreorder)
>> 		    macro_build (NULL, "nop", "");
>> 
>> ought to be conditional on !mips_opts.micromips.
>
>  No, the delay slot always has to be explicitly filled here.  Otherwise 
> you'll end up with LW $gp there (the instruction has a 16-bit variation 
> too) -- that I fixed not so long ago.
>
>  For the avoidance of doubt: all the call (linked jump/branch) 
> instructions have a fixed-length delay slot that takes either 4 bytes (as 
> with BGEZAL, BLTZAL, JAL, JALR and JALX instructions) or 2 bytes (as with 
> BGEZALS, BLTZALS, JALS and JALRS).  All the other jump/branch instructions 
> have an any-length delay slot except from compact jump/branch instructions 
> that have none (these are BEQZC, BNEZC and JRC).  Overall the "S" suffix 
> stands for a short delay slot and the "C" one means a compact jump/branch, 
> i.e. no delay slot.

Ah, yeah, sorry.  As you'd guessed, I think I got "C" and "S" confused.

>  While doing this I noticed we have a bug.  In a sequence like this:
>
> 	alnv.ps	$f0, $f1, $f2, $3
> 	jalr	$3, $2
>
> the ALNV.PS will get reordered into the delay slot.  This is obviously 
> wrong.

Which answers my question in the other message. :-)  Good catch.

>> > +		  if (!ok)
>> > +		    {
>> > +		      switch (*args++)
>> 
>> I realise you've copied this from elsewhere, but why "++"?
>> The "for" loop increments "args", doesn't it?
>
>  This is the same as for 'r', etc. (i.e. a register that's optional in the 
> source if the destination is the same as the target).  Otherwise code 
> like:
>
> 	andi16	$7, 65535
> 	addiu16	$31, 7
>
> fails to assemble.  The thing is once we get to "65535", we still have a 
> "," unconsumed in args.  I have rewritten it more properly though, adding 
> a check for that "," too.

Thanks for the explanation and the change.  The new version is much clearer.

>> > +		    i = my_getSmallExpression (&imm_expr, imm_reloc, s);
>> > +		    if ((i == 0 && (imm_expr.X_op != O_constant
>> > +				    || (imm_expr.X_add_number & 3) != 0
>> > +				    || imm_expr.X_add_number > (63 << 2)
>> > +				    || imm_expr.X_add_number < (-64 << 2)))
>> > +			|| i > 0)
>> > +		      {
>> > +			imm_expr.X_op = O_absent;
>> > +			break;
>> > +		      }
>> > +		    immed = imm_expr.X_add_number >> 2;
>> > +		    INSERT_OPERAND (1, IMMA, *ip, immed);
>> > +		    imm_expr.X_op = O_absent;
>> > +		    s = expr_end;
>> > +		    continue;
>> 
>> Why set X_op to O_absent when rejecting this alternative?  What breaks
>> if you leave the constant in imm_expr?  I couldn't see any similar
>> error-handling code in this function.
>
>  I believe the reason is if the offset does not fit for the purpose of 
> this 16-bit encoding, then we'll fall back to a 32-bit one that uses 
> offset_expr instead.

Ah, yeah, imm_expr trumps offset_expr.

>  That written, I have given it some thinking and decided to use local 
> variables instead removing any references to imm_expr and thus any issue 
> about its usage.  We don't pass the results up to append_insn() from here 
> in any case.

Thanks, that's better.

>> > +		  /* If users want relax branch and don't specify to use
>> > +		     16-bit instructions, we will not match this pattern.
>> > +		     This will lead to matching 32-bit instructions, that
>> > +		     will be relaxed later.  */
>> > +		  if (mips_relax_branch && forced_insn_length != 2)
>> > +		    break;
>> 
>> This seems a bit lame.  It should be easy to relax the 16-bit form
>> in the same way as the 32-bit form.  We could use a bit in the
>> relaxation opcode to say whether the extra relaxation should be
>> enabled or not, i.e. a bit to record the relevant parts of this
>> condition:
>> 
>> +	   && (pinfo & INSN_UNCOND_BRANCH_DELAY
>> +	       || pinfo & INSN_COND_BRANCH_DELAY)
>> +	   && mips_relax_branch
>> +	   /* Don't try branch relaxation within .set nomacro, or within
>> +	      .set noat if we use $at for PIC computations.  If it turns
>> +	      out that the branch was out-of-range, we'll get an error.  */
>> +	   && !mips_opts.warn_about_macros
>> +	   && (mips_opts.at || mips_pic == NO_PIC)
>> +	   && mips_opts.micromips
>> +	   /* Don't try branch relaxation, when users specify 16-bit/32-bit
>> +	      instructions.  */
>> +	   && !forced_insn_length)
>> 
>> No need to do that as part of this patch, but let's at least put in
>> a FIXME.
>
>  Indeed; we have a preexisting bug here as well -- mips_opts.at may well 
> be != ATREG.  (A similar bug is present in fix_loongson2f_jump() BTW).

I'm not particularly happy about the loongson workarounds TBH.

>  Actually I've thought it's lame enough to implement it.  In the course of 
> which I discovered (and fixed) other three bugs, so I think it was worth 
> the effort.  Sent as a sepate patch for the same reasons as the reloc 
> change above.

Thanks.

>> > +  /* For microMIPS PC relative relocations, we cannot convert it to
>> > +     against a section.  If we do, it will mess up the fixp->fx_offset.  */
>> >    if (fixp->fx_r_type == BFD_RELOC_VTABLE_INHERIT
>> > -      || fixp->fx_r_type == BFD_RELOC_VTABLE_ENTRY)
>> > +      || fixp->fx_r_type == BFD_RELOC_VTABLE_ENTRY
>> > +      || 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)
>> 
>> "to be against a section".  That's not a helpful comment though.
>> _How_ will it mess up fixp->fx_offset?  Give the reader a clue why
>> the problem applies to BFD_RELOC_MICROMIPS_16_PCREL_S1 but not
>> to something like BFD_RELOC_16_PCREL_S2.
>
>  I have failed to spot any problems with this hunk reverted and I'm not 
> sure what I should be looking for.  Therefore I feel a bit uneasy about 
> removing it and only rephrased the comment without actually changing its 
> meaning.  Chao-ying, do you have anything to add?

Thanks for the discussion and revision downthread.  The final version makes
much more sense.

>> > Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips1-fp.d
>> > ===================================================================
>> > --- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips1-fp.d	2010-12-07 00:05:05.000000000 +0000
>> > +++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips1-fp.d	2010-12-07 00:14:47.000000000 +0000
>> > @@ -9,4 +9,4 @@
>> >  [0-9a-f]+ <.*>:
>> >  .*:	46041000 	add.s	\$f0,\$f2,\$f4
>> >  .*:	44420000 	cfc1	\$2,\$0
>> > -#pass
>> > +	\.\.\.
>> > Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips1-fp.s
>> > ===================================================================
>> > --- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips1-fp.s	2010-12-07 00:05:05.000000000 +0000
>> > +++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips1-fp.s	2010-12-07 00:14:47.000000000 +0000
>> > @@ -5,3 +5,7 @@
>> >  foo:
>> >  	add.s	$f0,$f2,$f4
>> >  	cfc1	$2,$0
>> > +
>> > +# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
>> > +	.align	2
>> > +	.space	8
>> 
>> Leave out this kind of change.  I realise it's not the style you prefer,
>> but what's there now is fine.  Same for:
>> 
>> * gas/testsuite/gas/mips/mips32r2-fp32.d
>> * gas/testsuite/gas/mips/mips64.d
>
>  Well, that's not merely a matter of style as #pass simply ignores any 
> following rubbish GAS may have produced.  This forces a corresponding 
> update to gas/testsuite/gas/mips/micromips@mips1-fp.d as the results 
> differ between Linux and bare-iron targets.  I'd prefer to avoid adding 
> new instances of #pass although I've just noticed the original change 
> included some other too, so I've applied your suggestion reluctantly.  
>
>  These test cases should really be all audited and unjustified uses of 
> #pass removed -- I realise some people have difficulties following all 
> the details of and good ways to deal with subtleties in this area and 
> follow the path of least resistance, but we shouldn't be encouraging this 
> kind of behaviour.  Especially as you seem to be quite picky elsewhere. ;)

We'll just have to agree to disagree here.  Both "#pass" and "insert
a set amount of padding at the end of each source file" have their
disadvantages.  The former fails to catch cases where we emit extra
junk.  The latter means that we never test source files that people
are actually likely to write, or that have instructions at the end
of the section.  I think the former is at least as good as the latter.

>> > -#define STO_MIPS_PLT		0x8
>> > +#define STO_MIPS_PLT		(1 << 3)
>> 
>> Don't change the definitions of the existing constants; use hex constants
>> for the new stuff instead.
>
>  Well, STO_OPTIONAL already uses a shifted bit and I find this notation 
> clearer.  Since code is already inconsistent I have updated it not to 
> change the existing definitions, but kept newly-added ones as shifted 
> bitfields.  I'm happy to keep code consistent, but if a piece is not, I 
> will choose the style that suits me better, sorry.

Fair enough.

>> > +/* These are the bitmasks and shift counts used for the different
>> > +   fields in the instruction formats.  Other than OP, no masks are
>> > +   provided for the fixed portions of an instruction, since they are
>> > +   not needed.  */
>> 
>> Seems like too much cut-&-paste: there isn't an OP field here.
>> "Other than TARGET", perhaps, unless there are other opcode masks here.
>
>  This looks like copied verbatim from the MIPS16 part.  The two parts are 
> functionally equivalent and my understanding of the comment is no masks 
> are provided for the non-operand parts of instruction.  I've left the 
> comment as is; I'm not sure what TARGET might mean in this context, please 
> elaborate.

As you say, the MIPS16 comment is:

/* These are the bitmasks and shift counts used for the different
   fields in the instruction formats.  Other than OP, no masks are
   provided for the fixed portions of an instruction, since they are
   not needed.

OP in this case refers to the first field definition:

#define MIPS16OP_MASK_OP	0x1f
#define MIPS16OP_SH_OP		11

which is the opcode ("fixed portion").  There didn't seem to be
a corresponding MASK_OP and SH_OP for microMIPS.

I'm equally bemused as to where I'd got "TARGET" from...

>> > +/* Return 1 if a symbol associated with the location being disassembled
>> > +   indicates a compressed mode, either MIPS16 or microMIPS one.  Otherwise,
>> > +   return 0.  */
>> 
>> Reads more naturally to me without "one".
>
>  Both MIPS16 and microMIPS are adjectives; they need a noun or a pronoun.  
> I realise this requirement is not met everywhere, but that doesn't mean we 
> should add new such places IMO.

Really?  I'd have thought they were nouns.  If you want them to be
adjectives though, it should be "either the ...".

>> > +  for (i = 0; i < info->num_symbols; i++)
>> > +    {
>> > +      pos = info->symtab_pos + i;
>> > +
>> > +      if (bfd_asymbol_flavour (info->symtab[pos]) != bfd_target_elf_flavour)
>> > +	continue;
>> > +
>> > +      symbol = (elf_symbol_type *) info->symtab[pos];
>> > +      if ((!micromips_ase
>> > +	   && ELF_ST_IS_MIPS16 (symbol->internal_elf_sym.st_other))
>> > +	  || (micromips_ase
>> > +	      && ELF_ST_IS_MICROMIPS (symbol->internal_elf_sym.st_other)))
>> > +	    return 1;
>> > +    }
>> 
>> Why is a search necessary here, when the previous code was happy to
>> look only at the first symbol?  I'm not saying the code is wrong,
>> but a bit of commentary would be good.
>
>  My feeling is previous code was not "happy", but simply untested (or to 
> be more accurate, not tested satisfactorily).
>
>  Symbols sharing the same address are sorted alphabetically here which 
> becomes a problem when they include both objects and functions (or symbols 
> derived from standard MIPS functions defined elsewhere).  Disassembly 
> shouldn't yield different results based merely on the names of symbols 
> chosen and given the semantics of the compressed annotation (it is only 
> added to a function symbol if a genuine instruction has been emitted 
> following immediately in the source code) I think it should take 
> precedence, so we check if any symbol has one.

Agreed, and like I say, I was willing to believe the new code was right.
Please put in a comment along these lines.

>> What problem is the ld-lib.exp change fixing?
>
>  Currently you can't build the same source file multiple times with 
> different flags.  See ld/testsuite/ld-mips-elf/mips16-and-micromips.d for 
> a use case (and try it with the ld-lib.exp piece reverted).  I think the 
> framework shouldn't be limiting the developer like this and making a copy 
> of the source to work around the limitation sounds to me like the wrong 
> direction to go.

OK, thanks, makes sense.  The ld-lib.exp change is independently OK.
Please commit it separately.

Richard


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