From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28437 invoked by alias); 17 May 2007 17:18:48 -0000 Received: (qmail 28381 invoked by uid 22791); 17 May 2007 17:18:43 -0000 X-Spam-Check-By: sourceware.org Received: from mail-out4.apple.com (HELO mail-out4.apple.com) (17.254.13.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 17 May 2007 17:18:35 +0000 Received: from relay8.apple.com (relay8.apple.com [17.128.113.38]) by mail-out4.apple.com (Postfix) with ESMTP id 6E04A175421; Thu, 17 May 2007 10:18:31 -0700 (PDT) Received: from relay8.apple.com (unknown [127.0.0.1]) by relay8.apple.com (Symantec Mail Security) with ESMTP id 57A7840540; Thu, 17 May 2007 10:18:31 -0700 (PDT) X-AuditID: 11807126-affe7bb000003f8b-ff-464c8e6749ba Received: from [17.201.21.120] (athena.apple.com [17.201.21.120]) by relay8.apple.com (Apple SCV relay) with ESMTP id 2D2DC4007B; Thu, 17 May 2007 10:18:31 -0700 (PDT) Cc: gdb-patches@sourceware.org, Caroline Tice Message-Id: From: Caroline Tice To: Jim Blandy In-Reply-To: Content-Type: multipart/mixed; boundary=Apple-Mail-6--748352083 Mime-Version: 1.0 (Apple Message framework v880) Subject: Re: Ping! [PATCH]: Tracking and reporting uninitialized variables Date: Thu, 17 May 2007 17:18:00 -0000 References: <80EE31A6-4DF2-4D1F-B23D-8B814C1E6928@apple.com> <717BDA2E-D6FF-4AA5-B857-68BA5B140F5F@apple.com> X-Mailer: Apple Mail (2.880) 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/msg00296.txt.bz2 --Apple-Mail-6--748352083 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Content-length: 158 Okay, I agree with your suggestions and have made the changes. The modified patch is attached. Is this okay to commit? -- Caroline Tice ctice@apple.com --Apple-Mail-6--748352083 Content-Disposition: attachment; filename=fsf-gdb-patch3.txt Content-Type: text/plain; x-unix-mode=0644; name=fsf-gdb-patch3.txt Content-Transfer-Encoding: 7bit Content-length: 7539 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 17 May 2007 16:59:06 -0000 *************** c_value_print (struct value *val, struct *** 556,561 **** --- 556,564 ---- } } + if (!value_initialized (val)) + fprintf_filtered (stream, " [uninitialized] "); + if (objectprint && (TYPE_CODE (type) == TYPE_CODE_CLASS)) { /* Attempt to determine real type of object */ Index: gdb/dwarf2expr.c =================================================================== RCS file: /cvs/src/src/gdb/dwarf2expr.c,v retrieving revision 1.20 diff -c -3 -p -r1.20 dwarf2expr.c *** gdb/dwarf2expr.c 27 Apr 2007 22:34:56 -0000 1.20 --- gdb/dwarf2expr.c 17 May 2007 16:59:08 -0000 *************** execute_stack_op (struct dwarf_expr_cont *** 284,289 **** --- 284,290 ---- gdb_byte *op_ptr, gdb_byte *op_end) { ctx->in_reg = 0; + ctx->initialized = 1; /* Default is initialized. */ while (op_ptr < op_end) { *************** execute_stack_op (struct dwarf_expr_cont *** 410,416 **** case DW_OP_reg29: case DW_OP_reg30: case DW_OP_reg31: ! 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.")); --- 411,419 ---- case DW_OP_reg29: case DW_OP_reg30: case DW_OP_reg31: ! if (op_ptr != op_end ! && *op_ptr != DW_OP_piece ! && *op_ptr != DW_OP_GNU_uninit) error (_("DWARF-2 expression error: DW_OP_reg operations must be " "used either alone or in conjuction with DW_OP_piece.")); *************** execute_stack_op (struct dwarf_expr_cont *** 731,736 **** --- 734,747 ---- } goto no_push; + case DW_OP_GNU_uninit: + if (op_ptr != op_end) + error (_("DWARF-2 expression error: DW_OP_GNU_unint must always " + "be the very last op.")); + + ctx->initialized = 0; + goto no_push; + default: error (_("Unhandled dwarf expression opcode 0x%x"), op); } 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 17 May 2007 16:59:08 -0000 *************** struct dwarf_expr_context *** 76,81 **** --- 76,85 ---- will be on the expression stack. */ int in_reg; + /* Initialization status of variable: Non-zero if variable has been + initialized; zero otherwise. */ + int initialized; + /* An array of pieces. PIECES points to its first element; NUM_PIECES is its length. Index: gdb/dwarf2loc.c =================================================================== RCS file: /cvs/src/src/gdb/dwarf2loc.c,v retrieving revision 1.39 diff -c -3 -p -r1.39 dwarf2loc.c *** gdb/dwarf2loc.c 24 Jan 2007 22:04:48 -0000 1.39 --- gdb/dwarf2loc.c 17 May 2007 16:59:08 -0000 *************** dwarf2_evaluate_loc_desc (struct symbol *** 256,261 **** --- 256,263 ---- VALUE_ADDRESS (retval) = address; } + set_value_initialized (retval, ctx->initialized); + free_dwarf_expr_context (ctx); return retval; Index: gdb/dwarf2read.c =================================================================== RCS file: /cvs/src/src/gdb/dwarf2read.c,v retrieving revision 1.218 diff -c -3 -p -r1.218 dwarf2read.c *** gdb/dwarf2read.c 18 Apr 2007 13:25:04 -0000 1.218 --- gdb/dwarf2read.c 17 May 2007 16:59:08 -0000 *************** dwarf_stack_op_name (unsigned op) *** 8629,8634 **** --- 8629,8636 ---- return "DW_OP_bit_piece"; case DW_OP_GNU_push_tls_address: return "DW_OP_GNU_push_tls_address"; + case DW_OP_GNU_uninit: + return "DW_OP_GNU_uninit"; /* HP extensions. */ case DW_OP_HP_is_value: return "DW_OP_HP_is_value"; *************** decode_locdesc (struct dwarf_block *blk, *** 9204,9209 **** --- 9206,9214 ---- dwarf2_complex_location_expr_complaint (); break; + case DW_OP_GNU_uninit: + break; + default: complaint (&symfile_complaints, _("unsupported stack op: '%s'"), dwarf_stack_op_name (op)); Index: gdb/value.c =================================================================== RCS file: /cvs/src/src/gdb/value.c,v retrieving revision 1.41 diff -c -3 -p -r1.41 value.c *** gdb/value.c 13 Apr 2007 14:17:46 -0000 1.41 --- gdb/value.c 17 May 2007 16:59:08 -0000 *************** struct value *** 157,162 **** --- 157,165 ---- actually exist in the program. */ char optimized_out; + /* If value is a variable, is it initialized or not. */ + int initialized; + /* Actual contents of the value. For use of this value; setting it uses the stuff above. Not valid if lazy is nonzero. Target byte-order. We force it to be aligned properly for any possible *************** allocate_value (struct type *type) *** 232,237 **** --- 235,241 ---- val->embedded_offset = 0; val->pointed_to_offset = 0; val->modifiable = 1; + val->initialized = 1; /* Default to initialized. */ return val; } *************** using_struct_return (struct type *value_ *** 1691,1696 **** --- 1695,1716 ---- != RETURN_VALUE_REGISTER_CONVENTION); } + /* Set the initialized field in a value struct. */ + + void + set_value_initialized (struct value *val, int status) + { + val->initialized = status; + } + + /* Return the initialized field in a value struct. */ + + int + value_initialized (struct value *val) + { + return val->initialized; + } + void _initialize_values (void) { 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 17 May 2007 16:59:08 -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 debugging 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. */ Index: include/elf/dwarf2.h =================================================================== RCS file: /cvs/src/src/include/elf/dwarf2.h,v retrieving revision 1.19 diff -c -3 -p -r1.19 dwarf2.h *** include/elf/dwarf2.h 2 Mar 2006 00:54:27 -0000 1.19 --- include/elf/dwarf2.h 17 May 2007 16:59:09 -0000 *************** enum dwarf_location_atom *** 540,545 **** --- 540,546 ---- DW_OP_bit_piece = 0x9d, /* GNU extensions. */ DW_OP_GNU_push_tls_address = 0xe0, + DW_OP_GNU_uninit = 0xf0, /* HP extensions. */ DW_OP_HP_unknown = 0xe0, /* Ouch, the same as GNU_push_tls_address. */ DW_OP_HP_is_value = 0xe1, --Apple-Mail-6--748352083 Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes Content-Transfer-Encoding: 7bit Content-length: 4136 On May 16, 2007, at 4:35 PM, Jim Blandy wrote: > > 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". --Apple-Mail-6--748352083--