Re: SH breakpoint problem

[Kevin Buettner <kevinb at cygnus dot com>]

On Aug 9, 10:05pm, Jonathan Larmour wrote:

> Kevin Buettner wrote:
> > 
> > On Aug 9,  8:48pm, Jonathan Larmour wrote:
> > > I tried Kevin's suggestion anyway, and hit a problem in that even if I
> > > return the correct address from sh_skip_prologue(), the code that calls it
> > > in find_function_start_sal() in symtab.c "corrects" it due to the check:
> > >
> > > 2055    /* Check if SKIP_PROLOGUE left us in mid-line, and the next
> > > 2056       line is still part of the same function.  */
> > >
> > > So even if we do correct it, it will still put it back :-|.
> > 
> > You might try defining PROLOGUE_FIRSTLINE_OVERLAP for your port.  (This
> > might cause other things to break though.)
> 
> This would be defined for the whole SH architecture then.

Not necessarily.  You could put the define in your target specific
tm-*.h file.  BTW, for PROLOGUE_FIRSTLINE_OVERLAP to survive (long
term), it will have to be multiarched and it'll be the responsibility
of the target's tdep.c file to make sure it gets set correctly.

> What ramifications would it have?  i.e. what would/could break?

It's been a number of years since I tinkered with it.  My recollection
is that I didn't always see the correct values of the parameters when
stopping at the beginning of a function.  This problem could be fixed
in the prologue analyzer though.  It could also be fixed by making some
improvements to the core of gdb.

> It does seem a bit odd that after going to lengths to analyze the prologue
> etc. to determine the right address, it goes and basically ignores that in
> favour of the debug info. Why did it bother since it already knew the
> address of the start of the function?

I've sometimes wondered about this myself.  I think the answer is that
it doesn't necessarily trust that the prologue analyzer will have
gotten all the way to the end of the prologue.  Therefore, it attempts
to adjust the address based upon the debug info.  For your scenario,
this is definitely wrong.

> > > Perhaps it would be better (assuming it is allowed) to modify the PC
> > > in sh_breakpoint_from_pc()?
> > 
> > It is allowed, but is generally used for such things as clearing
> > bits which might make the address odd (see mips).
> > 
> > I don't recall the details, but changing the address in
> > *_breakpoint_from_pc() didn't work when I had a (sort of) similar
> > problem about a year ago.  I ended up submitting the following patch
> > which may be used for adjusting the breakpoint address:
> > 
> >     http://sources.redhat.com/ml/gdb-patches/2000-06/msg00257.html
> > 
> > Unfortunately, this patch needs a bit of work before it can go in.
> > (See Eli Zaretskii's comments.)
> 
> I'm surprised a function like this only needs using in such a small number
> of places, rather than whereever BREAKPOINT_FROM_PC is used. I suppose I
> can't really see the difference between them :-|.

On mips, all that's happening is that the breakpoint address is being
cleaned up a bit so that the breakpoint will get written to the
correct address.  Ideally, BREAKPOINT_FROM_PC() should only be
returning the sequence of bits that represents a breakpoint at the
given address.  It shouldn't have to muck with the address at all.

In any event, BREAKPOINT_FROM_PC() is called when it's time to obtain
insert a breakpoint on the target.  TARGET_ADJUST_BREAKPOINT_ADDRESS
is called to fix up the breakpoint address at the time the breakpoint
is created.  The user is warned if the breakpoint is actually moved.
(I believe that one of Eli's criticism's of the patch is that it should
also warn you when you hit one of these breakpoints which was moved.)
If we put the warning in BREAKPOINT_FROM_PC(), we'd see the warning
*every time* that breakpoints were inserted or removed from the target.
This happens a lot and would be extremely undesirable.

> Also, I can't help but think people might be annoyed that "b *0x12345678"
> may not actually set a bp at exactly that address. As for hardware
> breakpoints, I don't know either what would be different.

I agree; it is annoying when your breakpoints are moved on you.  OTOH,
for the architecture that I created this patch for, it was *impossible*
to set breakpoints at certain addresses due to architectural limitations.
Yet, the compiler would still indicate these addresses in its debug
info.

> > > One semi-related question: sh_breakpoint_from_pc() in sh-tdep.c says:
> > >
> > >   /* 0xc3c3 is trapa #c3, and it works in big and little endian modes */
> > >   static unsigned char breakpoint[] =  {0xc3, 0xc3};
> > >
> > > whereas config/sh/tm-sh.h says:
> > >
> > > #define BIG_REMOTE_BREAKPOINT    { 0xc3, 0x20 } /* Used in remote.c */
> > > #define LITTLE_REMOTE_BREAKPOINT { 0x20, 0xc3 } /* Used in remote.c */
> > >
> > > Which is right?
> > 
> > Maybe both of them?
> > 
> > They're both trapa instructions.  The sh_breakpoint_from_pc() version
> > is ``trapa #c3'' and the {BIG,LITTLE}_REMOTE_BREAKPOINT version is
> > ``trapa #20''.
> 
> But on the target, we have to know what traps are assigned to breakpoints,
> and what are free for other uses (genuine traps).

I agree.

> Is there a right answer, or should we just pick one?

It seems to me that the answer is target dependent and you've stumbled
across a case where two different targets encode the trap number via
different mechanisms.  For the remote board, the
{BIG,LITTLE}_REMOTE_BREAKPOINT encoding is used.  For a native port,
the sh_breakpoint_from_pc() encoding is used.

Ideally, the various target specific encodings ought to be collected
together in sh-tdep.c.  The multiarch mechanism ought to be able to
decide (or be told) which one to use...  Another solution is to
create a target specific tdep.c file which has specifies alternate
methods for things like BREAKPOINT_FROM_PC().