* [PATCH] gdb: trivial segfault fix in tui @ 2012-08-12 16:13 Hal Ashburner 2012-08-14 10:55 ` Abid, Hafiz 0 siblings, 1 reply; 6+ messages in thread From: Hal Ashburner @ 2012-08-12 16:13 UTC (permalink / raw) To: gdb-patches [-- Attachment #1: Type: text/plain, Size: 398 bytes --] Hello! I hope this is the correct etiquette and that it's useful. Copyright assigned to whoever commits to assign to GNU or do whatever is appropriate (assuming it is committed). To the easiest way to replicate the segfault: gdb some_prog b main run C-x a C-x 2 C-x 2 C-x 2 -- so you now have registers on the top panel and source in the middle. resize the terminal. step, segfault Regards, Hal [-- Attachment #2: gdbtui_segfault.patch --] [-- Type: application/octet-stream, Size: 825 bytes --] diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c index 9ba9b1d..0c94aed 100644 --- a/gdb/tui/tui-source.c +++ b/gdb/tui/tui-source.c @@ -334,11 +334,13 @@ tui_show_symtab_source (struct gdbarch *gdbarch, struct symtab *s, int tui_source_is_displayed (char *fname) { - return (TUI_SRC_WIN->generic.content_in_use - && (filename_cmp (((struct tui_win_element *) - (tui_locator_win_info_ptr ())-> - content[0])->which_element.locator.file_name, - fname) == 0)); + if (tui_locator_win_info_ptr()->content) + return (TUI_SRC_WIN->generic.content_in_use + && (filename_cmp (((struct tui_win_element *) + (tui_locator_win_info_ptr ())-> + content[0])->which_element.locator.file_name, + fname) == 0)); + return 0; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] gdb: trivial segfault fix in tui 2012-08-12 16:13 [PATCH] gdb: trivial segfault fix in tui Hal Ashburner @ 2012-08-14 10:55 ` Abid, Hafiz 2012-08-14 23:03 ` Hal Ashburner 0 siblings, 1 reply; 6+ messages in thread From: Abid, Hafiz @ 2012-08-14 10:55 UTC (permalink / raw) To: Hal Ashburner, gdb-patches This will require a ChangeLog entry. Regards, Abid -- Hafiz Abid Qadeer Mentor Graphics hafiz_abid@mentor.com > -----Original Message----- > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] On Behalf Of Hal Ashburner > Sent: Sunday, August 12, 2012 5:13 PM > To: gdb-patches@sourceware.org > Subject: [PATCH] gdb: trivial segfault fix in tui > > Hello! > > I hope this is the correct etiquette and that it's useful. Copyright > assigned to whoever commits to assign to GNU or do whatever is > appropriate (assuming it is committed). > > To the easiest way to replicate the segfault: > gdb some_prog > b main > run > C-x a > C-x 2 > C-x 2 > C-x 2 -- so you now have registers on the top panel and source in the > middle. > resize the terminal. > step, > segfault > > Regards, > Hal ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: trivial segfault fix in tui 2012-08-14 10:55 ` Abid, Hafiz @ 2012-08-14 23:03 ` Hal Ashburner 2012-08-15 0:34 ` Doug Evans 0 siblings, 1 reply; 6+ messages in thread From: Hal Ashburner @ 2012-08-14 23:03 UTC (permalink / raw) To: Abid, Hafiz; +Cc: gdb-patches Dear Hafiz, Thank you for letting me know this. I've added a ChangeLog entry to the patch. I hope this is done the correct way. By way of trivia, I fixed this bug because I hit it more than once. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 81c03ee..e1a080f 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,7 @@ +2012-08-15 Hal Ashburner <hal@ashburner.info> + + * tui/tui-source.c: Check for null pointer to prevent segfault. + 2012-08-10 Sergio Durigan Junior <sergiodj@redhat.com> * linespec.c (find_methods): Remove unused variables `i1' and diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c index 9ba9b1d..0c94aed 100644 --- a/gdb/tui/tui-source.c +++ b/gdb/tui/tui-source.c @@ -334,11 +334,13 @@ tui_show_symtab_source (struct gdbarch *gdbarch, struct symtab *s, int tui_source_is_displayed (char *fname) { - return (TUI_SRC_WIN->generic.content_in_use - && (filename_cmp (((struct tui_win_element *) - (tui_locator_win_info_ptr ())-> - content[0])->which_element.locator.file_name, - fname) == 0)); + if (tui_locator_win_info_ptr()->content) + return (TUI_SRC_WIN->generic.content_in_use + && (filename_cmp (((struct tui_win_element *) + (tui_locator_win_info_ptr ())-> + content[0])->which_element.locator.file_name, + fname) == 0)); + return 0; } On 14 August 2012 20:55, Abid, Hafiz <Hafiz_Abid@mentor.com> wrote: > This will require a ChangeLog entry. > > Regards, > Abid > -- > Hafiz Abid Qadeer > Mentor Graphics > hafiz_abid@mentor.com > >> -----Original Message----- >> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- >> owner@sourceware.org] On Behalf Of Hal Ashburner >> Sent: Sunday, August 12, 2012 5:13 PM >> To: gdb-patches@sourceware.org >> Subject: [PATCH] gdb: trivial segfault fix in tui >> >> Hello! >> >> I hope this is the correct etiquette and that it's useful. Copyright >> assigned to whoever commits to assign to GNU or do whatever is >> appropriate (assuming it is committed). >> >> To the easiest way to replicate the segfault: >> gdb some_prog >> b main >> run >> C-x a >> C-x 2 >> C-x 2 >> C-x 2 -- so you now have registers on the top panel and source in the >> middle. >> resize the terminal. >> step, >> segfault >> >> Regards, >> Hal ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: trivial segfault fix in tui 2012-08-14 23:03 ` Hal Ashburner @ 2012-08-15 0:34 ` Doug Evans 2012-08-16 1:50 ` Hal Ashburner 0 siblings, 1 reply; 6+ messages in thread From: Doug Evans @ 2012-08-15 0:34 UTC (permalink / raw) To: Hal Ashburner; +Cc: Abid, Hafiz, gdb-patches On Tue, Aug 14, 2012 at 4:03 PM, Hal Ashburner <hal@ashburner.info> wrote: > Dear Hafiz, > > Thank you for letting me know this. > I've added a ChangeLog entry to the patch. I hope this is done the correct way. > > By way of trivia, I fixed this bug because I hit it more than once. > > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 81c03ee..e1a080f 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,7 @@ > +2012-08-15 Hal Ashburner <hal@ashburner.info> > + > + * tui/tui-source.c: Check for null pointer to prevent segfault. > + > 2012-08-10 Sergio Durigan Junior <sergiodj@redhat.com> > > * linespec.c (find_methods): Remove unused variables `i1' and > diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c > index 9ba9b1d..0c94aed 100644 > --- a/gdb/tui/tui-source.c > +++ b/gdb/tui/tui-source.c > @@ -334,11 +334,13 @@ tui_show_symtab_source (struct gdbarch *gdbarch, > struct symtab *s, > int > tui_source_is_displayed (char *fname) > { > - return (TUI_SRC_WIN->generic.content_in_use > - && (filename_cmp (((struct tui_win_element *) > - (tui_locator_win_info_ptr ())-> > - content[0])->which_element.locator.file_name, > - fname) == 0)); > + if (tui_locator_win_info_ptr()->content) > + return (TUI_SRC_WIN->generic.content_in_use > + && (filename_cmp (((struct tui_win_element *) > + (tui_locator_win_info_ptr ())-> > + content[0])->which_element.locator.file_name, > + fname) == 0)); > + return 0; > } It seems tui is quite fragile with respect to window resizes. I was able to get a segv here for similar reasons, but I didn't write down the steps I did to trigger it. :-( tui-source.c: element->which_element.source.is_exec_point = (filename_cmp (((struct tui_win_element *) locator->content[0])->which_element.locator.file_name, s->filename) == 0 tui is also a bit, umm, weird. tui_free_window has this: generic_win = tui_locator_win_info_ptr (); if (generic_win != (struct tui_gen_win_info *) NULL) { tui_delete_win (generic_win->handle); generic_win->handle = (WINDOW *) NULL; } but tui_locator_win_info_ptr will never return NULL. struct tui_gen_win_info * tui_locator_win_info_ptr (void) { return &_locator; } Blech! In the end I suspect there's a better fix, but I don't know tui well enough. I'd wait to see if someone else does. Also, I'd add a comment explaining *why* the test for content != NULL, and I'd rewrite it as: return (TUI_SRC_WIN->generic.content_in_use && tui_locator_win_info_ptr()->content != NULL && ...); The style is more consistent this way. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: trivial segfault fix in tui 2012-08-15 0:34 ` Doug Evans @ 2012-08-16 1:50 ` Hal Ashburner 2012-08-27 17:06 ` Doug Evans 0 siblings, 1 reply; 6+ messages in thread From: Hal Ashburner @ 2012-08-16 1:50 UTC (permalink / raw) To: Doug Evans; +Cc: Abid, Hafiz, gdb-patches On 15 August 2012 10:34, Doug Evans <dje@google.com> wrote: > > It seems tui is quite fragile with respect to window resizes. > I was able to get a segv here for similar reasons, but I didn't write > down the steps I did to trigger it. :-( > > tui-source.c: > element->which_element.source.is_exec_point = > (filename_cmp (((struct tui_win_element *) > > locator->content[0])->which_element.locator.file_name, > s->filename) == 0 > > tui is also a bit, umm, weird. > > tui_free_window has this: > > generic_win = tui_locator_win_info_ptr (); > if (generic_win != (struct tui_gen_win_info *) NULL) > { > tui_delete_win (generic_win->handle); > generic_win->handle = (WINDOW *) NULL; > } > > but tui_locator_win_info_ptr will never return NULL. > > struct tui_gen_win_info * > tui_locator_win_info_ptr (void) > { > return &_locator; > } > > Blech! > > In the end I suspect there's a better fix, but I don't know tui well enough. > I'd wait to see if someone else does. > > Also, I'd add a comment explaining *why* the test for content != NULL, > and I'd rewrite it as: > > return (TUI_SRC_WIN->generic.content_in_use > && tui_locator_win_info_ptr()->content != NULL > && ...); > > The style is more consistent this way. Hi Doug, Thanks for the review! :-) Yeah it is "more consistent" the way you suggest, but is this what is wanted in this particular case? Or do we want it to stand out a little and attract our reader's attention, while being minimally intrusive to the existing code? There's a segfault, they're bad, they're annoying to have in the middle of a debugging session because you lose all your debugging work setting up context, yada yada I don't have to jibber about all that here I'm sure. The check for null fixes the segfaults that have bitten me. I sometimes need to do a !reset to make C-X work again in my terminal which triggers it less reliably, but not any more with the fix in. TUI has some pretty hairy code as you rightly point out. It appears to be not maintained quite as well as it might be. It does have a particular use case I love which is less popular, single stepping assembly language code so I would personally advocate keeping tui in the gdb codebase. My patch is a temporary fix until someone has time to look at the entirety of how tui works and where it doesn't. Possibly re-factoring and re-writing large parts of it. Now that might end up being me in the absence of someone more familiar with the project already doing it. tui_win_info_ptr()->content is a pointer to a pointer. Which is being used to store the address of an array of pointers to tui_gen_win_info. It gets set to null on one of the window init routines. tui_init_generic_part() tui_resize_all() in tui_win.c calls tui_free_win() on all windows also sets it to null so perhaps it's not being reallocated as implicitly expected. I'd be poking around there if it's me who gets time to delve deeper into it and the proper formulation of the fix. Note also that checking this thing for NULL is consistent with the following line in the same file, so mine isn't the first time: void tui_vertical_source_scroll (enum tui_scroll_direction scroll_direction, int num_to_scroll) { if (TUI_SRC_WIN->generic.content != NULL) if (foo == NULL) is not a style I care for, it's tautological by way of saying it twice. If this is how it's done in the gdb codebase then obviously it's the right way. So how about we express the patch as follows and get it in as it is clearly and obviously better than what is there now. Then it can be removed when something better and more stable comes along. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 81c03ee..e1a080f 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,7 @@ +2012-08-15 Hal Ashburner <hal@ashburner.info> + + * tui/tui-source.c: Check for null pointer to prevent segfault. + 2012-08-10 Sergio Durigan Junior <sergiodj@redhat.com> * linespec.c (find_methods): Remove unused variables `i1' and diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c index 9ba9b1d..2126b36 100644 --- a/gdb/tui/tui-source.c +++ b/gdb/tui/tui-source.c @@ -334,11 +334,17 @@ tui_show_symtab_source (struct gdbarch *gdbarch, struct symtab *s, int tui_source_is_displayed (char *fname) { - return (TUI_SRC_WIN->generic.content_in_use - && (filename_cmp (((struct tui_win_element *) - (tui_locator_win_info_ptr ())-> - content[0])->which_element.locator.file_name, - fname) == 0)); + if (tui_locator_win_info_ptr()->content == NULL) { + /* XXX FIXME corrects segfault, but why is content NULL + * after certian terminal resize operations? */ + return 0; + } else { + return (TUI_SRC_WIN->generic.content_in_use + && (filename_cmp (((struct tui_win_element *) + (tui_locator_win_info_ptr ())-> + content[0])->which_element.locator.file_name, + fname) == 0)); + } } Thanks again, Hal Ashburner ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: trivial segfault fix in tui 2012-08-16 1:50 ` Hal Ashburner @ 2012-08-27 17:06 ` Doug Evans 0 siblings, 0 replies; 6+ messages in thread From: Doug Evans @ 2012-08-27 17:06 UTC (permalink / raw) To: Hal Ashburner; +Cc: Abid, Hafiz, gdb-patches On Wed, Aug 15, 2012 at 6:50 PM, Hal Ashburner <hal@ashburner.info> wrote: > On 15 August 2012 10:34, Doug Evans <dje@google.com> wrote: > > [...] > Hi Doug, > > Thanks for the review! :-) > > Yeah it is "more consistent" the way you suggest, but is this what is > wanted in this particular case? Or do we want it to stand out a little > and attract our reader's attention, while being minimally intrusive to > the existing code? > > There's a segfault, they're bad, they're annoying to have in the > middle of a debugging session because you lose all your debugging work > setting up context, yada yada I don't have to jibber about all that > here I'm sure. > > The check for null fixes the segfaults that have bitten me. I > sometimes need to do a !reset to make C-X work again in my terminal > which triggers it less reliably, but not any more with the fix in. > > TUI has some pretty hairy code as you rightly point out. > It appears to be not maintained quite as well as it might be. It does > have a particular use case I love which is less popular, single > stepping assembly language code so I would personally advocate keeping > tui in the gdb codebase. > > My patch is a temporary fix until someone has time to look at the > entirety of how tui works and where it doesn't. Possibly re-factoring > and re-writing large parts of it. Now that might end up being me in > the absence of someone more familiar with the project already doing > it. > > tui_win_info_ptr()->content is a pointer to a pointer. Which is being > used to store the address of an array of pointers to tui_gen_win_info. > It gets set to null on one of the window init routines. > tui_init_generic_part() > tui_resize_all() in tui_win.c calls tui_free_win() on all windows also > sets it to null so perhaps it's not being reallocated as implicitly > expected. > > I'd be poking around there if it's me who gets time to delve deeper > into it and the proper formulation of the fix. > > Note also that checking this thing for NULL is consistent with the > following line in the same file, so mine isn't the first time: > void > tui_vertical_source_scroll (enum tui_scroll_direction scroll_direction, > int num_to_scroll) > { > if (TUI_SRC_WIN->generic.content != NULL) > > > if (foo == NULL) is not a style I care for, it's tautological by way > of saying it twice. If this is how it's done in the gdb codebase then > obviously it's the right way. > > So how about we express the patch as follows and get it in as it is > clearly and obviously better than what is there now. Then it can be > removed when something better and more stable comes along. > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 81c03ee..e1a080f 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,7 @@ > +2012-08-15 Hal Ashburner <hal@ashburner.info> > + > + * tui/tui-source.c: Check for null pointer to prevent segfault. > + > 2012-08-10 Sergio Durigan Junior <sergiodj@redhat.com> > * linespec.c (find_methods): Remove unused variables `i1' and > diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c > index 9ba9b1d..2126b36 100644 > --- a/gdb/tui/tui-source.c > +++ b/gdb/tui/tui-source.c > @@ -334,11 +334,17 @@ tui_show_symtab_source (struct gdbarch *gdbarch, > struct symtab *s, > int > tui_source_is_displayed (char *fname) > { > - return (TUI_SRC_WIN->generic.content_in_use > - && (filename_cmp (((struct tui_win_element *) > - (tui_locator_win_info_ptr ())-> > - content[0])->which_element.locator.file_name, > - fname) == 0)); > + if (tui_locator_win_info_ptr()->content == NULL) { > + /* XXX FIXME corrects segfault, but why is content NULL > + * after certian terminal resize operations? */ > + return 0; > + } else { > + return (TUI_SRC_WIN->generic.content_in_use > + && (filename_cmp (((struct tui_win_element *) > + (tui_locator_win_info_ptr ())-> > + content[0])->which_element.locator.file_name, > + fname) == 0)); > + } > } > > > Thanks again, > Hal Ashburner I like this alternative patch better: http://sourceware.org/ml/gdb-patches/2012-08/msg00809.html So let's go with that. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-27 17:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-08-12 16:13 [PATCH] gdb: trivial segfault fix in tui Hal Ashburner 2012-08-14 10:55 ` Abid, Hafiz 2012-08-14 23:03 ` Hal Ashburner 2012-08-15 0:34 ` Doug Evans 2012-08-16 1:50 ` Hal Ashburner 2012-08-27 17:06 ` Doug Evans
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox