[RFC] (unpack_value_bits_as_long_1): Fix handling of bitsize == 0.

[dje at google dot com (Doug Evans)]

Hi.

I found this while playing with the Go port.

>From gdbtypes.h:

      /* Size of this field, in bits, or zero if not packed.
	 If non-zero in an array type, indicates the element size in
	 bits (used only in Ada at the moment).
	 For an unpacked field, the field's type's length
	 says how many bytes the field occupies.  */

      unsigned int bitsize : 29;

The code in value.c:unpack_value_bits_as_long_1 has an explicit check
for (bitsize > 0) in a couple of places but it doesn't check for whether
bitsize == 0 here:

value.c:unpack_value_bits_as_long_1:

  if (gdbarch_bits_big_endian (get_type_arch (field_type)))
    lsbcount = (bytes_read * 8 - bitpos % 8 - bitsize);
  else
    lsbcount = (bitpos % 8);

So it seems like there is at least one bug here.
The problem is that there aren't enough comments to know
what the bug is exactly.

E.g., Why the test for (bitsize > 0) here:

  /* If the field does not entirely fill a LONGEST, then zero the sign bits.
     If the field is signed, and is negative, then sign extend.  */

  if ((bitsize > 0) && (bitsize < 8 * (int) sizeof (val)))

The comment says nothing about why the (bitsize > 0) test is there.
[This line has been there since cvs revision 1.1 btw.
I didn't try to dig any deeper.]

This patch has no regressions on amd64-linux though one should
obviously test it on at least a big endian system.
I have no urgent need for this, so I don't plan to do such testing myself,
at least in the near future.

Comments?
[Is there a comment explaining why things are the way they are
hidden in some obscure location?]

2012-01-25  Doug Evans  <dje@google.com>

	* value.c (unpack_value_bits_as_long_1): Fix handling of bitsize == 0.
	(unpack_value_bits_as_long): Document what bitsize == 0 means.

Index: value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.150
diff -u -p -r1.150 value.c
--- value.c	4 Jan 2012 08:27:58 -0000	1.150
+++ value.c	25 Jan 2012 22:17:35 -0000
@@ -2688,10 +2688,15 @@ unpack_value_bits_as_long_1 (struct type
   /* Read the minimum number of bytes required; there may not be
      enough bytes to read an entire ULONGEST.  */
   CHECK_TYPEDEF (field_type);
-  if (bitsize)
-    bytes_read = ((bitpos % 8) + bitsize + 7) / 8;
-  else
-    bytes_read = TYPE_LENGTH (field_type);
+  if (bitsize == 0)
+    {
+      bitsize = TYPE_LENGTH (field_type) * 8;
+      /* Bad debug info could leak through, though it's the job of the
+	 debug info readers to guarantee this doesn't happen.
+	 This is commented out as I don't know if that's the case.  */
+      /*gdb_assert (bitpos % 8) == 0;*/
+    }
+  bytes_read = ((bitpos % 8) + bitsize + 7) / 8;
 
   read_offset = bitpos / 8;
 
@@ -2714,7 +2719,7 @@ unpack_value_bits_as_long_1 (struct type
   /* If the field does not entirely fill a LONGEST, then zero the sign bits.
      If the field is signed, and is negative, then sign extend.  */
 
-  if ((bitsize > 0) && (bitsize < 8 * (int) sizeof (val)))
+  if (bitsize < 8 * (int) sizeof (val))
     {
       valmask = (((ULONGEST) 1) << bitsize) - 1;
       val &= valmask;
@@ -2735,7 +2740,9 @@ unpack_value_bits_as_long_1 (struct type
    VALADDR + EMBEDDED_OFFSET, and store the result in *RESULT.
    VALADDR points to the contents of ORIGINAL_VALUE, which must not be
    NULL.  The bitfield starts at BITPOS bits and contains BITSIZE
-   bits.
+   bits.  If BITSIZE is zero, use the length of FIELD_TYPE (this is used
+   to mark "unpacked" fields, see gdbtypes.h,
+   struct main_type.field.field_location.bitsize).
 
    Returns false if the value contents are unavailable, otherwise
    returns true, indicating a valid value has been stored in *RESULT.