Re: add file I/O support when debugging an embedded target via jtag

[Bart Veer <bartv at ecoscentric dot com>]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

    Daniel> On Wed, Oct 01, 2008 at 07:38:22PM +0100, Bart Veer wrote:
    >> Adding a new stratum certainly appears to be controversial,
    >> although I am not certain why.

    Daniel> Because this is not about what in the current sources will
    Daniel> break. It's about the purpose of the target stack, which
    Daniel> does not include synthesizing operations like this.

I find this somewhat confusing. My understanding of the target stack
is that it allows target-related operations to be encapsulated in
independent modules. The various target_ops structures are combined
into a single current target vector, with a simple priority scheme in
the form of strata. According to my earlier investigation the
individual modules do not care whether there are five strata or 500,
as long as the existing priority ordering is preserved. I do not see
anything obvious in the source code or the gdb internals documentation
to contradict this. Further, I see code in bsd-uthread.c, aix-thread.c
and linux-thread-db.c which appears to work in much the same way as my
patch. For example bsd_uthread_wait() chains to another wait() lower
down the stack.

The current file I/O support in remote.c is handled entirely at the
target stack level, completely transparent to higher-level code like
wait_for_inferior(). It seems strange that the h/w debug file I/O now
needs to be implemented higher up.

    Daniel>> I don't see why it has to be in the target vector at all.

    >> The h/w debug file I/O code needs to take some action for every
    >> load, resume and wait operation. For the wait,
    >> deprecated_target_wait_hook provides an alternative but I
    >> assume that hook is going to disappear at some point. There are
    >> no equivalent hooks for load and resume.

    Daniel> Those operations are where your patch acts, but I don't
    Daniel> think they're what you actually want. What you do after
    Daniel> load doesn't have anything to do with "load"; I assume
    Daniel> that at load time you're setting the breakpoint, but GDB
    Daniel> already has several hooks to set internal breakpoints. And
    Daniel> it already has support for hitting an internal breakpoint,
    Daniel> hiding it from the user, and taking some action - this is
    Daniel> how SVR4 shared library support works.

That is not exactly how my patch works. The breakpoint is permanently
embedded in the target-side executable and never needs to be set by
gdb. That avoids using up one of the hardware breakpoint when
debugging an executable held in flash. However there is a target-side
flag in the .data section which determines whether or not the
application is running under gdb control. If instead the application
is running stand-alone then any file I/O operations will fail with
ENOSYS. The load operation must be detected because it will invalidate
the flag, and next subsequent resume operation must be intercepted so
that the flag can be set again.

Anyway, I have been trying to understand some of the higher-level gdb
code, to see how to get things working there. It looks like it would
take something like the following:

  1) a new post-load notifier, invoked from load_command() after the
target_load() call.

  2) a new resume notifier, invoked before the target gets resumed. I
am not totally sure where this should go, but proceed() is a likely
candidate.

  3) extensions to breakpoint.c, along the lines of
create_solib_event_breakpoint() etc. This is where things get much
more complicated. hwdebug-fileio.o is part of REMOTE_OBS alongside
remote.o, remote-fileio.o, etc., and not part of the main sources.
Hence breakpoint.c cannot have any direct dependencies on the file I/O
support, and something more generic is needed. Something along the
lines of:

  a) two new breakpoint types, bp_callback and bp_callback_no_insert.
     should_be_inserted() can distinguish between these two types.
  
  b) a new field in the breakpoint structure,
       struct bpstat_what (*callback_fn)(something_or_other)
       
  c) a new function to register a callback breakpoint
       create_callback_breakpoint (bptype, CORE_ADDR, callback_fn);
     plus of course a matching remove function.

  d) when handle_inferior_event() calls bpstat_what(), that can detect
     a callback breakpoint, invoke the callback function, and return
     its bpstat_what value. handle_inferior_event() can then be made
     to keep_going().

  e) the h/w debug file I/O code can then register a no_insert
     callback breakpoint at the appropriate address, and the callback
     function would be responsible for performing the file I/O
     operation.

  That seems to be roughly how the SVR4 shared library support works.
No doubt there would be various details to sort out. I am also rather
concerned about handle_inferior_event(). There are almost 1000 lines
of code in that function before the call to bpstat_what(), and I have
very little understanding of what most of that code does or how it
might interfere with what I am trying to achieve. For example I don't
know whether gdb is going to try to remove all breakpoints and
reinsert them later, which would mess up performance for no good
reason. I do see some handling of thread events which are of no
interest if the target is going to be resumed immediately after the
file I/O operation has completed.


Now, my current patch changes one line apiece in target.h, remote.c,
and remote-fileio.h, plus a handful of simple changes in
remote-fileio.c. Everything else is done in a self-contained new
module, courtesy of the modularity within the current target vector
implementation. Doing things above the target vector level would seem
to involve changes to the gdb core that are much much bigger, and
would add yet more complexity to existing code. That looks like a bad
idea to me.

Bart

--
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.