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 Thu, Sep 25, 2008 at 09:26:30PM +0100, Bart Veer wrote:
    >> Although I have no doubt that a Python interface at the target
    >> vector level is possible, figuring out what such an interface
    >> should look like requires a far greater understanding of the
    >> gdb internals than I possess. I am pretty sure it would also
    >> involve much bigger changes to the internals than a one-line
    >> addition to the stratum enum. I really don't want to see the
    >> h/w debug I/O functionality to be delayed for a long time,
    >> possibly years, until all the required infrastructure is in
    >> place for a reimplementation in Python.

    Daniel> This is a reasonable argument. Unfortunately the other
    Daniel> direction is reasonable too: remember Stan's comment about
    Daniel> adding new strata? That's got maintenance cost for the
    Daniel> rest of GDB, because the strata aren't really meant to
    Daniel> work this way. And it may get in the way of future
    Daniel> development, e.g. the work that Stan and Pedro are doing
    Daniel> to support multiple processes and multi-core systems.

Adding a new stratum certainly appears to be controversial, although I
am not certain why. A grep -r stratum through the gdb sources
indicates that most files only set the to_stratum field to a known
value. As long as the order of the existing strata remains unchanged,
those files should not be affected. There are only six places, spread
over two files, where the to_stratum field is examined.

  gdbtk/generic/gdbtk-cmds.c, gdb_entry_point(), has a > check against
  dummy_stratum, to test for a fully-initialized target vector. If
  Insight is started with no initial executable and a "set hwdebug"
  command is issued before connecting to a target or specifying an
  executable file, that test could trigger where previously it would
  not. Except that "set hwdebug" will fail if there is no current
  executable, without pushing hwdebug_ops, so actually the behaviour
  is unchanged. And even if the test did trigger, all that would
  happen is that gdb_entry_point() would call entry_point_address()
  which would return 0, and the higher-level code in interface.tcl
  copes with that.

  target.c, push_stratum(). There is a >= test when deciding where to
  insert a target. My patch preserves the order of the existing
  strata. There is also an == test for replacing the current stratum.
  For now nothing else uses process_override_stratum, so adding the
  stratum won't change the behaviour.

  target.c, pop_all_targets_above(), has a > test. Again this is of no
  concern since the order of existing strata is preserved.

  target.c, target_info() has a <= test against dummy_stratum. That
  can only trigger for dummy_stratum, there is nothing lower. Adding
  process_override_stratum won't affect it.

  target.c, target_require_runnable(), has an == test against
  thread_stratum. Possibly that should be changed to >
  process_stratum. Otherwise if the hwdebug_ops are layered on top of
  another target_ops which does implement to_create_inferior, the
  runnable test will fail prematurely.

  target.c, find_core_target() has an == test against core_stratum.
  That won't be affected.

So, as far as I can tell, the only function in the current sources
that may be adversely affected by adding process_override_stratum is
target_require_runnable(), and the fix is straightforward.

Obviously if new code is written that makes assumptions about exactly
how a target vector gets constructed, that code may break after a "set
hwdebug". It is not clear why new code would want to make such
assumptions, but that is out of my control. A testcase would help a
bit, but there would be portability issues so I am not sure how useful
it would be, or how to go about writing one.

    Daniel> FWIW, I find your reason for putting it above
    Daniel> process_stratum convincing but not your reason for putting
    Daniel> it below thread_stratum.

Consider an idealized setup for debugging an embedded OS. This could
have:

  remote_ops @ process_stratum, talking to a gdb server that controls
  a jtag unit. Typically that will not provide any of the file I/O
  functionality associated with remote protocol 'F' packets. The gdb
  server will also not have any knowledge of how the OS implements
  multithreading so cannot provide any of the thread support.

  hwdebug_ops @ process_override_stratum, providing I/O functionality
  including a console for printf()'s.

  eCos_ops @ thread_stratum, currently theoretical. This has detailed
  knowledge of the internals of a specific OS so can implement e.g.
  to_find_new_threads by peeking around the appropriate kernel data
  structures.

This trio gives full debug capability using just a minimal gdb server.
Instead of remote_ops you could run on top of e.g. an architectural
simulator, without affecting hwdebug_ops or ecos_ops. For a different
embedded OS you could have e.g. FreeRTOS_ops instead of eCos_ops. That
other OS could reuse hwdebug_ops with no problems to get the file I/O
support. Or, if developing an embedded system without a multithreaded
OS, you could have nothing at thread_stratum and the file I/O support
would still be functional.

So, hwdebug_ops should not be part of the process_stratum. Merging it
into remote_ops would make the functionality inaccessible when e.g.
running on top of a simulator. Similarly hwdebug_ops should not be
part of the thread_stratum. You would end up duplicating the code in
ecos.c, freertos.c, etc.

With careful coding you could still have a module hwdebug-fileio.c
with no target_ops of its own, and depend on ecos.c, freertos.c, etc.
to merge the file I/O functionality into their thread stratum
target_ops. That seems messy, when gdb already has support for
multiple strata, and the no-OS case would be problematical.

There is still the question of whether hwdebug_ops should operate at a
stratum between process and thread, or at some stratum >
thread_stratum. Given the implementation in hwdebug-fileio.c it seems
obvious to me that the code should operate as closely as possible to
the process_stratum wait function. That avoids any overheads at the
thread stratum, e.g. detecting whether or not there has been a thread
context switch, every time the target halts for file I/O.

    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.

The existing support for multiple target_ops seems to provide exactly
the functionality that is required, and allows the h/w debug file I/O
functionality to be contained inside a single module with minimal
changes elsewhere. It does require adding a new stratum, but as far as
I can tell that is a harmless change.

The only obvious alternative I can see is to add observers to
target_wait()/load()/resume(), or to the various places where those
get invoked. That would require rather bigger changes to existing gdb
sources than the current patch. An observer would probably work fine
for load and resume, but wait is problematical. At first glance it
seems that observers are supposed to just observe, not change the flow
of control. A wait() observer could have to resume the target and
cause the target_wait() to restart, e.g. by setting a global flag.
This all seems messy compared with the current target_ops
implementation.

Bart