From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22946 invoked by alias); 9 May 2007 21:04:17 -0000 Received: (qmail 22932 invoked by uid 22791); 9 May 2007 21:04:15 -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; Wed, 09 May 2007 21:04:11 +0000 Received: from relay8.apple.com (relay8.apple.com [17.128.113.38]) by mail-out4.apple.com (Postfix) with ESMTP id 19212D3B55 for ; Wed, 9 May 2007 14:04:10 -0700 (PDT) Received: from relay8.apple.com (unknown [127.0.0.1]) by relay8.apple.com (Symantec Mail Security) with ESMTP id 0716B405A5; Wed, 9 May 2007 14:04:10 -0700 (PDT) X-AuditID: 11807126-a342fbb000004313-e8-4642374850d5 Received: from [17.219.213.36] (unknown [17.219.213.36]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by relay8.apple.com (Apple SCV relay) with ESMTP id 7C2ED404F5; Wed, 9 May 2007 14:04:08 -0700 (PDT) In-Reply-To: References: <80EE31A6-4DF2-4D1F-B23D-8B814C1E6928@apple.com> <717BDA2E-D6FF-4AA5-B857-68BA5B140F5F@apple.com> Mime-Version: 1.0 (Apple Message framework v749.3) Content-Type: multipart/mixed; boundary=Apple-Mail-2-721468816 Message-Id: Cc: Caroline Tice From: Caroline Tice Subject: Re: Ping! [PATCH]: Tracking and reporting uninitialized variables Date: Wed, 09 May 2007 21:04:00 -0000 To: gdb-patches@sourceware.org X-Mailer: Apple Mail (2.749.3) 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/msg00146.txt.bz2 --Apple-Mail-2-721468816 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=US-ASCII; delsp=yes; format=flowed Content-length: 1888 Okay, here is the modified patch. I went back and double checked the variable tracking stuff in GCC and discovered that there is NOT a way to mark individual pieces in a multi-piece location expression as initialized or not; it's one initialized value for the whole thing. Therefore I did not add the initialized field to the dwarf_expr_piece as suggested below. But I did address everything else. 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? -- Caroline Tice ctice@apple.com 2007-05-09 Caroline Tice * c-valprint.c (c_value_print): If the initialized field of the value struct is 0, print out "[uninitialized]" before the value. * dwarf2expr.c (execute_stack_op): Initialize ctx- >initialized field; allow DW_OP_GNU_uninit as legal op following a DW_OP_reg op or a DW_OP_regx op; add case for DW_OP_GNU_uninit and update ctx->initialized appropriately. Verify no location op follows DW_OP_GNU_uninit. * dwarf2expr.h (struct dwarf_expr_context): New field, initialized. * dwarf2loc.c (dwarf2_evaluate_loc_desc): Add call to set_value_initialized. * dwarf2read.c (dwarf_stack_op_name): Add case for DW_OP_GNU_uninit. (decode_locdesc): Add case for DW_OP_GNU_uninit. * value.c (struct value): New field, initialized. (allocate_value): Initialize new field. (set_value_initialized): New function. (value_initialized): New function. * value.h (value_initialized): New extern declaration. (set_value_initialized): Likewise. * include/elf/dwarf2.h: (enum dwarf_location_atom): Add new DW_OP, DW_OP_GNU_uninit. --Apple-Mail-2-721468816 Content-Transfer-Encoding: 7bit Content-Type: text/plain; x-unix-mode=0644; name="fsf-gdb-patch2.txt" Content-Disposition: attachment; filename=fsf-gdb-patch2.txt Content-length: 7542 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 */ 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 9 May 2007 20:53:21 -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,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); } 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; + /* 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 9 May 2007 20:53:21 -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 9 May 2007 20:53:21 -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 9 May 2007 20:53:21 -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 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. */ 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 9 May 2007 20:53:22 -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-2-721468816 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=US-ASCII; delsp=yes; format=flowed Content-length: 2544 On May 9, 2007, at 10:36 AM, Caroline Tice wrote: > > On May 8, 2007, at 5:33 PM, Jim Blandy wrote: > >> >> Caroline Tice writes: >>>> As part of some work I have been doing on improving debugging of >>>> optimized code, I >>>> have created a GCC patch that tags variables it believes are >>>> uninitialized with a new >>>> Dwarf op (an extension), DW_OP_GNU_uninit. I have submitted that >>>> patch to the >>>> GCC patches list and am waiting for approval to commit it. I have >>>> also created the >>>> following gdb patch to recognize the new op and inform the user >>>> when >>>> a variable >>>> the user requests to see is uninitialized. >>>> >>>> I have tested this patch on some small testcases and I have run the >>>> gdb dejagnu >>>> testsuite with no regressions. I am new to submitting things to >>>> this list, so if there >>>> is anything else I ought to have done, please let me know >>>> (kindly!). >> >> Hi, Caroline. This seems like a nice patch. >> >> In a multi-piece location expression, can each piece be individually >> initialized or uninitialized? If that's so, then there should >> also be >> an 'initialized' member of 'struct dwarf_expr_piece', which gets set >> appropriately for each piece in a multi-piece location expression. >> > > Okay, will do. (Yes, I believe each piece can be individually > initialized or uninitialized.) > >> Either way, the code for DW_OP_GNU_uninit should check that it's the >> last opcode in the piece or in the entire expression, as the >> DW_OP_reg* cases do. >> > > Will do. > >> I think the 'struct dwarf_expr_context' member should be named >> simply 'initialized', instead of 'var_status'. The 'struct value' >> field should be named 'initialized', and the accessor functions >> should >> be named 'value_initialized' and 'set_value_initialized'. The >> comment >> in value.h actually needs to be filled in; the description should be >> thorough enough to allow someone who otherwise knows how GDB works to >> use those functions, without reading their definitions. >> > > Will do. > >> I couldn't see from your patch why 'signed_address_type', >> 'unsigned_address_type', and 'add_piece' were made visible outside >> dwarf2expr.c; that should be left out of the patch if it's not >> needed. >> > > I went back and checked; making them globally visible was actually > for a different patch I did. Sorry; I will remove that from this > patch. > >> Have you filed a copyright assignment with the FSF? > > Jim Ingham answered this one. > --Apple-Mail-2-721468816--