Re: [RFC] mips-tdep.c: Fix mips16 bit rot

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

Hi Kevin,

> Sorry it's taken me so long to get back to you this.  I will try to
> answer your questions to the best of my ability, but it's been a
> while since I've looked at or thought about this code.  I fear
> that I might not be of much help...

 No worries, I keep being distracted by various stuff too.  Thanks for 
your time.

> >  First of all this construct used in a few places:
> > 
> > +  if (is_mips16_addr (pc))
> > +    pc = unmake_mips16_addr (pc);
> > 
> > makes me a bit nervous you might be removing the ISA bit for code 
> > references where it is the only means of signalling GDB the address is a 
> > MIPS16 code address -- that would happen if pc pointed to a location that 
> > has no associated symbol information.  While in this case the 
> > functionality GDB provides is limited, it still has to work correctly up 
> > to expectations, e.g. instruction-level single-stepping has to work and 
> > where software stepping is used the MIPS16 BREAK instruction encoding has 
> > to be used rather than the MIPS32 one.  Have you verified this 
> > functionality has not regressed?  Similarly the MIPS16 heuristic frame 
> > unwinder may be affected.
> > 
> >  Otherwise I'd just be tempted to change all these cases into something 
> > functionally equivalent to:
> > 
> > +  if (msymbol_is_special (lookup_minimal_symbol_by_pc (pc)))
> > +    pc = unmake_mips16_addr (pc);
> > 
> > where the ISA bit is only stripped if there's symbol information 
> > indicating this is a piece of MIPS16 code so there's no information lost.
> 
> I see your point.  You proposed change looks reasonable to me.
> 
> >  Second, you only made corresponding adjustments to 
> > mips_eabi_push_dummy_call and mips_o64_push_dummy_call which are the least 
> > standard and probably the least used MIPS ABIs.  Any particular reason you 
> > did not make similar changes to mips_o32_push_dummy_call or 
> > mips_n32n64_push_dummy_call?
> > 
> >  Also I see you only adjust function pointers that are arguments on their 
> > own -- isn't a similar adjustment required for such pointers that are 
> > parts of aggregate types as well?
> 
> I don't remember enough about what I did roughly a year ago to be able
> to answer this.  I think it's likely that changes should be made to the
> areas that you've identified.
> 
> >  Overall, what was the rationale behing your change? -- as it's unclear to 
> > me from your e-mail.  Did you just want to fix test results you discovered 
> > that were quite poor or does this change address problems you stumbled 
> > across during actual MIPS16 debugging?
> 
> The former - I was attempting to fix some very bad test results with respect
> to mips16.

 So essentially at this point, after some testing and lots of thinking, 
I'd like to revert your change entirely and make additional changes 
elsewhere, so that the ISA bit is actually preserved all the time.  This 
reflects the program's view of the world which I think GDB should mimic.  
Unless proved otherwise, I think any other approach is unworkable and is 
simply missing the point.

 I've opened a discussion and sent a proposal here:

http://sourceware.org/ml/gdb-patches/2012-05/msg00515.html

-- feel free to join if interested.

  Maciej