Patchwork [RFA] Clear entire "location" in value constructor

My recent change to allocate values with "new" may have introduced a
small bug.  In particular, the previous code allocated with XCNEW, but
the new code only clears a part of the "location" field in the
constructor.  I didn't try very hard to actually trigger a bug here,
the problem remains theoretical.

This patch changes the constructor to clear the entire "location".

Regression tested by the buildbot.

2018-04-10  Tom Tromey  <tom@tromey.com>

	* value.c (value::value): Clear "location".
---
 gdb/ChangeLog | 4 ++++
 gdb/value.c   | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> 2018-04-10  Tom Tromey  <tom@tromey.com>
Tom> 	* value.c (value::value): Clear "location".

Ping.

Tom

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> 2018-04-10  Tom Tromey  <tom@tromey.com>
Tom> * value.c (value::value): Clear "location".

Tom> Ping.

Ping.  Just FYI, this one is borderline obvious.

Tom

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> 2018-04-10  Tom Tromey  <tom@tromey.com>
Tom> * value.c (value::value): Clear "location".

Tom> Ping.

Tom> Ping.  Just FYI, this one is borderline obvious.

Ping again.
Maybe this should use {} instead of memset, what do you think?

Tom

On 2018-04-10 13:37, Tom Tromey wrote:
> My recent change to allocate values with "new" may have introduced a
> small bug.  In particular, the previous code allocated with XCNEW, but
> the new code only clears a part of the "location" field in the
> constructor.  I didn't try very hard to actually trigger a bug here,
> the problem remains theoretical.
> 
> This patch changes the constructor to clear the entire "location".
> 
> Regression tested by the buildbot.
> 
> 2018-04-10  Tom Tromey  <tom@tromey.com>
> 
> 	* value.c (value::value): Clear "location".
> ---
>  gdb/ChangeLog | 4 ++++
>  gdb/value.c   | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index d46ecdd120..8cc1486c87 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +2018-04-10  Tom Tromey  <tom@tromey.com>
> +
> +	* value.c (value::value): Clear "location".
> +
>  2018-04-10  Pedro Alves  <palves@redhat.com>
> 
>  	* gdbthread.h (finish_thread_state_cleanup): Delete declaration.
> diff --git a/gdb/value.c b/gdb/value.c
> index 12aa2b8bb4..64e3eaca22 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -180,7 +180,7 @@ struct value
>        type (type_),
>        enclosing_type (type_)
>    {
> -    location.address = 0;
> +    memset (&location, 0, sizeof (location));
>    }
> 
>    ~value ()

Ah, I stumbled upon that code recently and wondered why only 
location.address was set.  Either memset or {} is fine with me.

Simon

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Ah, I stumbled upon that code recently and wondered why only
Simon> location.address was set.  Either memset or {} is fine with me.

Thanks.  I think I will change it to {}, since that seems cleaner.

Tom

On 05/25/2018 08:49 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> Ah, I stumbled upon that code recently and wondered why only
> Simon> location.address was set.  Either memset or {} is fine with me.

Note that this is a union.

> 
> Thanks.  I think I will change it to {}, since that seems cleaner.

Note that {} has the same effect as only setting the first field
of the union with Clang, so it's a nop patch.  It shouldn't
really matter -- we're only supposed to access the active
member anyway.

Thanks,
Pedro Alves