[patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Jan Kratochvil]

On Friday 01 January 2010 21:45:05 Jan Kratochvil wrote:
> -PASS: gdb.mi/mi-var-child.exp: get children of struct_declarations.s2.u2.u1s1
> +FAIL: gdb.mi/mi-var-child.exp: get children of struct_declarations.s2.u2.u1s1
> -PASS: gdb.mi/mi2-var-child.exp: get children of struct_declarations.s2.u2.u1s1
> +FAIL: gdb.mi/mi2-var-child.exp: get children of struct_declarations.s2.u2.u1s1
> -PASS: gdb.python/py-mi.exp: examine container children=0, no pretty-printing
> +FAIL: gdb.python/py-mi.exp: examine container children=0, no pretty-printing
> 
> due to:
> Re: RFA: unbreak typedefed bitfield
> http://sourceware.org/ml/gdb-patches/2009-12/msg00295.html
> commit fc85da4ee2a7c32afc53b1b334a4f84e2e9bd84e
> http://sourceware.org/ml/gdb-cvs/2009-12/msg00100.html

attached a fix on top of existing HEAD.

Original PR gdb/10884 was a regression 6.8 -> 7.0 due to:
RFC: Lazy bitfields
http://sourceware.org/ml/gdb-patches/2009-07/msg00437.html
http://sourceware.org/ml/gdb-cvs/2009-07/msg00143.html
07491b3409f6ace0b7a9a707775a56ce10fece1c

No regressions for HEAD on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.  Plus:
-FAIL: gdb.mi/mi-var-child.exp: get children of struct_declarations.s2.u2.u1s1
+PASS: gdb.mi/mi-var-child.exp: get children of struct_declarations.s2.u2.u1s1
-FAIL: gdb.mi/mi2-var-child.exp: get children of struct_declarations.s2.u2.u1s1
+PASS: gdb.mi/mi2-var-child.exp: get children of struct_declarations.s2.u2.u1s1
-FAIL: gdb.python/py-mi.exp: examine container children=0, no pretty-printing
+PASS: gdb.python/py-mi.exp: examine container children=0, no pretty-printing

Going to check it in also for gdb_7_0-branch after an approval.


Thanks,
Jan


gdb/
2010-01-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* value.c (value_primitive_field): Remove one check_typedef call.
	Move bitpos and container_bitsize initialization after
	allocate_value_lazy.  New comment before accessing TYPE_LENGTH.

gdb/testsuite/
2010-01-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.mi/var-cmd.c (do_bitfield_tests): Change "V.sharable" type to
	"uint_for_mi_testing".

