* [PATCH 1/3] Pass inferior to terminal_save_inferior @ 2018-10-16 3:38 Simon Marchi 2018-10-16 3:38 ` [PATCH 2/3] Pass inferior to terminal_inferior Simon Marchi ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Simon Marchi @ 2018-10-16 3:38 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Instead of relying on the current inferior, pass an inferior pointer to the target implementing terminal_save_inferior. There should be no change in behavior. I added documentation to terminal_save_inferior, as I understand it (maybe I understood it wrong, so please take a look). gdb/ChangeLog: * target.h (struct target_ops) <terminal_save_inferior>: Add inferior pointer parameter. * target.c (target_terminal_is_ours_kind): Pass inferior to terminal_save_inferior instead of changing current inferior. * target-delegates.c: Re-generate. * inf-child.h (inf_child_target) <terminal_save_inferior>: Add inferior pointer parameter. * inf-child.c (inf_child_target::terminal_save_inferior): Likewise. * inferior.h (child_terminal_save_inferior): Likewise. * inflow.c (child_terminal_save_inferior): Likewise. --- gdb/inf-child.c | 4 ++-- gdb/inf-child.h | 2 +- gdb/inferior.h | 3 ++- gdb/inflow.c | 3 +-- gdb/target-delegates.c | 15 ++++++++------- gdb/target.c | 5 +---- gdb/target.h | 5 ++++- 7 files changed, 19 insertions(+), 18 deletions(-) diff --git a/gdb/inf-child.c b/gdb/inf-child.c index 44aa2f66fbfe..94f42ec2adc0 100644 --- a/gdb/inf-child.c +++ b/gdb/inf-child.c @@ -114,9 +114,9 @@ inf_child_target::terminal_inferior () } void -inf_child_target::terminal_save_inferior () +inf_child_target::terminal_save_inferior (inferior *inf) { - child_terminal_save_inferior (this); + child_terminal_save_inferior (this, inf); } void diff --git a/gdb/inf-child.h b/gdb/inf-child.h index 98969bc5fa31..b0625391ebb2 100644 --- a/gdb/inf-child.h +++ b/gdb/inf-child.h @@ -46,7 +46,7 @@ public: bool supports_terminal_ours () override; void terminal_init () override; void terminal_inferior () override; - void terminal_save_inferior () override; + void terminal_save_inferior (inferior *inf) override; void terminal_ours_for_output () override; void terminal_ours () override; void terminal_info (const char *, int) override; diff --git a/gdb/inferior.h b/gdb/inferior.h index af5e92019618..d09263e34bef 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -154,7 +154,8 @@ extern void child_terminal_ours_for_output (struct target_ops *self); extern void child_terminal_inferior (struct target_ops *self); -extern void child_terminal_save_inferior (struct target_ops *self); +extern void child_terminal_save_inferior (struct target_ops *self, + inferior *inf); extern void child_terminal_init (struct target_ops *self); diff --git a/gdb/inflow.c b/gdb/inflow.c index caff64620709..372ef7844f01 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -454,13 +454,12 @@ child_terminal_ours (struct target_ops *self) cache. */ void -child_terminal_save_inferior (struct target_ops *self) +child_terminal_save_inferior (struct target_ops *self, inferior *inf) { /* Avoid attempting all the ioctl's when running in batch. */ if (!gdb_has_a_terminal ()) return; - inferior *inf = current_inferior (); terminal_info *tinfo = get_inflow_inferior_data (inf); /* No need to save/restore if the inferior is not sharing GDB's diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index 03136dfacf25..f2b352c44888 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -45,7 +45,7 @@ struct dummy_target : public target_ops bool supports_terminal_ours () override; void terminal_init () override; void terminal_inferior () override; - void terminal_save_inferior () override; + void terminal_save_inferior (inferior *arg0) override; void terminal_ours_for_output () override; void terminal_ours () override; void terminal_info (const char *arg0, int arg1) override; @@ -212,7 +212,7 @@ struct debug_target : public target_ops bool supports_terminal_ours () override; void terminal_init () override; void terminal_inferior () override; - void terminal_save_inferior () override; + void terminal_save_inferior (inferior *arg0) override; void terminal_ours_for_output () override; void terminal_ours () override; void terminal_info (const char *arg0, int arg1) override; @@ -1249,22 +1249,23 @@ debug_target::terminal_inferior () } void -target_ops::terminal_save_inferior () +target_ops::terminal_save_inferior (inferior *arg0) { - this->beneath ()->terminal_save_inferior (); + this->beneath ()->terminal_save_inferior (arg0); } void -dummy_target::terminal_save_inferior () +dummy_target::terminal_save_inferior (inferior *arg0) { } void -debug_target::terminal_save_inferior () +debug_target::terminal_save_inferior (inferior *arg0) { fprintf_unfiltered (gdb_stdlog, "-> %s->terminal_save_inferior (...)\n", this->beneath ()->shortname ()); - this->beneath ()->terminal_save_inferior (); + this->beneath ()->terminal_save_inferior (arg0); fprintf_unfiltered (gdb_stdlog, "<- %s->terminal_save_inferior (", this->beneath ()->shortname ()); + target_debug_print_inferior_p (arg0); fputs_unfiltered (")\n", gdb_stdlog); } diff --git a/gdb/target.c b/gdb/target.c index 2d98954b54ac..93d16b90f179 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -511,10 +511,7 @@ target_terminal_is_ours_kind (target_terminal_state desired_state) ALL_INFERIORS (inf) { if (inf->terminal_state == target_terminal_state::is_inferior) - { - set_current_inferior (inf); - current_top_target ()->terminal_save_inferior (); - } + current_top_target ()->terminal_save_inferior (inf); } ALL_INFERIORS (inf) diff --git a/gdb/target.h b/gdb/target.h index a3000c80c641..c37405205a0a 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -583,7 +583,10 @@ struct target_ops TARGET_DEFAULT_IGNORE (); virtual void terminal_inferior () TARGET_DEFAULT_IGNORE (); - virtual void terminal_save_inferior () + + /* If INF shares a terminal with GDB, save the current terminal settings + in a data structure associated to INF. */ + virtual void terminal_save_inferior (inferior *inf) TARGET_DEFAULT_IGNORE (); virtual void terminal_ours_for_output () TARGET_DEFAULT_IGNORE (); -- 2.19.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] Pass inferior to terminal_inferior 2018-10-16 3:38 [PATCH 1/3] Pass inferior to terminal_save_inferior Simon Marchi @ 2018-10-16 3:38 ` Simon Marchi 2018-10-22 20:54 ` Pedro Alves 2018-10-16 3:38 ` [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) Simon Marchi 2018-10-22 20:54 ` [PATCH 1/3] Pass inferior to terminal_save_inferior Pedro Alves 2 siblings, 1 reply; 17+ messages in thread From: Simon Marchi @ 2018-10-16 3:38 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Same as the previous patch, but for the terminal_inferior method. gdb/ChangeLog: * target.h (struct target_ops) <terminal_inferior>: Add inferior pointer parameter. * target.c (target_terminal::inferior): Pass inferior to terminal_inferior. * target-delegates.c: Re-generate. * inf-child.h (inf_child_target) <terminal_inferior>: Add inferior pointer parameter. * inf-child.c (inf_child_target::terminal_inferior): Likewise. * inferior.h (child_terminal_inferior): Likewise. * inflow.c (child_terminal_inferior): Likewise. * remote.c (struct remote_target) <terminal_inferior>: Likewise. (remote_target::terminal_inferior): Likewise. --- gdb/inf-child.c | 4 ++-- gdb/inf-child.h | 2 +- gdb/inferior.h | 2 +- gdb/inflow.c | 3 +-- gdb/remote.c | 4 ++-- gdb/target-delegates.c | 15 ++++++++------- gdb/target.c | 5 ++--- gdb/target.h | 9 ++++++++- 8 files changed, 25 insertions(+), 19 deletions(-) diff --git a/gdb/inf-child.c b/gdb/inf-child.c index 94f42ec2adc0..f02720028211 100644 --- a/gdb/inf-child.c +++ b/gdb/inf-child.c @@ -108,9 +108,9 @@ inf_child_target::terminal_init () } void -inf_child_target::terminal_inferior () +inf_child_target::terminal_inferior (inferior *inf) { - child_terminal_inferior (this); + child_terminal_inferior (this, inf); } void diff --git a/gdb/inf-child.h b/gdb/inf-child.h index b0625391ebb2..c544ceaf520e 100644 --- a/gdb/inf-child.h +++ b/gdb/inf-child.h @@ -45,7 +45,7 @@ public: bool supports_terminal_ours () override; void terminal_init () override; - void terminal_inferior () override; + void terminal_inferior (inferior *inf) override; void terminal_save_inferior (inferior *inf) override; void terminal_ours_for_output () override; void terminal_ours () override; diff --git a/gdb/inferior.h b/gdb/inferior.h index d09263e34bef..badd82e17c35 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -152,7 +152,7 @@ extern void child_terminal_ours (struct target_ops *self); extern void child_terminal_ours_for_output (struct target_ops *self); -extern void child_terminal_inferior (struct target_ops *self); +extern void child_terminal_inferior (struct target_ops *self, inferior *inf); extern void child_terminal_save_inferior (struct target_ops *self, inferior *inf); diff --git a/gdb/inflow.c b/gdb/inflow.c index 372ef7844f01..e355f4aa9fc5 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -342,7 +342,7 @@ sharing_input_terminal (inferior *inf) preparation for starting or resuming the inferior. */ void -child_terminal_inferior (struct target_ops *self) +child_terminal_inferior (struct target_ops *self, inferior *inf) { /* If we resume more than one inferior in the foreground on GDB's terminal, then the first inferior's terminal settings "win". @@ -353,7 +353,6 @@ child_terminal_inferior (struct target_ops *self) if (gdb_tty_state == target_terminal_state::is_inferior) return; - inferior *inf = current_inferior (); terminal_info *tinfo = get_inflow_inferior_data (inf); if (gdb_has_a_terminal () diff --git a/gdb/remote.c b/gdb/remote.c index c53553af5bda..268cce84afa8 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -557,7 +557,7 @@ public: int can_do_single_step () override; - void terminal_inferior () override; + void terminal_inferior (inferior *inf) override; void terminal_ours () override; @@ -6812,7 +6812,7 @@ remote_target::interrupt_query () is required. */ void -remote_target::terminal_inferior () +remote_target::terminal_inferior (inferior *inf) { /* NOTE: At this point we could also register our selves as the recipient of all input. Any characters typed could then be diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index f2b352c44888..1d1bd0ae4234 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -44,7 +44,7 @@ struct dummy_target : public target_ops int can_do_single_step () override; bool supports_terminal_ours () override; void terminal_init () override; - void terminal_inferior () override; + void terminal_inferior (inferior *arg0) override; void terminal_save_inferior (inferior *arg0) override; void terminal_ours_for_output () override; void terminal_ours () override; @@ -211,7 +211,7 @@ struct debug_target : public target_ops int can_do_single_step () override; bool supports_terminal_ours () override; void terminal_init () override; - void terminal_inferior () override; + void terminal_inferior (inferior *arg0) override; void terminal_save_inferior (inferior *arg0) override; void terminal_ours_for_output () override; void terminal_ours () override; @@ -1229,22 +1229,23 @@ debug_target::terminal_init () } void -target_ops::terminal_inferior () +target_ops::terminal_inferior (inferior *arg0) { - this->beneath ()->terminal_inferior (); + this->beneath ()->terminal_inferior (arg0); } void -dummy_target::terminal_inferior () +dummy_target::terminal_inferior (inferior *arg0) { } void -debug_target::terminal_inferior () +debug_target::terminal_inferior (inferior *arg0) { fprintf_unfiltered (gdb_stdlog, "-> %s->terminal_inferior (...)\n", this->beneath ()->shortname ()); - this->beneath ()->terminal_inferior (); + this->beneath ()->terminal_inferior (arg0); fprintf_unfiltered (gdb_stdlog, "<- %s->terminal_inferior (", this->beneath ()->shortname ()); + target_debug_print_inferior_p (arg0); fputs_unfiltered (")\n", gdb_stdlog); } diff --git a/gdb/target.c b/gdb/target.c index 93d16b90f179..631b2c3b0f12 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -444,7 +444,7 @@ target_terminal::inferior (void) if (inf->terminal_state != target_terminal_state::is_inferior) { - current_top_target ()->terminal_inferior (); + current_top_target ()->terminal_inferior (inf); inf->terminal_state = target_terminal_state::is_inferior; } @@ -479,8 +479,7 @@ target_terminal::restore_inferior (void) { if (inf->terminal_state == target_terminal_state::is_ours_for_output) { - set_current_inferior (inf); - current_top_target ()->terminal_inferior (); + current_top_target ()->terminal_inferior (inf); inf->terminal_state = target_terminal_state::is_inferior; } } diff --git a/gdb/target.h b/gdb/target.h index c37405205a0a..310c942bbe5b 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -581,7 +581,14 @@ struct target_ops TARGET_DEFAULT_RETURN (false); virtual void terminal_init () TARGET_DEFAULT_IGNORE (); - virtual void terminal_inferior () + + /* If INF shares a terminal with GDB, restore the saved terminal settings + associated to INF (see terminal_save_inferior) to the current + terminal. In a scenario where multiple inferiors share GDB's terminal, + don't apply the settings of another inferior's settings are currently + applied (in other words, the first inferior's settings win and should not + be overwritten). */ + virtual void terminal_inferior (inferior *inf) TARGET_DEFAULT_IGNORE (); /* If INF shares a terminal with GDB, save the current terminal settings -- 2.19.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] Pass inferior to terminal_inferior 2018-10-16 3:38 ` [PATCH 2/3] Pass inferior to terminal_inferior Simon Marchi @ 2018-10-22 20:54 ` Pedro Alves 2018-10-23 9:14 ` Simon Marchi 0 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2018-10-22 20:54 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 10/16/2018 04:38 AM, Simon Marchi wrote: > diff --git a/gdb/target.h b/gdb/target.h > index c37405205a0a..310c942bbe5b 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -581,7 +581,14 @@ struct target_ops > TARGET_DEFAULT_RETURN (false); > virtual void terminal_init () > TARGET_DEFAULT_IGNORE (); > - virtual void terminal_inferior () > + > + /* If INF shares a terminal with GDB, restore the saved terminal settings > + associated to INF (see terminal_save_inferior) to the current > + terminal. In a scenario where multiple inferiors share GDB's terminal, > + don't apply the settings of another inferior's settings are currently s/of/if/. > + applied (in other words, the first inferior's settings win and should not > + be overwritten). */ > + virtual void terminal_inferior (inferior *inf) > TARGET_DEFAULT_IGNORE (); > > /* If INF shares a terminal with GDB, save the current terminal settings > OK. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] Pass inferior to terminal_inferior 2018-10-22 20:54 ` Pedro Alves @ 2018-10-23 9:14 ` Simon Marchi 0 siblings, 0 replies; 17+ messages in thread From: Simon Marchi @ 2018-10-23 9:14 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 2018-10-22 16:54, Pedro Alves wrote: > On 10/16/2018 04:38 AM, Simon Marchi wrote: > >> diff --git a/gdb/target.h b/gdb/target.h >> index c37405205a0a..310c942bbe5b 100644 >> --- a/gdb/target.h >> +++ b/gdb/target.h >> @@ -581,7 +581,14 @@ struct target_ops >> TARGET_DEFAULT_RETURN (false); >> virtual void terminal_init () >> TARGET_DEFAULT_IGNORE (); >> - virtual void terminal_inferior () >> + >> + /* If INF shares a terminal with GDB, restore the saved terminal >> settings >> + associated to INF (see terminal_save_inferior) to the current >> + terminal. In a scenario where multiple inferiors share GDB's >> terminal, >> + don't apply the settings of another inferior's settings are >> currently > > s/of/if/. Fixed. >> + applied (in other words, the first inferior's settings win and >> should not >> + be overwritten). */ > >> + virtual void terminal_inferior (inferior *inf) >> TARGET_DEFAULT_IGNORE (); >> >> /* If INF shares a terminal with GDB, save the current terminal >> settings >> > > OK. Thanks. Simon ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) 2018-10-16 3:38 [PATCH 1/3] Pass inferior to terminal_save_inferior Simon Marchi 2018-10-16 3:38 ` [PATCH 2/3] Pass inferior to terminal_inferior Simon Marchi @ 2018-10-16 3:38 ` Simon Marchi 2018-10-17 17:05 ` Pedro Alves 2018-10-22 20:54 ` [PATCH 1/3] Pass inferior to terminal_save_inferior Pedro Alves 2 siblings, 1 reply; 17+ messages in thread From: Simon Marchi @ 2018-10-16 3:38 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Here's a summary of PR 23368: #include <unistd.h> int main (void) { char *exec_args[] = { "/bin/ls", NULL }; execve (exec_args[0], exec_args, NULL); } $ gdb -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run ... [1] + 13146 suspended (tty output) gdb -q -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run $ Here's what happens: when the inferior execs with "follow-exec-mode new", we first "mourn" it before creating the new one. This ends up calling inflow_inferior_exit, which sets the per-inferior terminal state to "is_ours": inf->terminal_state = target_terminal_state::is_ours; At this point, the inferior's terminal_state is is_ours, while the "reality", tracked by gdb_tty_state, is is_inferior (GDB doesn't own the terminal). Later, we continue processing the exec inferior event and decide we want to stop (because of the "catch exec") and call target_terminal::ours to make sure we own the terminal. However, we don't actually go to the target backend to change the settings, because the core thinks that no inferior owns the terminal (inf->terminal_state is target_terminal_state::is_ours, as checked in target_terminal_is_ours_kind, for both inferiors). When something in readline tries to mess with the terminal settings, it generates a SIGTTOU. This patch manages to fix this particular case by calling target_terminal::ours() in inflow_inferior_exit. This makes so that inflow actually changes the terminal settings so that GDB owns it, which avoids the SIGTTOU later. The buildbot doesn't complain, but I am not sure this is the most bestest way to fix this, maybe it's just papering over the actual problem or introduces some latent bug. In particular, I am not sure if this is correct in case we have multiple inferiors sharing the same terminal. But I thought I would still submit this patch to get the ball rolling. gdb/ChangeLog: PR gdb/23368 * inflow.c (inflow_inferior_exit): Update doc. Call target_terminal::ours. --- gdb/inflow.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/gdb/inflow.c b/gdb/inflow.c index e355f4aa9fc5..95e195167ef7 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -653,18 +653,15 @@ get_inflow_inferior_data (struct inferior *inf) return info; } -/* This is a "inferior_exit" observer. Releases the TERMINAL_INFO member - of the inferior structure. This field is private to inflow.c, and - its type is opaque to the rest of GDB. PID is the target pid of - the inferior that is about to be removed from the inferior - list. */ +/* This is a "inferior_exit" observer. Releases the terminal info associated + to INF. */ static void inflow_inferior_exit (struct inferior *inf) { struct terminal_info *info; - inf->terminal_state = target_terminal_state::is_ours; + target_terminal::ours (); info = (struct terminal_info *) inferior_data (inf, inflow_inferior_data); if (info != NULL) -- 2.19.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) 2018-10-16 3:38 ` [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) Simon Marchi @ 2018-10-17 17:05 ` Pedro Alves 2018-10-20 15:14 ` Simon Marchi 0 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2018-10-17 17:05 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 10/16/2018 04:38 AM, Simon Marchi wrote: > Here's a summary of PR 23368: > > #include <unistd.h> > int main (void) > { > char *exec_args[] = { "/bin/ls", NULL }; > execve (exec_args[0], exec_args, NULL); > } > > $ gdb -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run > ... > [1] + 13146 suspended (tty output) gdb -q -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run > $ > > Here's what happens: when the inferior execs with "follow-exec-mode > new", we first "mourn" it before creating the new one. This ends up > calling inflow_inferior_exit, which sets the per-inferior terminal state > to "is_ours": > > inf->terminal_state = target_terminal_state::is_ours; > > At this point, the inferior's terminal_state is is_ours, while the > "reality", tracked by gdb_tty_state, is is_inferior (GDB doesn't own the > terminal). > > Later, we continue processing the exec inferior event and decide we want > to stop (because of the "catch exec") and call target_terminal::ours to > make sure we own the terminal. However, we don't actually go to the > target backend to change the settings, because the core thinks that no > inferior owns the terminal (inf->terminal_state is > target_terminal_state::is_ours, as checked in > target_terminal_is_ours_kind, for both inferiors). ^^^^^^^^^^^^^^^^^^ I think the "for both inferiors" part is what's dubious here. The process lives on in the new inferior, but we lost its terminal settings! Seems to me that they should be migrated from the old inferior to the new one. And then the problem sorts itself out, because then the new inferior will have target_terminal_state::is_inferior state. > When something in > readline tries to mess with the terminal settings, it generates a > SIGTTOU. > > This patch manages to fix this particular case by calling > target_terminal::ours() in inflow_inferior_exit. This makes so that > inflow actually changes the terminal settings so that GDB owns it, which > avoids the SIGTTOU later. > > The buildbot doesn't complain, but I am not sure this is the most > bestest way to fix this, maybe it's just papering over the actual > problem or introduces some latent bug. In particular, I am not sure if > this is correct in case we have multiple inferiors sharing the same > terminal. But I thought I would still submit this patch to get the ball > rolling. Yeah, I don't think this is right in the multi-inferior (or multi-target) case. It may happen to just work because we always stop everything when an inferior exits. In the exec case, if you don't catch the exec, then we won't stop; likely it ends up working because we end up calling target_terminal::inferior() on resume. Still, I think this is a hack that would likely cause trouble down the road. > > gdb/ChangeLog: > > PR gdb/23368 > * inflow.c (inflow_inferior_exit): Update doc. Call > target_terminal::ours. > --- > gdb/inflow.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/gdb/inflow.c b/gdb/inflow.c > index e355f4aa9fc5..95e195167ef7 100644 > --- a/gdb/inflow.c > +++ b/gdb/inflow.c > @@ -653,18 +653,15 @@ get_inflow_inferior_data (struct inferior *inf) > return info; > } > > -/* This is a "inferior_exit" observer. Releases the TERMINAL_INFO member > - of the inferior structure. This field is private to inflow.c, and > - its type is opaque to the rest of GDB. PID is the target pid of > - the inferior that is about to be removed from the inferior > - list. */ > +/* This is a "inferior_exit" observer. Releases the terminal info associated > + to INF. */ > > static void > inflow_inferior_exit (struct inferior *inf) > { > struct terminal_info *info; > > - inf->terminal_state = target_terminal_state::is_ours; > + target_terminal::ours (); > > info = (struct terminal_info *) inferior_data (inf, inflow_inferior_data); > if (info != NULL) Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) 2018-10-17 17:05 ` Pedro Alves @ 2018-10-20 15:14 ` Simon Marchi 2018-10-20 15:38 ` Simon Marchi 0 siblings, 1 reply; 17+ messages in thread From: Simon Marchi @ 2018-10-20 15:14 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 2018-10-17 1:04 p.m., Pedro Alves wrote: > On 10/16/2018 04:38 AM, Simon Marchi wrote: >> Here's a summary of PR 23368: >> >> #include <unistd.h> >> int main (void) >> { >> char *exec_args[] = { "/bin/ls", NULL }; >> execve (exec_args[0], exec_args, NULL); >> } >> >> $ gdb -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run >> ... >> [1] + 13146 suspended (tty output) gdb -q -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run >> $ >> >> Here's what happens: when the inferior execs with "follow-exec-mode >> new", we first "mourn" it before creating the new one. This ends up >> calling inflow_inferior_exit, which sets the per-inferior terminal state >> to "is_ours": >> >> inf->terminal_state = target_terminal_state::is_ours; >> >> At this point, the inferior's terminal_state is is_ours, while the >> "reality", tracked by gdb_tty_state, is is_inferior (GDB doesn't own the >> terminal). >> >> Later, we continue processing the exec inferior event and decide we want >> to stop (because of the "catch exec") and call target_terminal::ours to >> make sure we own the terminal. However, we don't actually go to the >> target backend to change the settings, because the core thinks that no >> inferior owns the terminal (inf->terminal_state is >> target_terminal_state::is_ours, as checked in >> target_terminal_is_ours_kind, for both inferiors). > ^^^^^^^^^^^^^^^^^^ > > I think the "for both inferiors" part is what's dubious here. > > The process lives on in the new inferior, but we lost its > terminal settings! Seems to me that they should be migrated > from the old inferior to the new one. And then the problem > sorts itself out, because then the new inferior will have > target_terminal_state::is_inferior state. Yeah that makes sense. This crossed my mind when I look at the issue, but for some reason I didn't went that route. But now that you say it, it appears obvious that this should be the fix. I just tried copying the inferior::terminal_state value from the old inferior to the new inferior, and it fixes the problem. But actually, we should also be transferring all of the struct terminal_info associated to the inferior, is that right? > >> When something in >> readline tries to mess with the terminal settings, it generates a >> SIGTTOU. >> >> This patch manages to fix this particular case by calling >> target_terminal::ours() in inflow_inferior_exit. This makes so that >> inflow actually changes the terminal settings so that GDB owns it, which >> avoids the SIGTTOU later. >> >> The buildbot doesn't complain, but I am not sure this is the most >> bestest way to fix this, maybe it's just papering over the actual >> problem or introduces some latent bug. In particular, I am not sure if >> this is correct in case we have multiple inferiors sharing the same >> terminal. But I thought I would still submit this patch to get the ball >> rolling. > > Yeah, I don't think this is right in the multi-inferior (or multi-target) > case. It may happen to just work because we always stop everything > when an inferior exits. In the exec case, if you don't catch the exec, > then we won't stop; likely it ends up working because we end up calling > target_terminal::inferior() on resume. Still, I think this is > a hack that would likely cause trouble down the road. Ok. Thanks, Simon ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) 2018-10-20 15:14 ` Simon Marchi @ 2018-10-20 15:38 ` Simon Marchi 2018-10-22 20:56 ` Pedro Alves 0 siblings, 1 reply; 17+ messages in thread From: Simon Marchi @ 2018-10-20 15:38 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 2018-10-20 11:13 a.m., Simon Marchi wrote: > On 2018-10-17 1:04 p.m., Pedro Alves wrote: >> On 10/16/2018 04:38 AM, Simon Marchi wrote: >>> Here's a summary of PR 23368: >>> >>> #include <unistd.h> >>> int main (void) >>> { >>> char *exec_args[] = { "/bin/ls", NULL }; >>> execve (exec_args[0], exec_args, NULL); >>> } >>> >>> $ gdb -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run >>> ... >>> [1] + 13146 suspended (tty output) gdb -q -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run >>> $ >>> >>> Here's what happens: when the inferior execs with "follow-exec-mode >>> new", we first "mourn" it before creating the new one. This ends up >>> calling inflow_inferior_exit, which sets the per-inferior terminal state >>> to "is_ours": >>> >>> inf->terminal_state = target_terminal_state::is_ours; >>> >>> At this point, the inferior's terminal_state is is_ours, while the >>> "reality", tracked by gdb_tty_state, is is_inferior (GDB doesn't own the >>> terminal). >>> >>> Later, we continue processing the exec inferior event and decide we want >>> to stop (because of the "catch exec") and call target_terminal::ours to >>> make sure we own the terminal. However, we don't actually go to the >>> target backend to change the settings, because the core thinks that no >>> inferior owns the terminal (inf->terminal_state is >>> target_terminal_state::is_ours, as checked in >>> target_terminal_is_ours_kind, for both inferiors). >> ^^^^^^^^^^^^^^^^^^ >> >> I think the "for both inferiors" part is what's dubious here. >> >> The process lives on in the new inferior, but we lost its >> terminal settings! Seems to me that they should be migrated >> from the old inferior to the new one. And then the problem >> sorts itself out, because then the new inferior will have >> target_terminal_state::is_inferior state. > > Yeah that makes sense. This crossed my mind when I look at the issue, > but for some reason I didn't went that route. But now that you say it, > it appears obvious that this should be the fix. > > I just tried copying the inferior::terminal_state value from the old > inferior to the new inferior, and it fixes the problem. But > actually, we should also be transferring all of the struct terminal_info > associated to the inferior, is that right? This would be the updated patch (testing on the buildbot at the moment). From c54c1dc3f124aef65c8a1daf042ded2b6b4a8a46 Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@polymtl.ca> Date: Sat, 20 Oct 2018 10:57:33 -0400 Subject: [PATCH] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) Here's a summary of PR 23368: #include <unistd.h> int main (void) { char *exec_args[] = { "/bin/ls", NULL }; execve (exec_args[0], exec_args, NULL); } $ gdb -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run ... [1] + 13146 suspended (tty output) gdb -q -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run $ Here's what happens: when the inferior execs with "follow-exec-mode new", we first "mourn" it before creating the new one. This ends up calling inflow_inferior_exit, which sets the per-inferior terminal state to "is_ours": inf->terminal_state = target_terminal_state::is_ours; At this point, the inferior's terminal_state is is_ours, while the "reality", tracked by gdb_tty_state, is is_inferior (GDB doesn't own the terminal). Later, we continue processing the exec inferior event and decide we want to stop (because of the "catch exec") and call target_terminal::ours to make sure we own the terminal. However, we don't actually go to the target backend to change the settings, because the core thinks that no inferior owns the terminal (inf->terminal_state is target_terminal_state::is_ours, as checked in target_terminal_is_ours_kind, for both inferiors). When something in readline tries to mess with the terminal settings, it generates a SIGTTOU. This patch fixes this by tranferring the state of the terminal from the old inferior to the new inferior. gdb/ChangeLog: * infrun.c (follow_exec): In the follow_exec_mode_new case, transfer terminal state from old new new inferior. --- gdb/infrun.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 72e249617670..85a5200ddf4b 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1189,12 +1189,13 @@ follow_exec (ptid_t ptid, char *exec_file_target) /* The user wants to keep the old inferior and program spaces around. Create a new fresh one, and switch to it. */ - /* Do exit processing for the original inferior before adding - the new inferior so we don't have two active inferiors with - the same ptid, which can confuse find_inferior_ptid. */ + /* Do exit processing for the original inferior before setting the new + inferior's pid. Having two inferiors with the same pid would confuse + find_inferior_p(t)id. */ + inf = add_inferior_with_spaces (); + copy_terminal_info (inf, current_inferior ()); exit_inferior_silent (current_inferior ()); - inf = add_inferior_with_spaces (); inf->pid = pid; target_follow_exec (inf, exec_file_target); -- 2.19.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) 2018-10-20 15:38 ` Simon Marchi @ 2018-10-22 20:56 ` Pedro Alves 2018-10-23 9:49 ` Simon Marchi 0 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2018-10-22 20:56 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 10/20/2018 04:38 PM, Simon Marchi wrote: > On 2018-10-20 11:13 a.m., Simon Marchi wrote: >> On 2018-10-17 1:04 p.m., Pedro Alves wrote: >>> I think the "for both inferiors" part is what's dubious here. >>> >>> The process lives on in the new inferior, but we lost its >>> terminal settings! Seems to me that they should be migrated >>> from the old inferior to the new one. And then the problem >>> sorts itself out, because then the new inferior will have >>> target_terminal_state::is_inferior state. >> >> Yeah that makes sense. This crossed my mind when I look at the issue, >> but for some reason I didn't went that route. But now that you say it, >> it appears obvious that this should be the fix. >> >> I just tried copying the inferior::terminal_state value from the old >> inferior to the new inferior, and it fixes the problem. But >> actually, we should also be transferring all of the struct terminal_info >> associated to the inferior, is that right? > > This would be the updated patch (testing on the buildbot at the moment). Yes. Looks good. I think you could avoid the copying-with-garanteed-deleting by adding a "swap_terminal_info" function that swaps the terminal_info and terminal_state between the inferiors. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) 2018-10-22 20:56 ` Pedro Alves @ 2018-10-23 9:49 ` Simon Marchi 2018-10-23 10:55 ` Pedro Alves 2018-10-24 9:41 ` Andreas Schwab 0 siblings, 2 replies; 17+ messages in thread From: Simon Marchi @ 2018-10-23 9:49 UTC (permalink / raw) To: Pedro Alves, Simon Marchi, gdb-patches On 2018-10-22 4:56 p.m., Pedro Alves wrote: > On 10/20/2018 04:38 PM, Simon Marchi wrote: >> On 2018-10-20 11:13 a.m., Simon Marchi wrote: >>> On 2018-10-17 1:04 p.m., Pedro Alves wrote: >>>> I think the "for both inferiors" part is what's dubious here. >>>> >>>> The process lives on in the new inferior, but we lost its >>>> terminal settings! Seems to me that they should be migrated >>>> from the old inferior to the new one. And then the problem >>>> sorts itself out, because then the new inferior will have >>>> target_terminal_state::is_inferior state. >>> >>> Yeah that makes sense. This crossed my mind when I look at the issue, >>> but for some reason I didn't went that route. But now that you say it, >>> it appears obvious that this should be the fix. >>> >>> I just tried copying the inferior::terminal_state value from the old >>> inferior to the new inferior, and it fixes the problem. But >>> actually, we should also be transferring all of the struct terminal_info >>> associated to the inferior, is that right? >> >> This would be the updated patch (testing on the buildbot at the moment). > > Yes. Looks good. I think you could avoid the > copying-with-garanteed-deleting by adding a "swap_terminal_info" function > that swaps the terminal_info and terminal_state between the inferiors. Like this? (commit message and ChangeLog to be updated) From a43edf2aada8d5312370429a4d7fad7cfb1ec85c Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@polymtl.ca> Date: Sat, 20 Oct 2018 10:57:33 -0400 Subject: [PATCH] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) Here's a summary of PR 23368: #include <unistd.h> int main (void) { char *exec_args[] = { "/bin/ls", NULL }; execve (exec_args[0], exec_args, NULL); } $ gdb -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run ... [1] + 13146 suspended (tty output) gdb -q -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run $ Here's what happens: when the inferior execs with "follow-exec-mode new", we first "mourn" it before creating the new one. This ends up calling inflow_inferior_exit, which sets the per-inferior terminal state to "is_ours": inf->terminal_state = target_terminal_state::is_ours; At this point, the inferior's terminal_state is is_ours, while the "reality", tracked by gdb_tty_state, is is_inferior (GDB doesn't own the terminal). Later, we continue processing the exec inferior event and decide we want to stop (because of the "catch exec") and call target_terminal::ours to make sure we own the terminal. However, we don't actually go to the target backend to change the settings, because the core thinks that no inferior owns the terminal (inf->terminal_state is target_terminal_state::is_ours, as checked in target_terminal_is_ours_kind, for both inferiors). When something in readline tries to mess with the terminal settings, it generates a SIGTTOU. This patch fixes this by tranferring the state of the terminal from the old inferior to the new inferior. gdb/ChangeLog: * infrun.c (follow_exec): In the follow_exec_mode_new case, transfer terminal state from old new new inferior. --- gdb/inflow.c | 16 ++++++++++++++++ gdb/infrun.c | 9 +++++---- gdb/terminal.h | 3 +++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/gdb/inflow.c b/gdb/inflow.c index e355f4aa9fc..d673bef8b43 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -700,6 +700,22 @@ copy_terminal_info (struct inferior *to, struct inferior *from) to->terminal_state = from->terminal_state; } +/* See terminal.h. */ + +void +swap_terminal_info (inferior *a, inferior *b) +{ + terminal_info *info_a + = (struct terminal_info *) inferior_data (a, inflow_inferior_data); + terminal_info *info_b + = (struct terminal_info *) inferior_data (a, inflow_inferior_data); + + set_inferior_data (a, inflow_inferior_data, info_b); + set_inferior_data (b, inflow_inferior_data, info_a); + + std::swap (a->terminal_state, b->terminal_state); +} + void info_terminal_command (const char *arg, int from_tty) { diff --git a/gdb/infrun.c b/gdb/infrun.c index 72e24961767..cedaaecd91a 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1189,12 +1189,13 @@ follow_exec (ptid_t ptid, char *exec_file_target) /* The user wants to keep the old inferior and program spaces around. Create a new fresh one, and switch to it. */ - /* Do exit processing for the original inferior before adding - the new inferior so we don't have two active inferiors with - the same ptid, which can confuse find_inferior_ptid. */ + /* Do exit processing for the original inferior before setting the new + inferior's pid. Having two inferiors with the same pid would confuse + find_inferior_p(t)id. */ + inf = add_inferior_with_spaces (); + swap_terminal_info (inf, current_inferior ()); exit_inferior_silent (current_inferior ()); - inf = add_inferior_with_spaces (); inf->pid = pid; target_follow_exec (inf, exec_file_target); diff --git a/gdb/terminal.h b/gdb/terminal.h index 7774eefcd46..da27e46f6ec 100644 --- a/gdb/terminal.h +++ b/gdb/terminal.h @@ -29,6 +29,9 @@ extern void new_tty_postfork (void); extern void copy_terminal_info (struct inferior *to, struct inferior *from); +/* Exchange the terminal info and state between inferiors A and B. */ +extern void swap_terminal_info (inferior *a, inferior *b); + extern pid_t create_tty_session (void); /* Set up a serial structure describing standard input. In inflow.c. */ -- 2.19.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) 2018-10-23 9:49 ` Simon Marchi @ 2018-10-23 10:55 ` Pedro Alves 2018-10-24 9:41 ` Andreas Schwab 1 sibling, 0 replies; 17+ messages in thread From: Pedro Alves @ 2018-10-23 10:55 UTC (permalink / raw) To: Simon Marchi, Simon Marchi, gdb-patches On 10/23/2018 10:48 AM, Simon Marchi wrote: > On 2018-10-22 4:56 p.m., Pedro Alves wrote: >> Yes. Looks good. I think you could avoid the >> copying-with-garanteed-deleting by adding a "swap_terminal_info" function >> that swaps the terminal_info and terminal_state between the inferiors. > > Like this? (commit message and ChangeLog to be updated) > Yes, like that. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) 2018-10-23 9:49 ` Simon Marchi 2018-10-23 10:55 ` Pedro Alves @ 2018-10-24 9:41 ` Andreas Schwab 2018-10-24 14:08 ` Simon Marchi 1 sibling, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2018-10-24 9:41 UTC (permalink / raw) To: Simon Marchi; +Cc: Pedro Alves, Simon Marchi, gdb-patches That's still broken: $ ./gdb gdb GNU gdb (GDB) 8.2.50.20181024-git Copyright (C) 2018 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "ia64-unknown-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from gdb... Setting up the environment for debugging gdb. During symbol reading: unsupported tag: 'DW_TAG_unspecified_type' Breakpoint 1 at 0x400000000018fc00: file ../../gdb/common/errors.c, line 51. During symbol reading: Member function "~_Sp_counted_base" (offset 0x517e90) is virtual but the vtable offset is not specified During symbol reading: cannot get low and high bounds for subprogram DIE at 0x5201ee During symbol reading: missing name for subprogram DIE at 0x5246c8 During symbol reading: Child DIE 0x52f12a and its abstract origin 0x52cafc have different parents During symbol reading: Multiple children of DIE 0x53138e refer to DIE 0x52be3c as their abstract origin During symbol reading: DW_AT_call_target target DIE has invalid low pc, for referencing DIE 0x53a7e0 [in module /usr/local/gcc/gdb/Build/gdb/gdb] Breakpoint 2 at 0x4000000000150ac0: file ../../gdb/cli/cli-cmds.c, line 197. (top-gdb) r Starting program: /usr/local/gcc/gdb/Build/gdb/gdb During symbol reading: .debug_line section has line program sequence without an end [1]+ Stopped ./gdb gdb $ fg ./gdb gdb Failed to read a valid object file image from memory. During symbol reading: .debug_line section has line program sequence without an end During symbol reading: .debug_line section has line program sequence without an end [Detaching after vfork from child process 11772] GNU gdb (GDB) 8.2.50.20181024-git Copyright (C) 2018 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "ia64-unknown-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word". Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) 2018-10-24 9:41 ` Andreas Schwab @ 2018-10-24 14:08 ` Simon Marchi 0 siblings, 0 replies; 17+ messages in thread From: Simon Marchi @ 2018-10-24 14:08 UTC (permalink / raw) To: Andreas Schwab; +Cc: Simon Marchi, Pedro Alves, gdb-patches On 2018-10-24 05:41, Andreas Schwab wrote: > That's still broken: > > $ ./gdb gdb > GNU gdb (GDB) 8.2.50.20181024-git > Copyright (C) 2018 Free Software Foundation, Inc. > License GPLv3+: GNU GPL version 3 or later > <http://gnu.org/licenses/gpl.html> > This is free software: you are free to change and redistribute it. > There is NO WARRANTY, to the extent permitted by law. > Type "show copying" and "show warranty" for details. > This GDB was configured as "ia64-unknown-linux-gnu". > Type "show configuration" for configuration details. > For bug reporting instructions, please see: > <http://www.gnu.org/software/gdb/bugs/>. > Find the GDB manual and other documentation resources online at: > <http://www.gnu.org/software/gdb/documentation/>. > > For help, type "help". > Type "apropos word" to search for commands related to "word"... > Reading symbols from gdb... > Setting up the environment for debugging gdb. > During symbol reading: unsupported tag: 'DW_TAG_unspecified_type' > Breakpoint 1 at 0x400000000018fc00: file ../../gdb/common/errors.c, > line 51. > During symbol reading: Member function "~_Sp_counted_base" (offset > 0x517e90) is virtual but the vtable offset is not specified > During symbol reading: cannot get low and high bounds for subprogram > DIE at 0x5201ee > During symbol reading: missing name for subprogram DIE at 0x5246c8 > During symbol reading: Child DIE 0x52f12a and its abstract origin > 0x52cafc have different parents > During symbol reading: Multiple children of DIE 0x53138e refer to DIE > 0x52be3c as their abstract origin > During symbol reading: DW_AT_call_target target DIE has invalid low > pc, for referencing DIE 0x53a7e0 [in module > /usr/local/gcc/gdb/Build/gdb/gdb] > Breakpoint 2 at 0x4000000000150ac0: file ../../gdb/cli/cli-cmds.c, line > 197. > (top-gdb) r > Starting program: /usr/local/gcc/gdb/Build/gdb/gdb > During symbol reading: .debug_line section has line program sequence > without an end > > [1]+ Stopped ./gdb gdb > $ fg > ./gdb gdb > Failed to read a valid object file image from memory. > During symbol reading: .debug_line section has line program sequence > without an end > During symbol reading: .debug_line section has line program sequence > without an end > [Detaching after vfork from child process 11772] > GNU gdb (GDB) 8.2.50.20181024-git > Copyright (C) 2018 Free Software Foundation, Inc. > License GPLv3+: GNU GPL version 3 or later > <http://gnu.org/licenses/gpl.html> > This is free software: you are free to change and redistribute it. > There is NO WARRANTY, to the extent permitted by law. > Type "show copying" and "show warranty" for details. > This GDB was configured as "ia64-unknown-linux-gnu". > Type "show configuration" for configuration details. > For bug reporting instructions, please see: > <http://www.gnu.org/software/gdb/bugs/>. > Find the GDB manual and other documentation resources online at: > <http://www.gnu.org/software/gdb/documentation/>. > > For help, type "help". > Type "apropos word" to search for commands related to "word". > > Andreas. That looks like a different case of SIGTTOU, same symptom but different root cause. I didn't claim I fixed all of them :) Simon ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Pass inferior to terminal_save_inferior 2018-10-16 3:38 [PATCH 1/3] Pass inferior to terminal_save_inferior Simon Marchi 2018-10-16 3:38 ` [PATCH 2/3] Pass inferior to terminal_inferior Simon Marchi 2018-10-16 3:38 ` [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) Simon Marchi @ 2018-10-22 20:54 ` Pedro Alves 2018-10-23 9:01 ` Simon Marchi 2 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2018-10-22 20:54 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 10/16/2018 04:38 AM, Simon Marchi wrote: > Instead of relying on the current inferior, pass an inferior pointer to > the target implementing terminal_save_inferior. There should be no > change in behavior. > Your original patch 3/3 ended up not using the inferior pointer. :-) I suppose that this doesn't hurt, like the equivalent target_detach change. > I added documentation to terminal_save_inferior, as I understand it > (maybe I understood it wrong, so please take a look). That looks good. (please recall to update commit log.) > diff --git a/gdb/target.c b/gdb/target.c > index 2d98954b54ac..93d16b90f179 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -511,10 +511,7 @@ target_terminal_is_ours_kind (target_terminal_state desired_state) > ALL_INFERIORS (inf) > { > if (inf->terminal_state == target_terminal_state::is_inferior) > - { > - set_current_inferior (inf); > - current_top_target ()->terminal_save_inferior (); > - } > + current_top_target ()->terminal_save_inferior (inf); > } With multi-target, current_top_target() will depend on the current inferior, so I'll need to put that set_current_inferior call back. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Pass inferior to terminal_save_inferior 2018-10-22 20:54 ` [PATCH 1/3] Pass inferior to terminal_save_inferior Pedro Alves @ 2018-10-23 9:01 ` Simon Marchi 2018-10-23 11:11 ` Pedro Alves 0 siblings, 1 reply; 17+ messages in thread From: Simon Marchi @ 2018-10-23 9:01 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 2018-10-22 16:54, Pedro Alves wrote: > On 10/16/2018 04:38 AM, Simon Marchi wrote: >> Instead of relying on the current inferior, pass an inferior pointer >> to >> the target implementing terminal_save_inferior. There should be no >> change in behavior. >> > > Your original patch 3/3 ended up not using the inferior pointer. :-) > > I suppose that this doesn't hurt, like the equivalent target_detach > change. Well, the original 3/3 patch doesn't change the implementation of terminal_save_inferior, it changes the inferior_exit observer, so it's not really related to this change. >> I added documentation to terminal_save_inferior, as I understand it >> (maybe I understood it wrong, so please take a look). > > That looks good. (please recall to update commit log.) > >> diff --git a/gdb/target.c b/gdb/target.c >> index 2d98954b54ac..93d16b90f179 100644 >> --- a/gdb/target.c >> +++ b/gdb/target.c >> @@ -511,10 +511,7 @@ target_terminal_is_ours_kind >> (target_terminal_state desired_state) >> ALL_INFERIORS (inf) >> { >> if (inf->terminal_state == target_terminal_state::is_inferior) >> - { >> - set_current_inferior (inf); >> - current_top_target ()->terminal_save_inferior (); >> - } >> + current_top_target ()->terminal_save_inferior (inf); >> } > With multi-target, current_top_target() will depend on > the current inferior, so I'll need to put that > set_current_inferior call back. Can't you access the inferior's target stack directly instead of changing the current inferior? Simon ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Pass inferior to terminal_save_inferior 2018-10-23 9:01 ` Simon Marchi @ 2018-10-23 11:11 ` Pedro Alves 2018-10-23 20:56 ` Simon Marchi 0 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2018-10-23 11:11 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On 10/23/2018 10:00 AM, Simon Marchi wrote: > On 2018-10-22 16:54, Pedro Alves wrote: >> On 10/16/2018 04:38 AM, Simon Marchi wrote: >>> Instead of relying on the current inferior, pass an inferior pointer to >>> the target implementing terminal_save_inferior. There should be no >>> change in behavior. >>> >> >> Your original patch 3/3 ended up not using the inferior pointer. :-) >> >> I suppose that this doesn't hurt, like the equivalent target_detach >> change. > > Well, the original 3/3 patch doesn't change the implementation of terminal_save_inferior, it changes the inferior_exit observer, so it's not really related to this change. Oh, in that case it'd be clearer to not bundle it in the same series then. > >>> I added documentation to terminal_save_inferior, as I understand it >>> (maybe I understood it wrong, so please take a look). >> >> That looks good. (please recall to update commit log.) >> >>> diff --git a/gdb/target.c b/gdb/target.c >>> index 2d98954b54ac..93d16b90f179 100644 >>> --- a/gdb/target.c >>> +++ b/gdb/target.c >>> @@ -511,10 +511,7 @@ target_terminal_is_ours_kind (target_terminal_state desired_state) >>>   ALL_INFERIORS (inf) >>>     { >>>       if (inf->terminal_state == target_terminal_state::is_inferior) >>> -   { >>> -     set_current_inferior (inf); >>> -     current_top_target ()->terminal_save_inferior (); >>> -   } >>> +   current_top_target ()->terminal_save_inferior (inf); >>>     } >> With multi-target, current_top_target() will depend on >> the current inferior, so I'll need to put that >> set_current_inferior call back. > > Can't you access the inferior's target stack directly instead of changing the current inferior? I can for the call to the top target, but then the problem is the beneath() calls in all the target-delegates.c delegating implementations. E.g. here: void -target_ops::terminal_save_inferior () +target_ops::terminal_save_inferior (inferior *arg0) { - this->beneath ()->terminal_save_inferior (); + this->beneath ()->terminal_save_inferior (arg0); ^^^^^^^^^ } because beneath() looks at the target stack of the current inferior. It would need to look for the target beneath in the target stack of the arg0 inferior instead. Otherwise you start the top target call in inferior B, and then cross the the beneath target of inferior A (the current inferior). Whoops. At some point in the branch I made target_ops::beneath take an optional inferior pointer, but when I stumbled on this issue in target-delegates.c I ended up reverting it, as it wasn't easy to fix. I think that we could maybe teach make-target-delegates to automatically emit void target_ops::method (inferior *arg0) { this->beneath (arg0)->method (arg0); } and: void target_ops::method (thread_info *arg0) { this->beneath (arg0->inf)->method (arg0); } iff the method's first parameter is an inferior or thread_info pointer. But that was just an idea, I never toyed with it, because it would be a detour. So I gave up on the inferior parameter to target beneath, and thought I'd better focus instead of getting the multi-target basics in first, even if that means we need to swap current inferior/thread here and there, as usual. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Pass inferior to terminal_save_inferior 2018-10-23 11:11 ` Pedro Alves @ 2018-10-23 20:56 ` Simon Marchi 0 siblings, 0 replies; 17+ messages in thread From: Simon Marchi @ 2018-10-23 20:56 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 2018-10-23 7:11 a.m., Pedro Alves wrote: > Oh, in that case it'd be clearer to not bundle it in the same series then. Right, I happened to write it at the same time (when trying to figure out how things worked) but it's not actually related. >> Can't you access the inferior's target stack directly instead of changing the current inferior? > > I can for the call to the top target, but then the problem is the beneath() > calls in all the target-delegates.c delegating implementations. E.g. here: > > void > -target_ops::terminal_save_inferior () > +target_ops::terminal_save_inferior (inferior *arg0) > { > - this->beneath ()->terminal_save_inferior (); > + this->beneath ()->terminal_save_inferior (arg0); > ^^^^^^^^^ > } > > because beneath() looks at the target stack of the current > inferior. It would need to look for the target beneath in > the target stack of the arg0 inferior instead. Otherwise > you start the top target call in inferior B, and then > cross the the beneath target of inferior A (the current inferior). > Whoops. At some point in the branch I made target_ops::beneath > take an optional inferior pointer, but when I stumbled on this > issue in target-delegates.c I ended up reverting it, as it > wasn't easy to fix. I think that we could maybe teach > make-target-delegates to automatically emit > > void > target_ops::method (inferior *arg0) > { > this->beneath (arg0)->method (arg0); > } > > and: > > void > target_ops::method (thread_info *arg0) > { > this->beneath (arg0->inf)->method (arg0); > } > > iff the method's first parameter is an inferior or thread_info > pointer. But that was just an idea, I never toyed with it, > because it would be a detour. So I gave up on the inferior > parameter to target beneath, and thought I'd better focus instead > of getting the multi-target basics in first, even if that means we > need to swap current inferior/thread here and there, as usual. Ok I understand. At some point I think it would be very nice if it "just worked", but I totally understand the need to go by small increment. In that case, I think I'll drop patches 1 and 2, since they don't really provide any benefit at the moment, and just cause complications going further. Simon ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-10-24 14:08 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-16 3:38 [PATCH 1/3] Pass inferior to terminal_save_inferior Simon Marchi 2018-10-16 3:38 ` [PATCH 2/3] Pass inferior to terminal_inferior Simon Marchi 2018-10-22 20:54 ` Pedro Alves 2018-10-23 9:14 ` Simon Marchi 2018-10-16 3:38 ` [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368) Simon Marchi 2018-10-17 17:05 ` Pedro Alves 2018-10-20 15:14 ` Simon Marchi 2018-10-20 15:38 ` Simon Marchi 2018-10-22 20:56 ` Pedro Alves 2018-10-23 9:49 ` Simon Marchi 2018-10-23 10:55 ` Pedro Alves 2018-10-24 9:41 ` Andreas Schwab 2018-10-24 14:08 ` Simon Marchi 2018-10-22 20:54 ` [PATCH 1/3] Pass inferior to terminal_save_inferior Pedro Alves 2018-10-23 9:01 ` Simon Marchi 2018-10-23 11:11 ` Pedro Alves 2018-10-23 20:56 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).