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

[Jim Blandy <jimb@codesourcery.com>]

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".