[PATCH] PR21985: set inferior-tty doesn't work for remote or sim
Simon Marchi
simon.marchi@polymtl.ca
Sun Sep 3 16:27:00 GMT 2017
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
More information about the Gdb-patches
mailing list