Re: [PATCH] microMIPS support

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Wed, 25 Apr 2012, Yao Qi wrote:

> > +int
> > +mips_pc_is_micromips (struct gdbarch *gdbarch, CORE_ADDR memaddr)
> > +{
> > +  struct minimal_symbol *sym;
> > +
> > +  /* A flag indicating that this is a microMIPS function is stored by
> > +     elfread.c in the high bit of the info field.  Use this to decide
> > +     if the function is microMIPS.  Otherwise if bit 0 of the address
> > +     is set, then ELF file flags will tell if this is a microMIPS
> > +     function.  */
> > +  sym = lookup_minimal_symbol_by_pc (memaddr);
> > +  if (sym)
> > +    return msymbol_is_micromips (sym);
> > +  else
> > +    return is_micromips_addr (gdbarch, memaddr);
> > +}
> 
> Why don't we check `is_micromips_addr' first, and return early if it
> returns true?  In this way, we can avoid some symbol lookups.

 The choice of the sequence is deliberate and actually we used to do the 
checks in the opposite order for MIPS16 code until recently.  The reason 
is there is IMO no point in using the wrong ISA type where the ISA bit has 
been set incorrectly, e.g. the wrong software breakpoint instruction when 
someone says:

(gdb) break *0xdeadbeef

and the address actually refers MIPS32 code.  Also the opposite does not 
work, that is with the current arrangement the ISA bit of the MEMADDR 
argument may have been implicitly cleared elsewhere even when referencing 
a piece of compressed code and no numeric address actually used.

 I think code should behave as consistently as possible, and therefore for 
all the three ISAs any associated symbol's attribute is checked first, 
followed by checking the ISA bit specified explicitly in the address.  The 
latter case is only meant to cover addresses for which no symbol 
information of any kind could have been found.

 Does it clear your concern?

  Maciej