Re: [PATCH] microMIPS support

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

On Wed, 25 Apr 2012, Joel Brobecker wrote:

> I could only scan the patch briefly for today, but noticed something
> that caught my attention:
> 
> > +/* For backwards compatibility we default to MIPS16.  This flag is
> > +   overridden as soon as unambiguous ELF file flags tell us the
> > +   compressed ISA encoding used.  */
> > +static const char mips_compression_mips16[] = "mips16";
> > +static const char mips_compression_micromips[] = "micromips";
> > +static const char *mips_compression_strings[] = {
> > +  mips_compression_mips16,
> > +  mips_compression_micromips,
> > +  NULL
> > +};
> 
> We usually provide this sort of feature a little differently: Instead
> of 2 values that are adjusted automatically by the debugger, we provide
> 3 values: auto, mips16, and micromips. If auto, then the debugger has
> to guess, possibly defaulting to mips16 if guessing did not work.
> But if the user sets the setting to either of the non-auto values,
> then the setting should be honored, even if the user is wrong.
> 
> This is usually implemented using two variables: One representing
> the setting, and one representing the actual value.

 I have been aware of that and decided the model does not fit.  Please 
note that this setting is a fall-back that's never used when you have an 
executable available.  If you do have an executable, then the ISA mode of 
compressed code is always known, because it's recorded in two places: 
individual symbol's ELF st_other flags, and globally, in the ELF file 
header.

 The latter is actually optional, in the sense that originally the 
compressed ISA was not recorded at all.  However all microMIPS objects 
must have the microMIPS flag set and therefore any compressed code in an 
executable that does not have any compressed flag set must be MIPS16 code 
in a legacy object.

 It's the st_other flags that's normally used by GDB to determine the 
compressed ISA kind, the ELF header flags are only checked for pieces of 
code that have no symbol information available for some reason.  And there 
is no point in overriding the ISA mode mandated by the executable, it must 
be clearly wrong, and there is really nothing to "guess" for GDB here -- 
all the required information is always available.

 Therefore the "auto" setting is meaningless -- you cannot infer any mode 
in the scope this setting has any use for, that is debugging with no 
executable (e.g. connecting to a live remote target), which is the very 
point of providing this setting at all.  Also note that this setting is 
"sticky", that is it's set from any executable selected.  If the 
executable is later on discarded, then the last setting is preserved 
assuming that if the user worked with one compressed ISA before it's more 
likely that they still use that ISA and not the other for non-debug code.

 Note this is a corner case too, but I decided it has to be handled 
somehow for robustness of GDB -- e.g. you may want to step-through 
power-on firmware you have no matching executable for via a JTAG probe 
for some reason.

> This brings me to the next question: Could this be an objfile-specific
> setting? In other words, is it possible to that the same executable
> might have one objfile using micromips while another might still be
> using mips16? (this might be the stupidest question of the week...).
> If not, I still believe that this is at least an inferior-specific
> property. With multi-inferior debugging being possible, I can see
> how one debugs two programs using a different setting. In that case,
> you need to store that information in the inferior private data
> (I do that in ada-tasks, IIRC, for storing the layout of some data
> structures).

 The question is of course fine and actually asked before in the process 
of adding microMIPS support to binutils.  There is no way for MIPS16 and 
microMIPS code to be present in a single executable.  We mandated this in 
binutils deciding that it would be a corner case not worth handling.

 Background information: the architecture does not permit the MIPS16 and 
the microMIPS ASE to be implemented in a single processor, one precludes 
the other (note that the architecture does permit an implementation that 
only supports the microMIPS mode and not the "traditional" standard MIPS 
mode, known since ~1985).  Therefore the only case where an executable 
could contain both MIPS16 and microMIPS code would be a piece of software 
that dynamically determines which of the two ASEs is present on a 
processor and switch to the right set of procedures on the fly (this is 
similar to what some power-on firmware does to handle both endiannesses 
with a single image on systems where you can switch the endianness on the 
fly, either with a physical switch or via some board logic).  We decided 
this is too much complication for little possibility of this case to ever 
matter for anyone.

 And personally I think the case of mixing MIPS16- and microMIPS-capable 
processors in a multi-processor systems (for multi-inferior debugging) is 
very unlikely too.  I don't think anyone has plans to support such a 
system, that would sort of be missing the point (the microMIPS ASE is 
intended to replace the MIPS16 ASE in deployments where code density 
matters), not even mentioning this would most likely require different 
base processor types too as I don't expect any single MIPS processor to 
support the choice between the MIPS16 ASE and the microMIPS ASE as RTL 
options (currently only the MIPS M14K and M14Kc processors support the 
microMIPS ASE and neither base architecture supports the MIPS16 ASE as an 
option).

 So until (or really unless) somebody comes with the requirement to 
support both the microMIPS and the MIPS16 ASE at a time in a single debug 
session let's keep this simple.

> (oops, style issue as well, the opening curly bracket should be
> at the start of the next line - we've seen a lot of your style too,
> but I think it should be fixed)

 Yes, I've changed that, thanks.  Actually the very preceding array uses 
the very style I did. ;)

 Actually I keep getting confused about the style expected for aggregate 
types, especially in the context of initialisers.  So for example is this 
correct:

static int
foo (void)
{
  struct
    {
      int i;
      void *p;
    }
  s[] =
    {
      { 1, &foo },
      { 2, NULL }
    };
}

or should that be written yet differently?  What if that's defined at the 
file scope:

struct bar
  {
    int i;
    void *p;
  }
s[] =
  {
    { 1, &foo },
    { 2, NULL }
  };

or:

struct bar
{
  int i;
  void *p;
}
s[] =
{
  { 1, &foo },
  { 2, NULL }
};

?  I recall seeing both styles (plus some variations), but my inclination 
is not to give indentation to curly brackets at the file level in any 
cases -- compare global functions vs nested functions (a GCC extension).

 Then this gets even weirder with C99's compound literals, e.g. this piece 
gets tricky:

void
baz(struct bar b[])
{
}

void
bax(void)
{
  baz ((struct bar[]) { { .i = 3, .p = &bax }, { .i = 4, .p = NULL } });
}

when you need to wrap the line (e.g. add third element), sigh...

  Maciej