--- a/gdb/testsuite/gdb.mi/var-cmd.c
+++ b/gdb/testsuite/gdb.mi/var-cmd.c
@@ -494,7 +494,7 @@ void do_bitfield_tests ()
     mi_create_varobj V d "create varobj for Data"
     mi_list_varobj_children "V" {
         {"V.alloc" "alloc" "0" "int"}
-        {"V.sharable" "sharable" "0" "unsigned int"}
+        {"V.sharable" "sharable" "0" "uint_for_mi_testing"}
     } "list children of Data"
     mi_check_varobj_value V.sharable 3 "access bitfield"
     :*/
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1873,7 +1873,6 @@ value_primitive_field (struct value *arg1, int offset,
 
   CHECK_TYPEDEF (arg_type);
   type = TYPE_FIELD_TYPE (arg_type, fieldno);
-  type = check_typedef (type);
 
   /* Handle packed fields */
 
@@ -1885,10 +1884,14 @@ value_primitive_field (struct value *arg1, int offset,
 	 Otherwise, adjust offset to the byte containing the first
 	 bit.  Assume that the address, offset, and embedded offset
 	 are sufficiently aligned.  */
-      int bitpos = TYPE_FIELD_BITPOS (arg_type, fieldno);
-      int container_bitsize = TYPE_LENGTH (type) * 8;
+      int bitpos, container_bitsize;
 
       v = allocate_value_lazy (type);
+
+      /* TYPE_LENGTH of TYPE gets initialized by allocate_value_lazy.  */
+      bitpos = TYPE_FIELD_BITPOS (arg_type, fieldno);
+      container_bitsize = TYPE_LENGTH (type) * 8;
+
       v->bitsize = TYPE_FIELD_BITSIZE (arg_type, fieldno);
       if ((bitpos % container_bitsize) + v->bitsize <= container_bitsize
 	  && TYPE_LENGTH (type) <= (int) sizeof (LONGEST))
@@ -1939,6 +1942,8 @@ value_primitive_field (struct value *arg1, int offset,
       else
 	{
 	  v = allocate_value (type);
+
+	  /* TYPE_LENGTH of TYPE gets initialized by allocate_value.  */
 	  memcpy (value_contents_raw (v),
 		  value_contents_raw (arg1) + offset,
 		  TYPE_LENGTH (type));

Re: [patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Joel Brobecker]

> gdb/
> 2010-01-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* value.c (value_primitive_field): Remove one check_typedef call.
> 	Move bitpos and container_bitsize initialization after
> 	allocate_value_lazy.  New comment before accessing TYPE_LENGTH.

Interesting. This patch allowed me to figure out why the initial patch
sent by Vladimir was in fact working when it looks like it could not
possibly work :).

Personally, I'm not a big fan of the approach you took in this case.
You rely on a side-effect of the various allocate_* routines, but
this is only because these routines call check_typedef. In particular,
there is something interesting in allocate_value_lazy. A variable is
declared...

        struct type *atype = check_typedef (type);

... but then never used. There is no available info as to why this is,
as this is here since day 1 of the public CVS repo, but it's now clear
to me that this helps fixing that length - a type length that does not
appear to be used at all in the rest of the routine!

In my opinion, I agree with Daniel's comment that it is unusual to
call check_typedef without storing the function result.  But this
seems to be less awkward than relying on an undocumented side-effect
of a function that uses an undocumented side-effect of a check_typedef,
especially since the function in question does not really need that
side-effect to be applied at the time the function is called!

So, my proposal, if the other maintainers agree, is to document
the side-effect of check_typedef (sets the typedef TYPE_LENGTH)
as this appears to be a fully-intended behavior, and then do:

> -  type = check_typedef (type);

> +  /* Call check_typedef on our type to make sure that, if TYPE
> +     is a TYPE_CODE_TYPEDEF, its length is set to the length
> +     of the target type instead of zero.  However, we do not
> +     replace the typedef type by the target type, because we want
> +     to keep the typedef in order to be able to print the type
> +     description correctly.  */
> +  check_typedef (type);

> gdb/testsuite/
> 2010-01-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.mi/var-cmd.c (do_bitfield_tests): Change "V.sharable" type to
> 	"uint_for_mi_testing".

This part is OK.

-- 
Joel

Re: [patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Jan Kratochvil]

On Sun, 03 Jan 2010 05:57:17 +0100, Joel Brobecker wrote:
> So, my proposal, if the other maintainers agree, is to document
> the side-effect of check_typedef (sets the typedef TYPE_LENGTH)
> as this appears to be a fully-intended behavior, and then do:

OK, attached.  As I find the type system [censored] I am never sure how it can
be used in a sane way.

OK to check-in with this wording?

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-01-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Joel Brobecker  <brobecker@adacore.com>

	* gdbtypes.c (check_typedef): New comment on type length.
	* value.c (allocate_value_lazy): Remove the unused atype variable.  New
	comment on type length.
	(value_primitive_field): Keep the original TYPE value, new comment.

gdb/testsuite/
2010-01-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.mi/var-cmd.c (do_bitfield_tests): Change "V.sharable" type to
	"uint_for_mi_testing".

--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1342,12 +1342,17 @@ stub_noname_complaint (void)
    symbols which contain a full definition for the type.
 
    This used to be coded as a macro, but I don't think it is called 
-   often enough to merit such treatment.  */
+   often enough to merit such treatment.
 
-/* Find the real type of TYPE.  This function returns the real type,
+   Find the real type of TYPE.  This function returns the real type,
    after removing all layers of typedefs and completing opaque or stub
    types.  Completion changes the TYPE argument, but stripping of
-   typedefs does not.  */
+   typedefs does not.
+
+   If TYPE is a TYPE_CODE_TYPEDEF, its length is (also) set to the length of
+   the target type instead of zero.  However, in the case of TYPE_CODE_TYPEDEF
+   check_typedef can still return different type than the original TYPE
+   pointer.  */
 
 struct type *
 check_typedef (struct type *type)
--- a/gdb/testsuite/gdb.mi/var-cmd.c
+++ b/gdb/testsuite/gdb.mi/var-cmd.c
@@ -494,7 +494,7 @@ void do_bitfield_tests ()
     mi_create_varobj V d "create varobj for Data"
     mi_list_varobj_children "V" {
         {"V.alloc" "alloc" "0" "int"}
-        {"V.sharable" "sharable" "0" "unsigned int"}
+        {"V.sharable" "sharable" "0" "uint_for_mi_testing"}
     } "list children of Data"
     mi_check_varobj_value V.sharable 3 "access bitfield"
     :*/
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -254,7 +254,14 @@ struct value *
 allocate_value_lazy (struct type *type)
 {
   struct value *val;
-  struct type *atype = check_typedef (type);
+
+  /* Call check_typedef on our type to make sure that, if TYPE
+     is a TYPE_CODE_TYPEDEF, its length is set to the length
+     of the target type instead of zero.  However, we do not
+     replace the typedef type by the target type, because we want
+     to keep the typedef in order to be able to set the VAL's type
+     description correctly.  */
+  check_typedef (type);
 
   val = (struct value *) xzalloc (sizeof (struct value));
   val->contents = NULL;
@@ -1873,7 +1880,14 @@ value_primitive_field (struct value *arg1, int offset,
 
   CHECK_TYPEDEF (arg_type);
   type = TYPE_FIELD_TYPE (arg_type, fieldno);
-  type = check_typedef (type);
+
+  /* Call check_typedef on our type to make sure that, if TYPE
+     is a TYPE_CODE_TYPEDEF, its length is set to the length
+     of the target type instead of zero.  However, we do not
+     replace the typedef type by the target type, because we want
+     to keep the typedef in order to be able to print the type
+     description correctly.  */
+  check_typedef (type);
 
   /* Handle packed fields */
 

Re: [patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Joel Brobecker]

> gdb/
> 2010-01-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Joel Brobecker  <brobecker@adacore.com>
> 
> 	* gdbtypes.c (check_typedef): New comment on type length.
> 	* value.c (allocate_value_lazy): Remove the unused atype variable.  New
> 	comment on type length.
> 	(value_primitive_field): Keep the original TYPE value, new comment.
> 
> gdb/testsuite/
> 2010-01-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.mi/var-cmd.c (do_bitfield_tests): Change "V.sharable" type to
> 	"uint_for_mi_testing".

That would be fine with me - thanks for doing that. Confirmation from
DanielJ would be nice, though, since he raised concerns about it.
Hopefully all the comments we added will address them.

-- 
Joel

Re: [patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Daniel Jacobowitz]

On Sun, Jan 03, 2010 at 10:03:05AM +0400, Joel Brobecker wrote:
> That would be fine with me - thanks for doing that. Confirmation from
> DanielJ would be nice, though, since he raised concerns about it.
> Hopefully all the comments we added will address them.

It looks fine to me.  I agree with Jan that this bit of our type
system is a bad idea; I'd rather TYPE_LENGTH just worked.  That's
for some other day.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Jan Kratochvil]

On Sun, 03 Jan 2010 18:06:03 +0100, Daniel Jacobowitz wrote:
> On Sun, Jan 03, 2010 at 10:03:05AM +0400, Joel Brobecker wrote:
> > That would be fine with me - thanks for doing that. Confirmation from
> > DanielJ would be nice, though, since he raised concerns about it.
> > Hopefully all the comments we added will address them.
> 
> It looks fine to me.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-01/msg00026.html


No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu
for gdb_7_0-branch - but not checked-in there:

For gdb_7_0-branch - is it OK to add the new ChangeLog entries without
rotating ChangeLog to ChangeLog-2009?


Thanks,
Jan

Re: [patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Daniel Jacobowitz]

On Sun, Jan 03, 2010 at 08:25:50PM +0100, Jan Kratochvil wrote:
> For gdb_7_0-branch - is it OK to add the new ChangeLog entries without
> rotating ChangeLog to ChangeLog-2009?

Yes.  I don't think we should rotate on a released branch; that'll
just be a hassle.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Jan Kratochvil]

On Sun, 03 Jan 2010 22:52:30 +0100, Daniel Jacobowitz wrote:
> On Sun, Jan 03, 2010 at 08:25:50PM +0100, Jan Kratochvil wrote:
> > For gdb_7_0-branch - is it OK to add the new ChangeLog entries without
> > rotating ChangeLog to ChangeLog-2009?
> 
> Yes.  I don't think we should rotate on a released branch; that'll
> just be a hassle.

Checked-in now also on gdb_7_0-branch:
	http://sourceware.org/ml/gdb-cvs/2010-01/msg00027.html


Thanks,
Jan

Re: [patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> In my opinion, I agree with Daniel's comment that it is unusual to
Joel> call check_typedef without storing the function result.

FWIW, we do have a few of these.  We should at least comment that this
is called for side effects where this is done, I've tried to do that
with new calls.

Joel> So, my proposal, if the other maintainers agree, is to document
Joel> the side-effect of check_typedef (sets the typedef TYPE_LENGTH)
Joel> as this appears to be a fully-intended behavior, and then do:

This is documented by the TYPE_LENGTH macro, which is how I knew about
it :-).  This comment also mentions that allocate_value calls
check_typedef.

Tom