RFA: unbreak typedefed bitfield

RFA: unbreak typedefed bitfield

[Vladimir Prus]

GDB presently crashes on any attempt to get a value of a bitfield that
has typedefed type. The bugzilla report is here:

	http://sourceware.org/bugzilla/show_bug.cgi?id=10884

An easier reproducer is this program:

typedef unsigned int uint;

      struct Data {
	  int alloc;
	  int begin;
	  int end;
	  uint sharable : 1;
      };

      int main()
      {
	  Data d = {1, 2, 3, 1};
	  return 0;
      }

and this session:

      ~"Breakpoint 1, main () at div.cpp:13\n"                                                                                                                
      ~"13\t    Data d = {1, 2, 3, 1};\n"                                                                                                                     
      *stopped,frame={addr="0x0804849a",func="main",args=[],file="div.cpp",fullname="/home/ghost/Build/gdb-git/gdb/div.cpp",line="13"},thread-id="1",stopped-threads="all",core="1"
      (gdb)                                                                                                                                                                        
      n                                                                                                                                                                            
      &"n\n"                                                                                                                                                                       
      &"During symbol reading, incomplete CFI data; unspecified registers (e.g., eax) at 0x8048497.\n"                                                                             
      ^running                                                                                                                                                                     
      *running,thread-id="1"                                                                                                                                                       
      (gdb)                                                                                                                                                                        
      ~"14\t    return 0;\n"                                                                                                                                                       
      *stopped,frame={addr="0x080484b9",func="main",args=[],file="div.cpp",fullname="/home/ghost/Build/gdb-git/gdb/div.cpp",line="14"},thread-id="1",stopped-threads="all",core="1"
      (gdb)                                                                                                                                                                        
      -var-create V * d                                                                                                                                                            
      ^done,name="V",numchild="1",value="{...}",type="Data",thread-id="1",has_more="0"                                                                                             
      (gdb)                                                                                                                                                                        
      -var-list-children V                                                                                                                                                         
      ^done,numchild="1",children=[child={name="V.public",exp="public",numchild="4",thread-id="1"}],has_more="0"                                                                   
      (gdb)                                                                                                                                                                        
      -var-list-children --all-values V.public                                                                                                                                     

      Program received signal SIGFPE, Arithmetic exception.
      0x0813add3 in value_primitive_field (arg1=0x8644840, offset=0, fieldno=3, arg_type=0x85a9d7c) at /home/ghost/Work/CodeSourcery/Projects/egdb/gdb-git/gdb/value.c:1892
      1892          if ((bitpos % container_bitsize) + v->bitsize <= container_bitsize

And here's a trivial patch to fix this problem. I would claim this clearly indicates a design bug in GDB
type mechanism -- and it bites us repeatedly, but I'm not ready to propose a specific way to fix it, yet.

OK to commit?

- Volodya

--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1873,6 +1873,7 @@ value_primitive_field (struct value *arg1, int offset,

   CHECK_TYPEDEF (arg_type);
   type = TYPE_FIELD_TYPE (arg_type, fieldno);
+  check_typedef (type);

   /* Handle packed fields */

Re: RFA: unbreak typedefed bitfield

[Joel Brobecker]

> And here's a trivial patch to fix this problem. I would claim this
> clearly indicates a design bug in GDB type mechanism -- and it bites
> us repeatedly, but I'm not ready to propose a specific way to fix it,
> yet.

I have noticed the same issue repeatedly in the Ada section of the code,
and it's been really annoying to bump into this. As a result, we've seen 
an explosion of check_typedef (or the Ada equivalent ada_check_typedef),
and I also feel that something's not right.  I have a general feeling
that most of the time, the typedef should have never been passed down.
But I haven't spent the time and effort to try to think globally.
I did have some thoughts for the Ada side, where things are a little
more codified (and where every type is also a typedef), but that
requires a revamp of the Ada code, which I'm leaving for later, when
most of AdaCore's code in the FSF tree.

In your case, we are being repeatdly bitten because we still manipulate
a raw struct gdb_type.  If we have an opaque structure, it would
probably be easier to have a more general solution.  Right now,
we access everything directly, so we have to be careful, depending
on the situation.

> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1873,6 +1873,7 @@ value_primitive_field (struct value *arg1, int offset,
> 
>    CHECK_TYPEDEF (arg_type);
>    type = TYPE_FIELD_TYPE (arg_type, fieldno);
> +  check_typedef (type);
> 
>    /* Handle packed fields */

Looks OK to me (you forgot the ChangeLog entry, btw).

-- 
Joel

Re: RFA: unbreak typedefed bitfield

[Daniel Jacobowitz]

On Fri, Dec 18, 2009 at 05:06:12PM +0400, Joel Brobecker wrote:
> > --- a/gdb/value.c
> > +++ b/gdb/value.c
> > @@ -1873,6 +1873,7 @@ value_primitive_field (struct value *arg1, int offset,
> > 
> >    CHECK_TYPEDEF (arg_type);
> >    type = TYPE_FIELD_TYPE (arg_type, fieldno);
> > +  check_typedef (type);
> > 
> >    /* Handle packed fields */
> 
> Looks OK to me (you forgot the ChangeLog entry, btw).

What does this do?  CHECK_TYPEDEF side-effects its argument;
check_typedef will leave its argument as a TYPE_CODE_TYPEDEF.  I think
it fills in some other fields in the typedef, is that what matters
here?

It's unusual to see check_typedef without an assignment.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: unbreak typedefed bitfield

[Vladimir Prus]

On Friday 18 December 2009 17:17:40 Daniel Jacobowitz wrote:

> On Fri, Dec 18, 2009 at 05:06:12PM +0400, Joel Brobecker wrote:
> > > --- a/gdb/value.c
> > > +++ b/gdb/value.c
> > > @@ -1873,6 +1873,7 @@ value_primitive_field (struct value *arg1, int offset,
> > > 
> > >    CHECK_TYPEDEF (arg_type);
> > >    type = TYPE_FIELD_TYPE (arg_type, fieldno);
> > > +  check_typedef (type);
> > > 
> > >    /* Handle packed fields */
> > 
> > Looks OK to me (you forgot the ChangeLog entry, btw).
> 
> What does this do?  CHECK_TYPEDEF side-effects its argument;
> check_typedef will leave its argument as a TYPE_CODE_TYPEDEF.  I think
> it fills in some other fields in the typedef, is that what matters
> here?

Yes. It makes

	 int container_bitsize = TYPE_LENGTH (type) * 8;

below not to set 'container_bitsize' to 0.

> It's unusual to see check_typedef without an assignment.

I can surely use

	type = check_typedef (type);

if that seems less confusing.

- Volodya

Re: RFA: unbreak typedefed bitfield

[Daniel Jacobowitz]

On Fri, Dec 18, 2009 at 05:20:19PM +0300, Vladimir Prus wrote:
> I can surely use
> 
> 	type = check_typedef (type);
> 
> if that seems less confusing.

Yes, please.  That version's fine.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: unbreak typedefed bitfield

[Tom Tromey]

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

Joel> I have a general feeling that most of the time, the typedef should
Joel> have never been passed down.  But I haven't spent the time and
Joel> effort to try to think globally.

There are definitely cases where we want to preserve the type that the
user wrote.

In C, this matters for printing various character types.  E.g., both
wchar_t and char32_t may be typedefs of int, but we want to print them
differently if they use different encodings.

This preservation has to be pervasive, because of things like:

    print (wchar_t) 32

This example doesn't work today, but probably should.


I agree that check_typedef is a problem.

Perhaps we could approach the check_typedef problem using a semantic
analyzer to ensure correct use.  There are other idioms in gdb that also
require careful attention that would benefit from this; cleanups and
proper use of TRY_CATCH come to mind, but there are probably others.

Another approach would be to change TYPE_LENGTH to first call
check_typedef.

Tom

Re: RFA: unbreak typedefed bitfield

[Vladimir Prus]

[-- Attachment #1: Type: Text/Plain, Size: 342 bytes --]

On Friday 18 December 2009 17:24:07 Daniel Jacobowitz wrote:

> On Fri, Dec 18, 2009 at 05:20:19PM +0300, Vladimir Prus wrote:
> > I can surely use
> > 
> > 	type = check_typedef (type);
> > 
> > if that seems less confusing.
> 
> Yes, please.  That version's fine.

Here's what I've checked in — with tests added.

- Volodya

[-- Attachment #2: 10884-final.diff --]
[-- Type: text/x-patch, Size: 3947 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.11157
diff -u -p -r1.11157 ChangeLog
--- gdb/ChangeLog	21 Dec 2009 07:40:04 -0000	1.11157
+++ gdb/ChangeLog	21 Dec 2009 09:49:47 -0000
@@ -1,3 +1,10 @@
+2009-12-21  Vladimir Prus  <vladimir@codesourcery.com>
+
+	PR gdb/10884
+
+	* value.c (value_primitive_field): Call check_typedef
+	on the type.
+
 2009-12-21  Joel Brobecker  <brobecker@adacore.com>
 
 	* COPYING: Update to GPL version 3.
Index: gdb/value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.96
diff -u -p -r1.96 value.c
--- gdb/value.c	31 Aug 2009 20:18:45 -0000	1.96
+++ gdb/value.c	21 Dec 2009 09:49:47 -0000
@@ -1873,6 +1873,7 @@ value_primitive_field (struct value *arg
 
   CHECK_TYPEDEF (arg_type);
   type = TYPE_FIELD_TYPE (arg_type, fieldno);
+  type = check_typedef (type);
 
   /* Handle packed fields */
 
Index: gdb/testsuite/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.2049
diff -u -p -r1.2049 ChangeLog
--- gdb/testsuite/ChangeLog	20 Dec 2009 11:55:24 -0000	1.2049
+++ gdb/testsuite/ChangeLog	21 Dec 2009 09:49:48 -0000
@@ -1,3 +1,11 @@
+2009-12-21  Vladimir Prus  <vladimir@codesourcery.com>
+
+	PR gdb/10884
+
+	* gdb.mi/var-cmd.c (do_bitfield_tests): New
+	(main): Call do_bitfield_tests.
+	* gdb.mi/mi-cmd-var.exp: Run the 'bitfield' testcase.
+
 2009-12-20  Joel Brobecker  <brobecker@adacore.com>
 
 	* Makefile.in gdb.ada/gnat_ada.gpr, gdb.base/gcore-buffer-overflow.c,
Index: gdb/testsuite/gdb.mi/mi-var-cmd.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-var-cmd.exp,v
retrieving revision 1.45
diff -u -p -r1.45 mi-var-cmd.exp
--- gdb/testsuite/gdb.mi/mi-var-cmd.exp	15 Sep 2009 18:51:25 -0000	1.45
+++ gdb/testsuite/gdb.mi/mi-var-cmd.exp	21 Dec 2009 09:49:48 -0000
@@ -577,6 +577,8 @@ proc set_frozen {varobjs flag} {
 mi_prepare_inline_tests $srcfile
 mi_run_inline_test frozen
 
+mi_run_inline_test bitfield
+
 # Since the inline test framework does not really work with
 # function calls, first to inline tests and then do the reminder
 # manually.
Index: gdb/testsuite/gdb.mi/var-cmd.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/var-cmd.c,v
retrieving revision 1.19
diff -u -p -r1.19 var-cmd.c
--- gdb/testsuite/gdb.mi/var-cmd.c	3 Jan 2009 05:58:06 -0000	1.19
+++ gdb/testsuite/gdb.mi/var-cmd.c	21 Dec 2009 09:49:48 -0000
@@ -468,6 +468,40 @@ void do_at_tests ()
   /*: END: floating :*/
 }
 
+/* Some header appear to define uint already, so apply some
+   uglification.  Note that without uglification, the compile
+   does not fail, rather, we don't test what we want because
+   something else calls check_typedef on 'uint' already.  */
+typedef unsigned int uint_for_mi_testing;
+
+struct Data {
+  int alloc;
+  uint_for_mi_testing sharable : 4;
+};
+
+/* Accessing a value of a bitfield whose type is a typed used to
+   result in division by zero.  See:
+
+         http://sourceware.org/bugzilla/show_bug.cgi?id=10884
+
+   This tests for this bug.  */
+
+void do_bitfield_tests ()
+{
+  /*: BEGIN: bitfield :*/
+  struct Data d = {0, 3};
+  /*:
+    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"}
+    } "list children of Data"
+    mi_check_varobj_value V.sharable 3 "access bitfield"
+    :*/
+  return;
+  /*: END: bitfield :*/  
+}
+
 int
 main (int argc, char *argv [])
 {
@@ -477,6 +511,7 @@ main (int argc, char *argv [])
   do_special_tests ();
   do_frozen_tests ();
   do_at_tests ();
+  do_bitfield_tests ();
   exit (0);
 }
 

Re: RFA: unbreak typedefed bitfield

[Vladimir Prus]

Tom Tromey wrote:

>>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel> I have a general feeling that most of the time, the typedef should
> Joel> have never been passed down.  But I haven't spent the time and
> Joel> effort to try to think globally.
> 
> There are definitely cases where we want to preserve the type that the
> user wrote.
> 
> In C, this matters for printing various character types.  E.g., both
> wchar_t and char32_t may be typedefs of int, but we want to print them
> differently if they use different encodings.
> 
> This preservation has to be pervasive, because of things like:
> 
>     print (wchar_t) 32
> 
> This example doesn't work today, but probably should.
> 
> 
> I agree that check_typedef is a problem.
> 
> Perhaps we could approach the check_typedef problem using a semantic
> analyzer to ensure correct use.  There are other idioms in gdb that also
> require careful attention that would benefit from this; cleanups and
> proper use of TRY_CATCH come to mind, but there are probably others.
> 
> Another approach would be to change TYPE_LENGTH to first call
> check_typedef.

It seems to me that TYPE_LENGTH may return different values before and
after check_typedef is called. Is the 'before' value ever or any use?
If no, and as you say above in some cases we need to preserve some properties
of the typedef, why TYPE_LENGTH could not check if the type is typedef, and
if so, return length of the true type?

- Volodya

Re: RFA: unbreak typedefed bitfield

[Joel Brobecker]

> +2009-12-21  Vladimir Prus  <vladimir@codesourcery.com>
> +
> +	PR gdb/10884
> +
> +	* value.c (value_primitive_field): Call check_typedef
> +	on the type.
[...]
> +2009-12-21  Vladimir Prus  <vladimir@codesourcery.com>
> +
> +	PR gdb/10884
> +
> +	* gdb.mi/var-cmd.c (do_bitfield_tests): New
> +	(main): Call do_bitfield_tests.
> +	* gdb.mi/mi-cmd-var.exp: Run the 'bitfield' testcase.

FYI: I just checked this change in the 7.0 branch.

-- 
Joel

Re: RFA: unbreak typedefed bitfield

[Tom Tromey]

>>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:

Volodya> It seems to me that TYPE_LENGTH may return different values before and
Volodya> after check_typedef is called. Is the 'before' value ever or any use?

I don't think so.  My understanding is that before check_typedef is
called, TYPE_LENGTH is not guaranteed to be valid.

Volodya> If no, and as you say above in some cases we need to preserve
Volodya> some properties of the typedef, why TYPE_LENGTH could not check
Volodya> if the type is typedef, and if so, return length of the true
Volodya> type?

check_typedef is misnamed and is also used to resolve opaque types.
Other than that, I don't know of a reason.

You could try:

#define TYPE_LENGTH(thistype) check_typedef (thistype)->length

While this is probably insufficient to fix the check_typedef problem in
general, it may help with the most common source of problems.

Tom

Re: RFA: unbreak typedefed bitfield

[Daniel Jacobowitz]

On Mon, Dec 21, 2009 at 10:08:38AM -0700, Tom Tromey wrote:
> You could try:
> 
> #define TYPE_LENGTH(thistype) check_typedef (thistype)->length
> 
> While this is probably insufficient to fix the check_typedef problem in
> general, it may help with the most common source of problems.

      TYPE_LENGTH (type) = DW_UNSND (attr);

That's likely to cause some trouble.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: unbreak typedefed bitfield

[Vladimir Prus]

On Monday 21 December 2009 20:15:00 Daniel Jacobowitz wrote:

> On Mon, Dec 21, 2009 at 10:08:38AM -0700, Tom Tromey wrote:
> > You could try:
> > 
> > #define TYPE_LENGTH(thistype) check_typedef (thistype)->length
> > 
> > While this is probably insufficient to fix the check_typedef problem in
> > general, it may help with the most common source of problems.
> 
>       TYPE_LENGTH (type) = DW_UNSND (attr);
> 
> That's likely to cause some trouble.

IMO, this usage should just go, in preference to

	type_set_length (type, DW_UNSND (attr));

In fact, I think TYPE_LENGTH itself should go -- there's no reason to use a macro 
with all the nasty behaviours like above when inline functions would work just fine.

- Volodya

Re: RFA: unbreak typedefed bitfield

[Joel Brobecker]

> IMO, this usage should just go, in preference to
> 
> 	type_set_length (type, DW_UNSND (attr));

I tend to agree, but maybe for a different reason. I think we should
eventually try to make struct type and main_type opaque, which will
force to get rid of the macros and provide accessors instead.

-- 
Joel