[RFA] Clear entire "location" in value constructor

Message ID 20180410173704.21702-1-tom@tromey.com
State New
Headers show
Series
  • [RFA] Clear entire "location" in value constructor
Related show

Commit Message

Tom Tromey April 10, 2018, 5:37 p.m.
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(-)

-- 
2.13.6

Comments

Tom Tromey April 25, 2018, 3:32 p.m. | #1
>>>>> "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 Tromey May 9, 2018, 3:40 p.m. | #2
>>>>> "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 Tromey May 25, 2018, 5:28 p.m. | #3
>>>>> "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
Simon Marchi May 25, 2018, 7:36 p.m. | #4
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
Tom Tromey May 25, 2018, 7:49 p.m. | #5
>>>>> "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
Pedro Alves May 25, 2018, 7:53 p.m. | #6
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

Patch

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 ()