Re: [PATCH] MIPS: microMIPS ASE support

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Fu, Chao-Ying" <fu@mips.com> writes:
>> > > +  /* Now adjust the global symbols defined in this section.  */
>> > > +  symcount = (symtab_hdr->sh_size / sizeof (Elf32_External_Sym)
>> > > +	      - symtab_hdr->sh_info);
>> > > +  sym_hashes = start_hashes = elf_sym_hashes (abfd);
>> > > +  end_hashes = sym_hashes + symcount;
>> > > +
>> > > +  for (; sym_hashes < end_hashes; sym_hashes++)
>> > > +    {
>> > > +      struct elf_link_hash_entry *sym_hash = *sym_hashes;
>> > > +
>> > > +      if ((sym_hash->root.type == bfd_link_hash_defined
>> > > +	   || sym_hash->root.type == bfd_link_hash_defweak)
>> > > +	  && sym_hash->root.u.def.section == sec
>> > > +	  && sym_hash->root.u.def.value > addr
>> > > +	  && sym_hash->root.u.def.value < toaddr)
>> > > +	sym_hash->root.u.def.value -= count;
>> > > +    }
>> > 
>> > ...if we're willing to extend the upper bound in this way, I wonder
>> > whether there's really any point having an upper bound at all.
>> > 
>> > Then again, why doesn't the standard range (used by most targets)
>> > include toaddr?  If you define an end marker:
>> > 
>> > end_of_section:
>> > 	# nothing after this
>> > 
>> > then wouldn't end_of_section == toaddr, and shouldn't it be 
>> included?
>> 
>>  Ilie, I'm told you were responsible for this piece of code 
>> -- would you 
>> please respond to these questions?
>
>   I think we should include "end_of_section == toaddr".
> Ex:
> 	  && sym_hash->root.u.def.value <= toaddr)

I agree that's probably the best thing, although I admit the "Then again,"
question was really more general musing than a question about this patch.

To be more specific, the thing that worried me about this code was that:

- if the section ends with a microMIPS instruction and an end marker,
  the "value < toaddr" won't include the end marker.  (The end marker
  would be odd.)

- if the section ends with a normal MIPS instruction and an end marker,
  the "value < toaddr" _would_ include the end marker.  (The end marker
  would be even.)

That's the key difference between "toaddr |= 1 ... value < toaddr"
and "value &= -2 .... value < toaddr".  I think the latter is more
consistent.

I suggest removing:

+  BFD_ASSERT (toaddr % 2 == 0);
+  addr |= 1;
+  toaddr |= 1;

and instead masking the low bit off the u.def.value.  (Keep the other
two asserts though.  I certainly found them useful when reviewing
the code.)

>> > > +static unsigned long
>> > > +find_match (unsigned long opcode, const struct 
>> opcode_descriptor insn[])
>> > > +{
>> > > +    unsigned long indx;
>> > > +
>> > > +    /* First opcode_table[] entry is ignored.  */
>> > > +    for (indx = 1; insn[indx].mask != 0; indx++)
>> > > +      if (MATCH (opcode, insn[indx]))
>> > > +	return indx;
>> > > +
>> > > +    return 0;
>> > > +}
>> > 
>> > But _why_ is the first entry ignored?
>> 
>>  There must be a reason, Ilie?
>
>   I guess Ilie tries to avoid passing a single-entry table into find_match().
> But if we do pass a single-entry table into find_match(), 
> find_match() may not find the end marker and it will be wrong anyway.
> Maybe we just delete all the { 1, 1 } entry in opcode_descriptor [] tables, and
> find_match() doesn't ignore the first entry.
> And we make sure that each table has end marker at the end.

Yeah, I think that'd be better, thanks.

Richard