* [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