This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] MIPS: microMIPS ASE support
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: "Fu\, Chao-Ying" <fu at mips dot com>
- Cc: "Maciej W. Rozycki" <macro at codesourcery dot com>, "Garbacea\, Ilie" <ilie at mips dot com>, Joseph Myers <joseph at codesourcery dot com>, "binutils\ at sourceware dot org" <binutils at sourceware dot org>, "Fuhler\, Rich" <rich at mips dot com>, "Lau\, David" <davidlau at mips dot com>, "Mills\, Kevin" <kevinm at mips dot com>, Catherine Moore <clm at codesourcery dot com>, Nathan Sidwell <nathan at codesourcery dot com>, Nathan Froyd <froydnj at codesourcery dot com>
- Date: Sat, 18 Dec 2010 09:47:02 +0000
- Subject: Re: [PATCH] MIPS: microMIPS ASE support
- References: <alpine.DEB.1.10.1005181806590.4023@tp.orcam.me.uk> <87y6fa9u3t.fsf@firetop.home> <alpine.DEB.1.10.1007112325020.2824@tp.orcam.me.uk> <876302kqvu.fsf@firetop.home> <alpine.DEB.1.10.1012061924490.5345@tp.orcam.me.uk> <871v5n9m7e.fsf@firetop.home> <alpine.DEB.1.10.1012131400320.4142@tp.orcam.me.uk> <7C6479EB2BF52547AC332FD6034646DA3018F765@exchdb02.mips.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