Re: RFA: Fix compile time warning in gdb/mips-tdep.c

[Andrew Cagney <ac131313 at cygnus dot com>]

Andrew Cagney wrote:
> 
> Nick Clifton wrote:
> >
> > Hi Guys,
> >
> >   Whilst working on something else I noticed a warning from gcc about
> >   a variable in unpack_mips16() which was being used before it was
> >   initialised.  Looking at the code concerned it was obvious that it
> >   was wrong ('extension' is used without being initialised), but it
> >   was not obvious what the programmer actually intended.  Below is a
> >   patch that represent my best guess as to what should actually be
> >   happening.
> >
> >   Is the patch right ?  Can I apply it ?
> 
> I'm not sure - I don't have the correct manual handy.  For the moment,
> I'll simply disable this code.  As you note it is clearly wrong yet no
> one has noticed.

[time passes]

Nick,

I think the code is simply bogus so I'm going to pull it - I don't think
it ever did anything useful.  I'll try to explain the rationale below
(and then run some tests to prove it to my self :-).

In MIPS16 there is an extended instruction which in memory appears as:

	PC+0: EXTENDED instruction (opcode 30)
	PC+2: MIPS16 instruction
	PC+4:

the instruction pair is executed as a single unit (well it should be).


Problem one:

The function mips16_next_pc() takes as its argument the address of an
instruction and returns the address of the next instruction.  If the
instruction happens to be ``extended'' it should combine the two half
instructions into a single 32 bit instruction and analyse that.  The
code:

	case 30;   /* This is an extend instruction */
	  pc += 4; /* Dont be setting breakpints on the second half */
	  break;

completely fails to do that - it assumes that extended instructions
never branch and consequently PC+4 is always the destination.  This
isn't correct.  Branch instructions can be extended (Ref MIPS16 doc from
somewhere on SGI's WEB site).  I'd guess that GCC rarely generates them
so no one has ever noticed this bug.


Problem two:

The function unpack_mips16() appears to be trying to look one
instruction back in the instruction stream to determine if the MIPS16
instruction at the current PC was extended.  As you've noted it gets
that operation completely wrong and we can be fairly sure that the
existing code (non-deterministally) never worked.

Making the code even more puzzling is the fact that the function
unpack_mips16() is only ever called by mips16_next_pc() and then only
when there is a non extended branch instruction that needs to be
analysed (hope I'm reading that bit correct).  The only way the function
could be called with an extended instruction prefix is if GDB somehow
managed to halt the program midway through an extended instruction
(perhaps the sim does this - ugh?) or the user set the PC to the mid
point of an extended instruction.  In the former case it would be a sim
bug, in the latter case well the programmer gets exactly what they
deserve :-)


So what was the code ment to do?

I think the intention was that unpack_mips16() was called with the
extended argument as an argument (default of zero).  No one ever got
around to implementing this.

	enjoy,
		Andrew

> 
> > Cheers
> >         Nick
> >
> > 2000-08-07  Nick Clifton  <nickc@cygnus.com>
> >
> >         * mips-tdep.c (unpack_mips16): Fix the initialisation of
> >         'extension' and 'extended'.
> >
> > Index: gdb/mips-tdep.c
> > ===================================================================
> > RCS file: /cvs/src//src/gdb/mips-tdep.c,v
> > retrieving revision 1.29
> > diff -p -r1.29 mips-tdep.c
> > *** mips-tdep.c 2000/07/30 01:48:26     1.29
> > --- mips-tdep.c 2000/08/07 23:53:03
> > *************** unpack_mips16 (CORE_ADDR pc,
> > *** 915,928 ****
> >     CORE_ADDR extpc;
> >     unsigned long extension;
> >     int extended;
> > !   extpc = (pc - 4) & ~0x01;   /* Extensions are 32 bit instructions */
> > !   /* Decrement to previous address and loose the 16bit mode flag */
> > !   /* return if the instruction was extendable, but not actually extended */
> >     extended = ((mips32_op (extension) == 30) ? 1 : 0);
> > !   if (extended)
> > !     {
> > !       extension = mips_fetch_instruction (extpc);
> > !     }
> >     switch (upk->fmt)
> >       {
> >       case itype:
> > --- 915,927 ----
> >     CORE_ADDR extpc;
> >     unsigned long extension;
> >     int extended;
> > !
> > !   /* Decrement to previous address and loose the 16bit mode flag.  */
> > !   extpc = (pc - 4) & ~0x01;
> > !   /* Extensions are 32 bit instructions.  */
> > !   extension = mips_fetch_instruction (extpc);
> >     extended = ((mips32_op (extension) == 30) ? 1 : 0);
> > !
> >     switch (upk->fmt)
> >       {
> >       case itype: