From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17531 invoked by alias); 19 Jul 2012 14:38:16 -0000 Received: (qmail 17521 invoked by uid 22791); 19 Jul 2012 14:38:15 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,SPF_FAIL X-Spam-Check-By: sourceware.org Received: from gbenson.demon.co.uk (HELO gbenson.demon.co.uk) (80.177.220.214) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 19 Jul 2012 14:38:01 +0000 Date: Thu, 19 Jul 2012 14:38:00 -0000 From: Gary Benson To: Sergio Durigan Junior Cc: gdb-patches@sourceware.org Subject: Re: [RFA 4/4] Improved linker-debugger interface Message-ID: <20120719143759.GC25093@redhat.com> Mail-Followup-To: Sergio Durigan Junior , gdb-patches@sourceware.org References: <20120712123406.GA29236@redhat.com> <20120712123617.GE29236@redhat.com> <20120713094150.GB2689@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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-07/txt/msg00339.txt.bz2 Sergio Durigan Junior wrote: > On Friday, July 13 2012, Gary Benson wrote: > > diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c > > index c111f04..ebb8c3c 100644 > > --- a/gdb/solib-svr4.c > > +++ b/gdb/solib-svr4.c [snip] > > + /* Check that an appropriate number of arguments has been supplied. > > + We expect: > > + arg1: Lmid_t lmid (mandatory) > > + arg2: struct r_debug *r_debug (mandatory) > > + arg3: struct link_map *new (optional, for incremental updates) */ > > I guess you could rename the arguments listed here to 'arg0', 'arg1' and > 'arg2', because `evaluate_probe_argument' takes these numbers as > arguments. Or you could explicitly say that here. Otherwise it will > confuse the reader, IMO. > > > + probe_argc = get_probe_argument_count (os->objfile, pi->probe); > > + if (probe_argc == 2) > > + action = LM_CACHE_RELOAD; > > + else if (probe_argc < 2) > > + return LM_CACHE_INVALIDATE; > > This is OK... > > > + /* We only currently support the global namespace (PR gdb/11839). > > + If the probe's r_debug doesn't match the global r_debug then > > + this event refers to some other namespace and must be ignored. */ > > + info = get_svr4_info (); > > + > > + /* Always locate the debug struct, in case it has moved. */ > > + info->debug_base = 0; > > + locate_base (info); > > + > > + debug_base = value_as_address (evaluate_probe_argument (os->objfile, > > + pi->probe, 1)); > > ...but what would happen if `evaluate_probe_argument' returned NULL? > It's better to check this, because `value_as_address' calls `value_type' > which does not check NULL pointers. > > Currently, only the SystemTap backend is implemented, and if it returns > NULL in this case it would be an error, but it's better to guard your > code IMO. Thank you for the review. I've updated the comment and added NULL checks (amongst other things) here: http://www.cygwin.com/ml/gdb-patches/2012-07/msg00333.html Cheers, Gary -- http://gbenson.net/