From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18324 invoked by alias); 27 Aug 2012 17:06:13 -0000 Received: (qmail 18302 invoked by uid 22791); 27 Aug 2012 17:06:10 -0000 X-SWARE-Spam-Status: No, hits=-5.6 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-vc0-f169.google.com (HELO mail-vc0-f169.google.com) (209.85.220.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 27 Aug 2012 17:05:41 +0000 Received: by vcbfl10 with SMTP id fl10so5533083vcb.0 for ; Mon, 27 Aug 2012 10:05:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-system-of-record:x-gm-message-state; bh=2liGiatMtkJaCztWVb1Nf+Nttx25KXd5ss+NoEWX0FA=; b=aA4M11K6kWlAcB9eFdNoKMtBuCBqsMFx8l3+Nmwn+eK6UhOuO90iI8dZCgb2QvbzUy s6N7oMYHJxAx1bJ6euL/0PJMGs2Vr0NH7YJSM6jJvVg5yulPjxu973zc9bBQaKEZUnjs EK/1EhSYyA9OlTaCbv7njI204HQd06MdPP2wC18Ba/iQOKy5OhkepIIimEYGyl61H6kS xRDPr5T/H3KhowyZw1MrJGWOJCFvfb3Rw4OlbIjtsJ6TpqH0cfDei9t7UOi182Y7FOn3 duptWBa9gvUWk8gw89BYBRPHEmCKV80jGvrvPsNXseyxVrgt04s74BWEO6sBy0AWAm9H BrIQ== Received: by 10.52.34.212 with SMTP id b20mr10177488vdj.115.1346087140274; Mon, 27 Aug 2012 10:05:40 -0700 (PDT) MIME-Version: 1.0 Received: by 10.52.34.212 with SMTP id b20mr10177480vdj.115.1346087140148; Mon, 27 Aug 2012 10:05:40 -0700 (PDT) Received: by 10.52.24.239 with HTTP; Mon, 27 Aug 2012 10:05:40 -0700 (PDT) In-Reply-To: References: Date: Mon, 27 Aug 2012 17:06:00 -0000 Message-ID: Subject: Re: [PATCH] gdb: trivial segfault fix in tui From: Doug Evans To: Hal Ashburner Cc: "Abid, Hafiz" , "gdb-patches@sourceware.org" Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true X-Gm-Message-State: ALoCoQk1UuAzkVrcfP5Jgl52o1gGBLWICTgf7ILjbTKHCceYk+rZtoZLn6v3X+DBmZPFSnWR0FRiRIqW9w95DUWjuQM6wzTig3bpL+VGnMc5Fgn56PhZ9AulkbqoqBKSzVFTxZWPqxuuttVQSE9Sc5p7vJ+DO2llgyqmntvuVeRMETn/Y4dKR+Oa4QqFL3ENUK1b9MdUlH2MKpuqG865PxKsME0DJaeB3g== X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-08/txt/msg00811.txt.bz2 On Wed, Aug 15, 2012 at 6:50 PM, Hal Ashburner wrote: > On 15 August 2012 10:34, Doug Evans 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 > + > + * tui/tui-source.c: Check for null pointer to prevent segfault. > + > 2012-08-10 Sergio Durigan Junior > * 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.