* [RFA] Clear entire "location" in value constructor
@ 2018-04-10 17:37 Tom Tromey
2018-04-25 15:33 ` Tom Tromey
2018-05-25 19:53 ` Simon Marchi
0 siblings, 2 replies; 7+ messages in thread
From: Tom Tromey @ 2018-04-10 17:37 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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 ()
--
2.13.6
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Clear entire "location" in value constructor
2018-04-10 17:37 [RFA] Clear entire "location" in value constructor Tom Tromey
@ 2018-04-25 15:33 ` Tom Tromey
2018-05-09 15:40 ` Tom Tromey
2018-05-25 19:53 ` Simon Marchi
1 sibling, 1 reply; 7+ messages in thread
From: Tom Tromey @ 2018-04-25 15:33 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Clear entire "location" in value constructor
2018-04-25 15:33 ` Tom Tromey
@ 2018-05-09 15:40 ` Tom Tromey
2018-05-25 17:30 ` Tom Tromey
0 siblings, 1 reply; 7+ messages in thread
From: Tom Tromey @ 2018-05-09 15:40 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Clear entire "location" in value constructor
2018-05-09 15:40 ` Tom Tromey
@ 2018-05-25 17:30 ` Tom Tromey
0 siblings, 0 replies; 7+ messages in thread
From: Tom Tromey @ 2018-05-25 17:30 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Clear entire "location" in value constructor
2018-04-10 17:37 [RFA] Clear entire "location" in value constructor Tom Tromey
2018-04-25 15:33 ` Tom Tromey
@ 2018-05-25 19:53 ` Simon Marchi
2018-05-25 21:46 ` Tom Tromey
1 sibling, 1 reply; 7+ messages in thread
From: Simon Marchi @ 2018-05-25 19:53 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Clear entire "location" in value constructor
2018-05-25 19:53 ` Simon Marchi
@ 2018-05-25 21:46 ` Tom Tromey
2018-05-25 22:43 ` Pedro Alves
0 siblings, 1 reply; 7+ messages in thread
From: Tom Tromey @ 2018-05-25 21:46 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom Tromey, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Clear entire "location" in value constructor
2018-05-25 21:46 ` Tom Tromey
@ 2018-05-25 22:43 ` Pedro Alves
0 siblings, 0 replies; 7+ messages in thread
From: Pedro Alves @ 2018-05-25 22:43 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-05-25 19:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-10 17:37 [RFA] Clear entire "location" in value constructor Tom Tromey
2018-04-25 15:33 ` Tom Tromey
2018-05-09 15:40 ` Tom Tromey
2018-05-25 17:30 ` Tom Tromey
2018-05-25 19:53 ` Simon Marchi
2018-05-25 21:46 ` Tom Tromey
2018-05-25 22:43 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).