Re: [PATCH 3/3] Fix control-c handling on Windows

[Hannes Domani <ssbssa@yahoo.de>]

 Am Freitag, 9. Dezember 2022, 16:58:12 MEZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>
> Hannes> I've now tested this a bit, it's a big improvement.
>
> Thanks for trying it.  I appreciate that.
>
> Hannes> When I first started a program with 'new-console on', then the
> Hannes> sigint_ours variable would not be initialized, and I would later get
> Hannes> a crash on C-c.
>
> I looked at this and my feeling is that this is a latent bug.
>
> I think what's going on is that sigint_ours is set under different
> conditions than it is used.
>
> That is, it is set like:
>
>   if (gdb_has_a_terminal ()
>       && tinfo->ttystate != NULL
>       && sharing_input_terminal (inf))
> [...]
>       if (!job_control)
>     {
>       sigint_ours = install_sigint_handler (SIG_IGN);
> [...]
>       gdb_tty_state = target_terminal_state::is_inferior;
>
>
> However later it is used:
>
>   if (gdb_tty_state != desired_state)
> [...]
>       if (!job_control && desired_state == target_terminal_state::is_ours)
>     {
>       install_sigint_handler (sigint_ours);
>
> It's maybe hard to reason about but it seems to me that there's some
> possibility for the value to be used even though it hasn't been set, and
> I suspect that is what you are seeing.
>
> It might be useful if you could confirm this.  Just some simple logging
> at these two points would be sufficient.
>
> If that's the issue then I can write a patch to change sigint_ours to be
> a gdb::optional and check it that way.

I should have been more clear about this.
I started with 'new-console on', so sharing_input_terminal() returned false,
and that's why sigint_ours was not set.

So yes, gdb::optional would probably fix this.
I just wonder if this is never an issue on Linux, e.g. if you attach,
of does signal() maybe ignore NULL-pointer functions?


> Hannes> And for some reason one of my builds (x86_64+TUI+python) needed the
> Hannes> #include <signal.h> in mingw-hdep.c, but my other (i686 basic) didn't.
>
> I didn't see this but I went ahead and added the include to my patch,
> since it seems harmless.

Great, thanks.


> Hannes> But my basic i686 build had another problem when starting a program with
> Hannes> 'new-console on', because at program start it called install_sigint_handler(),
> Hannes> but rl_set_signals() would later override the SIGINT handler again, so C-c
> Hannes> didn't work work in this situation.
> Hannes> With 'new-console off' this didn't happen, since install_sigint_handler() was
> Hannes> again called later since it shared the console.
>
> I really don't understand the interaction between signal and
> SetConsoleCtrlHandler.  I tried searching for some docs on this but
> didn't find anything that was really enlightening.

As far as I could tell, signal() calls SetConsoleCtrlHandler(), probably
similar to how you handled this.


> Also it's somewhat surprising that the x86 and x86-64 builds would be
> different in this regard.
>
> Anyway ... I'm not sure what to do here yet.  The interactions with
> readline are pretty hard to understand.  I guess the question is where
> should install_sigint_handler be called where it is not called -- that
> is, to reset the signals from readline.  Alternatively, where is it
> called on x86-64 but not on x86?
>
> I did try 'new-console on' with my (x86-64) build, and that worked fine.
> I can try an x86 build and see if that works any better.

Again, I wasn't clear enough here.
The difference is not because of i686 and x86_64, but that the x86_64 build
has TUI+python enabled, but my i686 build has not.
Some TUI startup code would later call install_sigint_handler() again,
overriding the SIGINT handler again, and everything is fine.


Sorry that I wasn't clear enough before.


Hannes