* [PATCH] Handle bitfields inside inner structs for internalvars
@ 2013-02-06 20:39 Sergio Durigan Junior
2013-02-08 20:47 ` Jan Kratochvil
0 siblings, 1 reply; 6+ messages in thread
From: Sergio Durigan Junior @ 2013-02-06 20:39 UTC (permalink / raw)
To: GDB Patches; +Cc: Jan Kratochvil
Hi,
<https://bugzilla.redhat.com/show_bug.cgi?id=903734>
(The bug is marked as private, I believe you won't be able to see it.)
This is a very specific bug. It occurs when one tries to set the value
of an internal variable's bitfield which is declared inside an inner
structure, i.e., a structure declared inside another. For example:
struct foo
{
int a;
struct
{
unsigned int b : 1;
} inner;
};
Now, consider that we create an internal variable out of this structure:
(gdb) set $p = (struct foo) {0}
(gdb) print $p
$1 = {a = 0, inner = {b = 0}}
Now, if we try to set the bitfield `b':
(gdb) set $p.inner.b = 1
(gdb) p $p
$2 = {a = 1, inner = {b = 0}}
This happens because of some miscalculations of offsets inside
gdb/valops.c:value_assign: GDB forgets to add the offset of the `inner'
struct to the offset of the bitfield, thus setting the wrong field.
The patch below fixes it, and includes some new tests for this specific
scenario. I ran a regression test on my Fedora 16 box (x86, x86_64 with
and without -m32, gdbserver for both), and found no regressions.
OK to apply?
--
Sergio
gdb/
2013-02-06 Sergio Durigan Junior <sergiodj@redhat.com>
* valops.c (value_assign): Handling bitfield offset in
`lval_internalvar_component' case.
gdb/testsuite/
2013-02-06 Sergio Durigan Junior <sergiodj@redhat.com>
* gdb.base/bitfields.c (struct internalvartest): New declaration.
* gdb.base/bitfields.exp (bitfield_internalvar): New function.
---
gdb/testsuite/gdb.base/bitfields.c | 16 ++++++++++++++++
gdb/testsuite/gdb.base/bitfields.exp | 26 ++++++++++++++++++++++++++
gdb/valops.c | 17 ++++++++++++-----
3 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/gdb/testsuite/gdb.base/bitfields.c b/gdb/testsuite/gdb.base/bitfields.c
index ed1634c..3a6b76f 100644
--- a/gdb/testsuite/gdb.base/bitfields.c
+++ b/gdb/testsuite/gdb.base/bitfields.c
@@ -23,6 +23,22 @@ struct fields
signed char sc ;
} flags;
+struct internalvartest
+{
+ unsigned int a : 1;
+ struct
+ {
+ unsigned int b : 1;
+ struct
+ {
+ unsigned int c : 1;
+ signed int d : 1;
+ } deep;
+ signed int e : 1;
+ } inner;
+ signed int f : 1;
+} dummy_internalvartest;
+
void break1 ()
{
}
diff --git a/gdb/testsuite/gdb.base/bitfields.exp b/gdb/testsuite/gdb.base/bitfields.exp
index 9095736..c2e1c63 100644
--- a/gdb/testsuite/gdb.base/bitfields.exp
+++ b/gdb/testsuite/gdb.base/bitfields.exp
@@ -245,6 +245,31 @@ proc bitfield_at_offset {} {
gdb_test "print container.two.u3" ".* = 3"
}
+proc bitfield_internalvar {} {
+ global gdb_prompt
+
+ # First, we create an internal var holding an instance of
+ # the struct (zeroed out).
+ gdb_test "set \$myvar = \(struct internalvartest\) \{0\}" "" \
+ "set internal var"
+
+ # Now, we set the proper bits.
+ gdb_test_no_output "set \$myvar.a = 0"
+ gdb_test_no_output "set \$myvar.inner.b = 1"
+ gdb_test_no_output "set \$myvar.inner.deep.c = 0"
+ gdb_test_no_output "set \$myvar.inner.deep.d = -1"
+ gdb_test_no_output "set \$myvar.inner.e = 1"
+ gdb_test_no_output "set \$myvar.f = 1"
+
+ # Here comes the true testing.
+ gdb_test "print \$myvar.a" "\\$\[0-9\]\+ = 0"
+ gdb_test "print \$myvar.inner.b" "\\$\[0-9\]\+ = 1"
+ gdb_test "print \$myvar.inner.deep.c" "\\$\[0-9\]\+ = 0"
+ gdb_test "print \$myvar.inner.deep.d" "\\$\[0-9\]\+ = -1"
+ gdb_test "print \$myvar.inner.e" "\\$\[0-9\]\+ = -1"
+ gdb_test "print \$myvar.f" "\\$\[0-9\]\+ = -1"
+}
+
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}
@@ -256,3 +281,4 @@ bitfield_containment
bitfield_unsignedness
bitfield_signedness
bitfield_at_offset
+bitfield_internalvar
diff --git a/gdb/valops.c b/gdb/valops.c
index 2132f3e..625e8d6 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1233,11 +1233,18 @@ value_assign (struct value *toval, struct value *fromval)
VALUE_INTERNALVAR (toval));
case lval_internalvar_component:
- set_internalvar_component (VALUE_INTERNALVAR (toval),
- value_offset (toval),
- value_bitpos (toval),
- value_bitsize (toval),
- fromval);
+ {
+ int offset = value_offset (toval);
+
+ if (value_bitsize (toval))
+ offset += value_offset (value_parent (toval));
+
+ set_internalvar_component (VALUE_INTERNALVAR (toval),
+ offset,
+ value_bitpos (toval),
+ value_bitsize (toval),
+ fromval);
+ }
break;
case lval_memory:
--
1.7.7.6
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Handle bitfields inside inner structs for internalvars
2013-02-06 20:39 [PATCH] Handle bitfields inside inner structs for internalvars Sergio Durigan Junior
@ 2013-02-08 20:47 ` Jan Kratochvil
2013-02-09 4:52 ` Sergio Durigan Junior
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kratochvil @ 2013-02-08 20:47 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: GDB Patches
On Wed, 06 Feb 2013 21:39:10 +0100, Sergio Durigan Junior wrote:
> gdb/
> 2013-02-06 Sergio Durigan Junior <sergiodj@redhat.com>
>
> * valops.c (value_assign): Handling bitfield offset in
> `lval_internalvar_component' case.
>
> gdb/testsuite/
> 2013-02-06 Sergio Durigan Junior <sergiodj@redhat.com>
>
> * gdb.base/bitfields.c (struct internalvartest): New declaration.
> * gdb.base/bitfields.exp (bitfield_internalvar): New function.
OK for check-in with the bits below.
> --- a/gdb/testsuite/gdb.base/bitfields.exp
> +++ b/gdb/testsuite/gdb.base/bitfields.exp
> @@ -245,6 +245,31 @@ proc bitfield_at_offset {} {
> gdb_test "print container.two.u3" ".* = 3"
> }
>
> +proc bitfield_internalvar {} {
> + global gdb_prompt
> +
> + # First, we create an internal var holding an instance of
> + # the struct (zeroed out).
> + gdb_test "set \$myvar = \(struct internalvartest\) \{0\}" "" \
Backslashes are excessive here, parentheses are not special characters for the
TCL interpretation and this string is not regexp:
gdb_test "set \$myvar = (struct internalvartest) \{0\}" "" \
> + "set internal var"
> +
> + # Now, we set the proper bits.
[...]
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -1233,11 +1233,18 @@ value_assign (struct value *toval, struct value *fromval)
> VALUE_INTERNALVAR (toval));
>
> case lval_internalvar_component:
> - set_internalvar_component (VALUE_INTERNALVAR (toval),
> - value_offset (toval),
> - value_bitpos (toval),
> - value_bitsize (toval),
> - fromval);
> + {
> + int offset = value_offset (toval);
> +
> + if (value_bitsize (toval))
> + offset += value_offset (value_parent (toval));
value_address rather tests for value_parent existence; although value_bitsize
is right as value_parent is currently not used elsewhere.
if (value_parent (toval))
{
/* VALUE_INTERNALVAR below corresponds to the parent value while
offset is relative to the parent value. */
gdb_assert (value_parent (value_parent (toval)) == NULL);
offset += value_offset (value_parent (toval));
}
And it was not so clear to me on the first look so I have added this comment
if you are OK with it.
> +
> + set_internalvar_component (VALUE_INTERNALVAR (toval),
> + offset,
> + value_bitpos (toval),
> + value_bitsize (toval),
> + fromval);
> + }
> break;
>
Thanks,
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Handle bitfields inside inner structs for internalvars
2013-02-08 20:47 ` Jan Kratochvil
@ 2013-02-09 4:52 ` Sergio Durigan Junior
2013-02-09 7:39 ` Jan Kratochvil
0 siblings, 1 reply; 6+ messages in thread
From: Sergio Durigan Junior @ 2013-02-09 4:52 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: GDB Patches
On Friday, February 08 2013, Jan Kratochvil wrote:
> On Wed, 06 Feb 2013 21:39:10 +0100, Sergio Durigan Junior wrote:
>> --- a/gdb/testsuite/gdb.base/bitfields.exp
>> +++ b/gdb/testsuite/gdb.base/bitfields.exp
>> @@ -245,6 +245,31 @@ proc bitfield_at_offset {} {
>> gdb_test "print container.two.u3" ".* = 3"
>> }
>>
>> +proc bitfield_internalvar {} {
>> + global gdb_prompt
>> +
>> + # First, we create an internal var holding an instance of
>> + # the struct (zeroed out).
>> + gdb_test "set \$myvar = \(struct internalvartest\) \{0\}" "" \
>
> Backslashes are excessive here, parentheses are not special characters for the
> TCL interpretation and this string is not regexp:
> gdb_test "set \$myvar = (struct internalvartest) \{0\}" "" \
Thanks for the review. These extra backslashes were added by me while
testing a failure that I was seeing, sorry for letting those sneak in.
>> --- a/gdb/valops.c
>> +++ b/gdb/valops.c
>> @@ -1233,11 +1233,18 @@ value_assign (struct value *toval, struct value *fromval)
>> VALUE_INTERNALVAR (toval));
>>
>> case lval_internalvar_component:
>> - set_internalvar_component (VALUE_INTERNALVAR (toval),
>> - value_offset (toval),
>> - value_bitpos (toval),
>> - value_bitsize (toval),
>> - fromval);
>> + {
>> + int offset = value_offset (toval);
>> +
>> + if (value_bitsize (toval))
>> + offset += value_offset (value_parent (toval));
>
> value_address rather tests for value_parent existence; although value_bitsize
> is right as value_parent is currently not used elsewhere.
>
> if (value_parent (toval))
Do you think it's clearer to use `value_parent' here instead of
`value_bitsize'?
> {
> /* VALUE_INTERNALVAR below corresponds to the parent value while
> offset is relative to the parent value. */
> gdb_assert (value_parent (value_parent (toval)) == NULL);
> offset += value_offset (value_parent (toval));
> }
>
> And it was not so clear to me on the first look so I have added this comment
> if you are OK with it.
Thanks, the comment is fine by me. I'll wait for your reply to the
question above, and then proceed.
--
Sergio
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Handle bitfields inside inner structs for internalvars
2013-02-09 4:52 ` Sergio Durigan Junior
@ 2013-02-09 7:39 ` Jan Kratochvil
2013-02-11 18:06 ` Sergio Durigan Junior
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kratochvil @ 2013-02-09 7:39 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: GDB Patches
On Sat, 09 Feb 2013 05:52:32 +0100, Sergio Durigan Junior wrote:
> On Friday, February 08 2013, Jan Kratochvil wrote:
> >> + if (value_bitsize (toval))
> >> + offset += value_offset (value_parent (toval));
> >
> > value_address rather tests for value_parent existence; although value_bitsize
> > is right as value_parent is currently not used elsewhere.
> >
> > if (value_parent (toval))
>
> Do you think it's clearer to use `value_parent' here instead of
> `value_bitsize'?
Choose any way but therefore put there a comment that value_parent is non-NULL
iff value_bitsize is non-zero.
Otherwise I was curious - what to do if value_parent exists but TOVAL is not
a bitfield? Isn't it a forgotten case? (It is not but...)
Thanks,
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Handle bitfields inside inner structs for internalvars
2013-02-09 7:39 ` Jan Kratochvil
@ 2013-02-11 18:06 ` Sergio Durigan Junior
2013-02-11 18:11 ` Jan Kratochvil
0 siblings, 1 reply; 6+ messages in thread
From: Sergio Durigan Junior @ 2013-02-11 18:06 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: GDB Patches
On Saturday, February 09 2013, Jan Kratochvil wrote:
> On Sat, 09 Feb 2013 05:52:32 +0100, Sergio Durigan Junior wrote:
>> On Friday, February 08 2013, Jan Kratochvil wrote:
>> >> + if (value_bitsize (toval))
>> >> + offset += value_offset (value_parent (toval));
>> >
>> > value_address rather tests for value_parent existence; although value_bitsize
>> > is right as value_parent is currently not used elsewhere.
>> >
>> > if (value_parent (toval))
>>
>> Do you think it's clearer to use `value_parent' here instead of
>> `value_bitsize'?
>
> Choose any way but therefore put there a comment that value_parent is non-NULL
> iff value_bitsize is non-zero.
Ok, thanks, I committed the patch below.
> Otherwise I was curious - what to do if value_parent exists but TOVAL is not
> a bitfield? Isn't it a forgotten case? (It is not but...)
According to comments in gdb/value.{c,h}, value_parent is only used iff
the we are dealing with bitfields, so I guess this case is covered
(otherwise it is a bug). Was it a rhetorical question?
Checked-in:
http://sourceware.org/ml/gdb-cvs/2013-02/msg00071.html
Thanks,
--
Sergio
2013-02-11 Sergio Durigan Junior <sergiodj@redhat.com>
* valops.c (value_assign): Handling bitfield offset in
`lval_internalvar_component' case.
2013-02-11 Sergio Durigan Junior <sergiodj@redhat.com>
* gdb.base/bitfields.c (struct internalvartest): New declaration.
* gdb.base/bitfields.exp (bitfield_internalvar): New function.
---
gdb/testsuite/gdb.base/bitfields.c | 16 ++++++++++++++++
gdb/testsuite/gdb.base/bitfields.exp | 26 ++++++++++++++++++++++++++
gdb/valops.c | 26 +++++++++++++++++++++-----
3 files changed, 63 insertions(+), 5 deletions(-)
diff --git a/gdb/testsuite/gdb.base/bitfields.c b/gdb/testsuite/gdb.base/bitfields.c
index ed1634c..3a6b76f 100644
--- a/gdb/testsuite/gdb.base/bitfields.c
+++ b/gdb/testsuite/gdb.base/bitfields.c
@@ -23,6 +23,22 @@ struct fields
signed char sc ;
} flags;
+struct internalvartest
+{
+ unsigned int a : 1;
+ struct
+ {
+ unsigned int b : 1;
+ struct
+ {
+ unsigned int c : 1;
+ signed int d : 1;
+ } deep;
+ signed int e : 1;
+ } inner;
+ signed int f : 1;
+} dummy_internalvartest;
+
void break1 ()
{
}
diff --git a/gdb/testsuite/gdb.base/bitfields.exp b/gdb/testsuite/gdb.base/bitfields.exp
index 9095736..82f7b10 100644
--- a/gdb/testsuite/gdb.base/bitfields.exp
+++ b/gdb/testsuite/gdb.base/bitfields.exp
@@ -245,6 +245,31 @@ proc bitfield_at_offset {} {
gdb_test "print container.two.u3" ".* = 3"
}
+proc bitfield_internalvar {} {
+ global gdb_prompt
+
+ # First, we create an internal var holding an instance of
+ # the struct (zeroed out).
+ gdb_test "set \$myvar = (struct internalvartest) \{0\}" "" \
+ "set internal var"
+
+ # Now, we set the proper bits.
+ gdb_test_no_output "set \$myvar.a = 0"
+ gdb_test_no_output "set \$myvar.inner.b = 1"
+ gdb_test_no_output "set \$myvar.inner.deep.c = 0"
+ gdb_test_no_output "set \$myvar.inner.deep.d = -1"
+ gdb_test_no_output "set \$myvar.inner.e = 1"
+ gdb_test_no_output "set \$myvar.f = 1"
+
+ # Here comes the true testing.
+ gdb_test "print \$myvar.a" "\\$\[0-9\]\+ = 0"
+ gdb_test "print \$myvar.inner.b" "\\$\[0-9\]\+ = 1"
+ gdb_test "print \$myvar.inner.deep.c" "\\$\[0-9\]\+ = 0"
+ gdb_test "print \$myvar.inner.deep.d" "\\$\[0-9\]\+ = -1"
+ gdb_test "print \$myvar.inner.e" "\\$\[0-9\]\+ = -1"
+ gdb_test "print \$myvar.f" "\\$\[0-9\]\+ = -1"
+}
+
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}
@@ -256,3 +281,4 @@ bitfield_containment
bitfield_unsignedness
bitfield_signedness
bitfield_at_offset
+bitfield_internalvar
diff --git a/gdb/valops.c b/gdb/valops.c
index 2132f3e..93c09d8 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1233,11 +1233,27 @@ value_assign (struct value *toval, struct value *fromval)
VALUE_INTERNALVAR (toval));
case lval_internalvar_component:
- set_internalvar_component (VALUE_INTERNALVAR (toval),
- value_offset (toval),
- value_bitpos (toval),
- value_bitsize (toval),
- fromval);
+ {
+ int offset = value_offset (toval);
+
+ /* Are we dealing with a bitfield?
+
+ It is important to mention that `value_parent (toval)' is
+ non-NULL iff `value_bitsize (toval)' is non-zero. */
+ if (value_bitsize (toval))
+ {
+ /* VALUE_INTERNALVAR below refers to the parent value, while
+ the offset is relative to this parent value. */
+ gdb_assert (value_parent (value_parent (toval)) == NULL);
+ offset += value_offset (value_parent (toval));
+ }
+
+ set_internalvar_component (VALUE_INTERNALVAR (toval),
+ offset,
+ value_bitpos (toval),
+ value_bitsize (toval),
+ fromval);
+ }
break;
case lval_memory:
--
1.7.7.6
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Handle bitfields inside inner structs for internalvars
2013-02-11 18:06 ` Sergio Durigan Junior
@ 2013-02-11 18:11 ` Jan Kratochvil
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kratochvil @ 2013-02-11 18:11 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: GDB Patches
On Mon, 11 Feb 2013 19:05:58 +0100, Sergio Durigan Junior wrote:
> On Saturday, February 09 2013, Jan Kratochvil wrote:
> > Otherwise I was curious - what to do if value_parent exists but TOVAL is not
> > a bitfield? Isn't it a forgotten case? (It is not but...)
>
> According to comments in gdb/value.{c,h}, value_parent is only used iff
> the we are dealing with bitfields, so I guess this case is covered
> (otherwise it is a bug). Was it a rhetorical question?
I expected a future reader may have such a question.
I just believed the code was worth some comment, thanks for it. If it were
absolutely clear the bug would not happen there in the first place.
Thanks,
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-11 18:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-06 20:39 [PATCH] Handle bitfields inside inner structs for internalvars Sergio Durigan Junior
2013-02-08 20:47 ` Jan Kratochvil
2013-02-09 4:52 ` Sergio Durigan Junior
2013-02-09 7:39 ` Jan Kratochvil
2013-02-11 18:06 ` Sergio Durigan Junior
2013-02-11 18:11 ` Jan Kratochvil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox