From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27682 invoked by alias); 16 May 2007 23:35:33 -0000 Received: (qmail 27672 invoked by uid 22791); 16 May 2007 23:35:30 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 16 May 2007 23:35:28 +0000 Received: (qmail 31304 invoked from network); 16 May 2007 23:35:26 -0000 Received: from unknown (HELO localhost) (jimb@127.0.0.2) by mail.codesourcery.com with ESMTPA; 16 May 2007 23:35:26 -0000 To: Caroline Tice Cc: gdb-patches@sourceware.org Subject: Re: Ping! [PATCH]: Tracking and reporting uninitialized variables References: <80EE31A6-4DF2-4D1F-B23D-8B814C1E6928@apple.com> <717BDA2E-D6FF-4AA5-B857-68BA5B140F5F@apple.com> From: Jim Blandy Date: Wed, 16 May 2007 23:35:00 -0000 In-Reply-To: (Caroline Tice's message of "Wed, 9 May 2007 14:04:07 -0700") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2007-05/txt/msg00287.txt.bz2 Caroline Tice writes: > I tested it by running it on a small test case I have > (with DW_OP_GNU_uninit ops in it), as well as running the > dejagnu testsuite with no regressions. Is this modified patch okay > to commit to FSF GDB? Thanks for the revisions! They look good. I have a few more comments, mostly just details. > Index: gdb/c-valprint.c > =================================================================== > RCS file: /cvs/src/src/gdb/c-valprint.c,v > retrieving revision 1.42 > diff -c -3 -p -r1.42 c-valprint.c > *** gdb/c-valprint.c 26 Jan 2007 20:54:16 -0000 1.42 > --- gdb/c-valprint.c 9 May 2007 20:53:21 -0000 > *************** c_value_print (struct value *val, struct > *** 556,561 **** > --- 556,564 ---- > } > } > > + if (value_initialized (val) == 0) > + fprintf_filtered (stream, " [uninitialized] "); > + > if (objectprint && (TYPE_CODE (type) == TYPE_CODE_CLASS)) > { > /* Attempt to determine real type of object */ Nit-picky suggestion: wouldn't it be nicer to say "if (! value_initialized (val)) ..."? > *************** execute_stack_op (struct dwarf_expr_cont > *** 731,736 **** > --- 734,748 ---- > } > goto no_push; > > + case DW_OP_GNU_uninit: > + if (op_ptr != op_end > + && *op_ptr != DW_OP_piece) > + error (_("DWARF-2 expression error: DW_OP_reg operations must be " > + "used either alone or in conjuction with DW_OP_piece.")); > + > + ctx->initialized = 0; > + goto no_push; > + > default: > error (_("Unhandled dwarf expression opcode 0x%x"), op); > } You said DW_OP_GNU_uninit doesn't apply to individual pieces. So shouldn't this code report an error whenever DW_OP_GNU_uninit isn't the last opcode in the whole expression --- that is, even when it's followed by DW_OP_piece? > Index: gdb/dwarf2expr.h > =================================================================== > RCS file: /cvs/src/src/gdb/dwarf2expr.h,v > retrieving revision 1.9 > diff -c -3 -p -r1.9 dwarf2expr.h > *** gdb/dwarf2expr.h 9 Jan 2007 17:58:50 -0000 1.9 > --- gdb/dwarf2expr.h 9 May 2007 20:53:21 -0000 > *************** struct dwarf_expr_context > *** 76,81 **** > --- 76,84 ---- > will be on the expression stack. */ > int in_reg; > > + /* Initialization status of variable. */ > + int initialized; > + Another nit-pick: the comment should actually explain the interpretation of the value: "Non-zero if variable has been initialized; zero otherwise." > Index: gdb/value.h > =================================================================== > RCS file: /cvs/src/src/gdb/value.h,v > retrieving revision 1.96 > diff -c -3 -p -r1.96 value.h > *** gdb/value.h 9 Jan 2007 17:58:59 -0000 1.96 > --- gdb/value.h 9 May 2007 20:53:21 -0000 > *************** extern int value_contents_equal (struct > *** 193,198 **** > --- 193,204 ---- > extern int value_optimized_out (struct value *value); > extern void set_value_optimized_out (struct value *value, int val); > > + /* Set or return field indicating whether a variable is initialized or > + not, based on DWARF location information supplied by the compiler. > + 1 = initialized; 0 = uninitialized. */ > + extern int value_initialized (struct value *); > + extern void set_value_initialized (struct value *, int); > + > /* While the following fields are per- VALUE .CONTENT .PIECE (i.e., a > single value might have multiple LVALs), this hacked interface is > limited to just the first PIECE. Expect further change. */ A final nit-pick: value.h, value.c, and so on are meant to be independent of any particular debugging format. There should never be DWARF dependencies here. Your code does this right --- a generic mechanism to be used by DWARF-specific code in the appropriate way --- but the comment should also not suggest that it's DWARF-specific. Thus, "... based on debugging information supplied by the compiler".