Re: Ping! [PATCH]: Tracking and reporting uninitialized variables

[Caroline Tice <ctice at apple dot com>]

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

Attachment: fsf-gdb-patch3.txt
Description: Text document

On May 16, 2007, at 4:35 PM, Jim Blandy wrote:

> Caroline Tice <ctice@apple.com> 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".