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

[Caroline Tice <ctice at apple dot com>]

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

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

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

On May 9, 2007, at 10:36 AM, Caroline Tice wrote:

> On May 8, 2007, at 5:33 PM, Jim Blandy wrote:
>
> > Caroline Tice <ctice@apple.com> 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.