Re: [WIP] TI msp430 CIO support

[Yao Qi <yao at codesourcery dot com>]

On 05/17/2013 12:23 PM, Kevin Buettner wrote:
> I don't intend to commit this patch as is, but I wanted to post it
> because it provides useful functionality for msp430 programming and
> debugging.  I have found it very useful for running the GDB test suite
> because it provides the necessary functionality for printf() and
> write() to work.
>
> TI has an I/O mechanism used by their compiler and libraries that they
> call CIO.  In a nutshell, it defines a small number of operations
> such as open, close, read, write, plus a few others.  Drivers exist at
> several levels for implementing this functionality.  E.g. there are
> a board level drivers that causes I/O to occur against devices on the
> board.  When the target is connected to a debugger, a debug-based
> driver is available which causes I/O to be performed on the host
> running GDB (or some other debugger).
>
> The debugger based driver uses a simple breakpoint driven
> implementation.  The debugger places a breakpoint on a known location
> which is always called when debugger-based I/O is to be performed.
> When the breakpoint at that location is hit, the debugger reads the
> details of the system call and its parameters from a memory based
> buffer.  The debugger writes back the output of the system call to the
> same buffer.  (See my patch for the exact details.)

Kevin,
TI C6X uses the same CIO mechanism too, but it was implemented in the 
stub side instead of the GDB side.  The stub has to query to GDB about 
the address some special symbols and manage the breakpoint by itself. 
Your explanation to CIO is quite clear, and I suggest that we can move 
this explanation to the head comment of msp430-cio.c.

> One of my early implementations of this functionality used a normal
> (but hidden) GDB breakpoint with a command list whose last command
> was "continue".  This is similar to the way that dprintf is implemented.
> This implementation had a serious flaw, however, in that it doesn't
> work correctly when attempting to use "next" to step over a statement
> which performs one of these CIO operations.  (I think that other commands
> such as "finish" and "until" have similar issues.)
>
> The current dprintf implementation can be used to illustrate this
> flaw.  Here's an example using the test case from gdb.base/break.exp:

We realized this flaw and try to fix it.  Hui is pushing the fix forward 
and the recent patch is posted here.

  [RFC] PR 15075 dprintf interferes with "next"
  http://sourceware.org/ml/gdb-patches/2013-05/msg00574.html

Hope this patch fixes the issue you encountered.

> I finally settled on using the shared library machinery because I was
> able to obtain correct behavior.  The only good thing about doing it
> this way is that it works correctly.  I do not want this patch to go
> in as is because the shared library mechanism should not be subverted
> for this purpose.  That's why this patch is a work-in-progress instead
> of one for which I seek comments or approval.
>
> In order for this work to go into the GDB sources, the use of the
> shared library machinery needs to be removed and replaced with some
> other kind of breakpoint machinery which still provides correct
> behavior.   This same mechanism could be used to improve dprintf
> behavior as well.

That sounds good.


> diff -uprN ../../src/gdb/msp430-cio.c ./msp430-cio.c
> --- ../../src/gdb/msp430-cio.c	1969-12-31 17:00:00.000000000 -0700
> +++ ./msp430-cio.c	2013-05-16 09:37:39.100986167 -0700

Looks CIO is widely used in different TI processors, so probably we can 
rename this file to cio.c or ti-cio.c.

> +
> +/* Create the C I/O breakpoint when the `inferior_created' observer
> +   is invoked.  */
> +
> +static void
> +cio_inferior_created (struct target_ops *target, int from_tty)
> +{
> +  int status;
> +  struct minimal_symbol *ciobuf_sym, *cio_bp_sym;
> +  struct breakpoint *cio_breakpoint;
> +  struct cio_state *cio_state = get_cio_state ();
> +
> +  /* Look up the CIO buffer using several different variations.  */
> +  ciobuf_sym = lookup_minimal_symbol ("_CIOBUF_", NULL, NULL);
> +
> +  if (ciobuf_sym == NULL)
> +    ciobuf_sym = lookup_minimal_symbol ("__CIOBUF_", NULL, NULL);
> +
> +  if (ciobuf_sym == NULL)
> +    ciobuf_sym = lookup_minimal_symbol ("__CIOBUF__", NULL, NULL);
> +
> +  /* Look up the symbol at which to place a breakpoint.  */
> +  cio_bp_sym = lookup_minimal_symbol ("C$$IO$$", NULL, NULL);
> +
> +  /* Delete CIO breakpoint from earlier runs.  */
> +  remove_solib_event_breakpoints ();
> +
> +  /* Can't do CIO if the required symbols do not exist.  */
> +  if (ciobuf_sym == NULL || cio_bp_sym == NULL)
> +    return;
> +
> +  cio_state->ciobuf_addr = SYMBOL_VALUE_ADDRESS (ciobuf_sym);
> +  cio_breakpoint = create_solib_event_breakpoint
> +                    (get_current_arch (), SYMBOL_VALUE_ADDRESS (cio_bp_sym));

Probably, we also need to lookup symbol "C$$EXIT" and set breakpoint on it.

> +/* Do the actual work associated with reading a CIO command, its parameters,
> +   etc, performing the I/O operation, and then writing back the result.  */
> +
> +static void
> +cio_do_cio (void)
> +{
> +  gdb_byte lcp[11];	/* length, command, params. */
> +  gdb_byte *data = NULL;
> +  gdb_byte *in_params, *out_params;
> +  int len, cmd;

I prefer to define a struct to represent the data in CIO buffer, like this:

struct __attribute__((packed)) cio_open_to_host
{
  /* Suggested file descriptor (little endian).  */
  short fd;
  /* Flags (little endian).  */
  short flags;
  /* Mode (little endian).  */
  uint32_t mode;
};

struct __attribute__((packed)) cio_to_host
{
  /* Data length.  */
  uint32_t length;
  /* Command.  */
  gdb_byte command;
  /* Parameters.  */
  union
  {
    gdb_byte buf[8];
    struct cio_open_to_host open;
    ....
  } parms;
  /* Variable-length data.  */
  gdb_byte data[];
};

then, the code is much more readable, IMO.

--
Yao (éå)