Re: [PATCH] PR21985: set inferior-tty doesn't work for remote or sim

[Simon Marchi <simon dot marchi at polymtl dot ca>]

Hi Jon,

Thanks for the patch and sorry for the delay.  When I try to apply it 
locally, git complains that it is corrupted.  Most likely your email 
client wrapped some lines.  To avoid this, I suggest using the "git 
send-email" command.  I was able to apply the patch you attached to the 
bug, however (I suppose it's the same).

Some notes about formatting:

 - Please verify the usage of tabs/spaces in the indentation.  8 
consecutive spaces become a tab.  There's at least one instance, in 
remote_fileio_func_write, that should be fixed.
 - There are some trailing spaces here and there, please remove them.

I have never used the simulator nor remote fileio, so this is a good 
pretext for me to learn about it.  Would it be possible for you to 
provide some small programs that I could toy with to test the patch (and 
how to run them, if you feel really nice :)) ?

I have some small comments inline:

On 2017-08-21 18:20, Jon Beniston wrote:
> The set inferior-tty command doesn't appear to work for remote or sim
> targets. (That is, remote targets using remote-fileio to have the IO 
> appear
> on the debug host).
>
> This is a shame as Eclipse (and potentially other front-ends) use this
> command to separate the windows used for target stdio and the gdb 
> console.
> Currently, therefore Eclipse isn't supporting remote and sim targets 
> fully.
>
> The attached patch adds support for this command in remote-fileio.c and
> remote-sim.c. Reads/writes from/to stdin/stdout are redirected to the 
> tty.
>
> This may not be the ideal way to implement this within GDB, but it does 
> get
> it working with Eclipse. I'm not hugely familiar with what ttys should
> offer, other than redirect stdio.

I looks right, Eclipse (or the user) should send the "set inferior-tty" 
command, but after it shouldn't have to do anything.  GDB should take 
care of it.

> diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
> index 252b423bfa..d36d2e7c43 100644
> --- a/gdb/remote-fileio.c
> +++ b/gdb/remote-fileio.c
> @@ -29,6 +29,7 @@
>  #include "target.h"
>  #include "filenames.h"
>  #include "filestuff.h"
> +#include "inferior.h"
>
>  #include <fcntl.h>
>  #include "gdb_sys_time.h"
> @@ -40,6 +41,8 @@
>  static struct {
>    int *fd_map;
>    int fd_map_size;
> +  int tty;                      /* File descriptor for tty to use if 
> set
> +                                   inferior-tty called.  */
>  } remote_fio_data;
>
>  #define FIO_FD_INVALID		-1
> @@ -52,7 +55,8 @@ static int
>  remote_fileio_init_fd_map (void)
>  {
>    int i;
> -
> +  const char *term;

You can declare the variables where they are used.

> +
>    if (!remote_fio_data.fd_map)
>      {
>        remote_fio_data.fd_map = XNEWVEC (int, 10);
> @@ -62,6 +66,15 @@ remote_fileio_init_fd_map (void)
>        remote_fio_data.fd_map[2] = FIO_FD_CONSOLE_OUT;
>        for (i = 3; i < 10; ++i)
>          remote_fio_data.fd_map[i] = FIO_FD_INVALID;
> +      term = get_inferior_io_terminal ();
> +      if (term != NULL)
> +        {
> +          remote_fio_data.tty = open (term, O_RDWR | O_NOCTTY);

This should probably use gdb_open_cloexec.

> @@ -357,15 +385,22 @@ gdb_os_flush_stdout (host_callback *p)
>  static int
>  gdb_os_write_stderr (host_callback *p, const char *buf, int len)
>  {
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (current_inferior (), 
> SIM_INSTANCE_NEEDED);
>    int i;
>    char b[2];
>
> -  for (i = 0; i < len; i++)
> +  if (sim_data->tty < 0)
>      {
> -      b[0] = buf[i];
> -      b[1] = 0;
> -      fputs_unfiltered (b, gdb_stdtargerr);
> +      for (i = 0; i < len; i++)
> +        {
> +          b[0] = buf[i];
> +          b[1] = 0;
> +          fputs_unfiltered (b, gdb_stdtargerr);
> +        }

The code in gdb_os_write_stdout has been changed long ago to use 
ui_file_write (and as you found, some unused variables were left) 
instead of this kind of loop.  Did you try to replace this loop here 
with a similar ui_file_write call?

>      }
> +  else
> +    len = write (sim_data->tty, buf, len);
>    return len;
>  }
>
> @@ -609,6 +644,7 @@ gdbsim_create_inferior (struct target_ops *target, 
> const
> char *exec_file,
>    int len;
>    char *arg_buf;
>    const char *args = allargs.c_str ();
> +  const char *term;
>
>    if (exec_file == 0 || exec_bfd == 0)
>      warning (_("No executable file specified."));
> @@ -652,6 +688,16 @@ gdbsim_create_inferior (struct target_ops *target,
> const char *exec_file,
>
>    insert_breakpoints ();	/* Needed to get correct instruction
>  				   in cache.  */
> +
> +  /* We don't do this in open as Eclispe calls set-inferior-tty after
> +     target-select, but before run.  */

Eclispe -> Eclipse.  Although I wouldn't refer to a particular 
front-end.  You could say something more generic like:

"We do this here and not in open, so that it's possible to call "set 
inferior-tty" after connecting to the target but before run."

> +  term = get_inferior_io_terminal ();
> +  if (term != NULL)
> +    {
> +      sim_data->tty = open (term, O_RDWR | O_NOCTTY);

This should also probably use gdb_open_cloexec.

> +      if (sim_data->tty < 0)
> +        error (_("Unable to open %s as inferior-tty."), term);
> +    }
>
>    clear_proceed_status (0);
>  }

Thanks,

Simon