This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Ping! [PATCH]: Tracking and reporting uninitialized variables
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".