This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC][PATCH][PR gdb/8527] Fix interrupt (^C) in Solaris


> This patch addresses an issue in Solaris, bug 8527.
> 
> The problem:
> 
> If gdb attaches to a process that either has no controlling
> terminal, or the controlling terminal differs from the one
> gdb is running under, hitting ^C doesn't interrupt the
> debugged process.
> 
> The change makes it possible to interrupt/resume
> without any apparent problems, but I don't know the
> ramifications of these changes.  I ran across this
> issue and a fix that was proposed 12 years ago.
> 
> Is this a reasonable change?
> 
> -brian

> gdb/Changelog:
> 2015-12-16  Brian Vandenberg <phantall@gmail.com>
> 
> 	PR gdb/8527
> 	* gdb/procfs.c (proc_wait_for_stop): Added set_sigint_trap() & clear_sigint_trap()

FWIW, we have had this change in AdaCore's version since 2007
at least (probably even earlier, but digging deeper is more
difficult), and we haven't heard of problems because of it.
Given that procfs.c now appears to be used by Solaris targets
only (x86, x86_64, sparc and sparc64), I propose that we integrate
that patch.

In terms of the patch's submission, I have a small number of
small comments (everything small :-)). 

First, in the ChangeLog is almost perfect.
  - two spaces after your name, before the email address.
  - Remove the "gdb/" in "gdb/procfs.c", since the name of the file
    is relative to the ChangeLog, which is in gdb/.
  - Also, we limit the length of each line to 79-80 characters.
  - small nitpick, I think "and" is better than "&" in this case,
    as "&" has a mathematical connotation for me;
  - we say "call to set_sigint_trap" rather than "set_sigint_trap()".
  - and we end sentences with periods (GNU Coding Style).

Therefore:

gdb/Changelog:
2015-12-16  Brian Vandenberg  <phantall@gmail.com>

	PR gdb/8527
	* procfs.c (proc_wait_for_stop): Added call to set_sigint_trap
        and clear_sigint_trap.

> diff --git a/gdb/procfs.c b/gdb/procfs.c
> index 7b7ff45..7cd870c 100644
> --- a/gdb/procfs.c
> +++ b/gdb/procfs.c
> @@ -1487,7 +1487,12 @@ proc_wait_for_stop (procinfo *pi)
>    {
>      procfs_ctl_t cmd = PCWSTOP;
> 
> +    set_sigint_trap();

I would add a comment before the call to set_sigint_trap,
to confirm to the reader that we are doing this in order to
resolve an issue. Something close to what you wrote in your
email seems to fit the bill...

>      win = (write (pi->ctl_fd, (char *) &cmd, sizeof (cmd)) == sizeof (cmd));
> +
> +    clear_sigint_trap();
> +
>      /* We been runnin' and we stopped -- need to update status.  */
>      pi->status_valid = 0;
>    }

And finally; this patch is fairly tiny, so there should be no
copyright issue preventing us from incorporating it. But I also
see that you have several contributions pending, and so, for us
to continue accepting patches from you, it would be better if
you had a copyright assignment on file. Has anyone asked you about
that?

Eventually, when the paperwork is done, the only requirement
for getting "write after approval" access to the repo is to submit
one good patch, which I think you have met, or will meet very
shortly.

Thanks,
-- 
Joel


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]