Re: [RFA] Setting long long bitfields

[Andrew Cagney <cagney at gnu dot org>]

Paul Hilfinger wrote:
> The patch below fixes a small problem with assignments in GDB to 
> long long bitfields (which, while not a standard part of C are provided 
> by GCC).  The existing code computes the correct mask for zeroing out
> the old value, but uses int computations in the code that truncates 
> the field value and tests whether it fits.  Bit-fields that don't fit
> in an int are rare in C code, but they do crop up more in Ada, which
> allows considerably more aggressive packing.  I have included a draft of 
> a new set of testsuite files (in C) that tickle the problem.

Looks under patch, nope no testcase, looking forward to it :-)

Yes, ok.

One possible tweak, several of us have an aversion to "?:", it would be 
nice if it wasn't there :-)

Andrew


> Paul Hilfinger
>
> 2004-10-31  Paul N. Hilfinger  <Hilfinger@gnat.com>
>
> 	* values.c (modify_field): Correct handling of bit-fields that
> 	don't fit in 32 bits.  Use unsigned operations consistently and 
> 	simplify the code a bit.
>
> Index: gdb/values.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/values.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 values.c
> --- gdb/values.c	28 Jul 2004 02:46:24 -0000	1.71
> +++ gdb/values.c	31 Oct 2004 08:26:17 -0000
> @@ -1075,40 +1075,36 @@ unpack_field_as_long (struct type *type,
>  void
>  modify_field (char *addr, LONGEST fieldval, int bitpos, int bitsize)
>  {
> -  LONGEST oword;
> +  ULONGEST oword;
> +  ULONGEST mask = (bitsize == 8 * (int) sizeof (ULONGEST)
> +		   ? ~(ULONGEST) 0 : ((ULONGEST) 1 << bitsize) - 1);
>  
>    /* If a negative fieldval fits in the field in question, chop
>       off the sign extension bits.  */
> -  if (bitsize < (8 * (int) sizeof (fieldval))
> -      && (~fieldval & ~((1 << (bitsize - 1)) - 1)) == 0)
> -    fieldval = fieldval & ((1 << bitsize) - 1);
> +  if ((~fieldval & ~(mask >> 1)) == 0)
> +    fieldval &= mask;
>  
>    /* Warn if value is too big to fit in the field in question.  */
> -  if (bitsize < (8 * (int) sizeof (fieldval))
> -      && 0 != (fieldval & ~((1 << bitsize) - 1)))
> +  if (0 != (fieldval & ~mask))
>      {
>        /* FIXME: would like to include fieldval in the message, but
>           we don't have a sprintf_longest.  */
>        warning ("Value does not fit in %d bits.", bitsize);
>  
>        /* Truncate it, otherwise adjoining fields may be corrupted.  */
> -      fieldval = fieldval & ((1 << bitsize) - 1);
> +      fieldval &= mask;
>      }
>  
> -  oword = extract_signed_integer (addr, sizeof oword);
> +  oword = extract_unsigned_integer (addr, sizeof oword);
>  
>    /* Shifting for bit field depends on endianness of the target machine.  */
>    if (BITS_BIG_ENDIAN)
>      bitpos = sizeof (oword) * 8 - bitpos - bitsize;
>  
> -  /* Mask out old value, while avoiding shifts >= size of oword */
> -  if (bitsize < 8 * (int) sizeof (oword))
> -    oword &= ~(((((ULONGEST) 1) << bitsize) - 1) << bitpos);
> -  else
> -    oword &= ~((~(ULONGEST) 0) << bitpos);
> +  oword &= ~(mask << bitpos);
>    oword |= fieldval << bitpos;
>  
> -  store_signed_integer (addr, sizeof oword, oword);
> +  store_unsigned_integer (addr, sizeof oword, oword);
>  }
>  
>  /* Convert C numbers into newly allocated values */
> Index: current-public.116/gdb/testsuite/gdb.base/bitfields2.exp
> --- current-public.116/gdb/testsuite/gdb.base/bitfields2.exp Sun, 31 Oct 2004 01:11:22 -0700 hilfingr ()
> +++ merge.238(w)/gdb/testsuite/gdb.base/bitfields2.exp Sun, 31 Oct 2004 00:51:17 -0700 hilfingr (GdbPub/q/c/36_bitfields2 1.1 644)
> @@ -0,0 +1,325 @@
> +# Copyright 1992, 1994, 1995, 1997, 2004 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +# 
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +# 
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
> +
> +# Please email any bugs, comments, and/or additions to this file to:
> +# bug-gdb@prep.ai.mit.edu
> +
> +# This file was adapted from bitfields.exp by Paul Hilfinger 
> +# (Hilfinger@gnat.com)
> +
> +#
> +# Tests for bit-fields that do not fit in type (unsigned) int, but do fit 
> +# in type (unsigned) long long.  We perform essentially the same tests as
> +# in bitfields.c, which considers only bit-fields that are <= 9 bits long.
> +#
> +
> +if $tracelevel then {
> +	strace $tracelevel
> +}
> +
> +set prms_id 0
> +set bug_id 0
> +
> +set testfile "bitfields2"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +    gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
> +}
> +
> +set no_signed 0
> +
> +#
> +# Test bitfield locating and uniqueness.
> +# For each member, set that member to 1 and verify that the member (and only
> +# that member) is 1, then reset it back to 0.
> +#
> +
> +proc bitfield_uniqueness {} {
> +    global decimal
> +    global hex
> +    global gdb_prompt
> +    global srcfile
> +
> +    if { ! [runto break1] } {
> +	gdb_suppress_tests;
> +    }
> +	
> +    if [gdb_test "print flags" ".*u1 = 0, u2 = 0, u3 = 0, s1 = 1, s2 = 0, s3 = 0.*" "bitfield uniqueness (s1)"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "cont" "Break.*break1 \\(\\) at .*$srcfile:$decimal.*" "continuing to break1 #1"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "print flags" ".*u1 = 1, u2 = 0, u3 = 0, s1 = 0, s2 = 0, s3 = 0.*" "bitfield uniqueness (u1)"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "cont" "Break.*break1 \\(\\) at .*$srcfile:$decimal.*" "continuing to break1 #2"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "print flags" ".*u1 = 0, u2 = 0, u3 = 0, s1 = 0, s2 = 1, s3 = 0.*" "bitfield uniqueness (s2)"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "cont" "Break.*break1 \\(\\) at .*$srcfile:$decimal.*" "continuing to break1 #3"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "print flags" ".*u1 = 0, u2 = 1, u3 = 0, s1 = 0, s2 = 0, s3 = 0.*" "bitfield uniqueness (u2)"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "cont" "Break.*break1 \\(\\) at .*$srcfile:$decimal.*" "continuing to break1 #4"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "print flags" ".*u1 = 0, u2 = 0, u3 = 0, s1 = 0, s2 = 0, s3 = 1.*" "bitfield uniqueness (s3)"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "cont" "Break.*break1 \\(\\) at .*$srcfile:$decimal.*" "continuing to break1 #5"] {
> +	gdb_suppress_tests;
> +    }
> +    if [gdb_test "print flags" ".*u1 = 0, u2 = 0, u3 = 1, s1 = 0, s2 = 0, s3 = 0.*" "bitfield uniqueness (u3)"] {
> +	gdb_suppress_tests
> +    }
> +    gdb_stop_suppressing_tests;
> +}
> +
> +
> +#
> +# Test bitfield containment.
> +# Fill alternating fields with all 1's and verify that none of the bits
> +# "bleed over" to the other fields.
> +#
> +
> +proc bitfield_containment {} {
> +    global decimal
> +    global hex
> +    global gdb_prompt
> +    global srcfile
> +
> +    delete_breakpoints
> +
> +    if { ![runto break2] } {
> +	gdb_suppress_tests
> +    }
> +
> +    # If program is compiled with Sun CC, signed fields print out as their
> +    # actual sizes; if compiled with gcc, they print out as 0xffffffff.
> +    if [gdb_test "print/x flags" "= {u1 = 0x7fff, u2 = 0x0, u3 = 0xffff, s1 = 0x0, s2 = 0x(1ffffffff|f*), s3 = 0x0}" "bitfield containment #1"] {
> +	gdb_suppress_tests
> +    }
> +
> +    if [gdb_test "cont" "Break.*break2 \\(\\) at .*$srcfile:$decimal.*" "continuing to break2"] {
> +	gdb_suppress_tests
> +    }
> +
> +    if [gdb_test "print/x flags" "= {u1 = 0x0, u2 = 0x1ffffffff, u3 = 0x0, s1 = 0x(7fff|f*), s2 = 0x0, s3 = 0xf*}" "bitfield containment #2"] {
> +	gdb_suppress_tests
> +    }
> +    gdb_stop_suppressing_tests;
> +}
> +
> +# Test unsigned bitfields for unsignedness and range.
> +# Fill the unsigned fields with the maximum positive value and verify that
> +# the values are printed correctly.
> +
> +proc bitfield_unsignedness {} {
> +    global decimal
> +    global hex
> +    global gdb_prompt
> +    global srcfile
> +
> +    delete_breakpoints
> +
> +    if { ![runto break3] } {
> +	gdb_suppress_tests
> +    }
> +
> +    if [gdb_test "print flags" ".*u1 = 32767, u2 = 8589934591, u3 = 65535, s1 = 0, s2 = 0, s3 = 0.*" "unsigned bitfield ranges"] {
> +	gdb_suppress_tests
> +    }
> +    gdb_stop_suppressing_tests;
> +}
> +
> +#
> +# Test signed bitfields for signedness and range.
> +# Fill the signed fields with the maximum positive value, then the maximally
> +# negative value, then -1, and verify in each case that the values are
> +# printed correctly.
> +#
> +
> +proc bitfield_signedness {} {
> +    global decimal
> +    global hex
> +    global gdb_prompt
> +    global srcfile
> +    global no_signed
> +
> +    delete_breakpoints
> +
> +    if { ! [runto break4] } {
> +	gdb_suppress_tests
> +    }
> +
> +    if [gdb_test "print flags" "= {.*u1 = 0, u2 = 0, u3 = 0, s1 = 16383, s2 = 4294967295, s3 = 32767.*}" "signed bitfields, max positive values"] {
> +	gdb_suppress_tests
> +    }
> +
> +    if [gdb_test "cont" "Break.*break4 \\(\\) at .*$srcfile:$decimal.*" "continuing to break4 #1"] {
> +	gdb_suppress_tests
> +    }
> +
> +    # Determine if the target has signed bitfields so we can xfail the
> +    # the signed bitfield tests if it doesn't.
> +    set no_signed 1
> +    send_gdb "print i\n"
> +    gdb_expect {
> +	-re ".* = -32768.*$gdb_prompt $" {
> +	    set no_signed 0
> +	    pass "determining signed-ness of bitfields"
> +	}
> +	-re ".* = 32768.*$gdb_prompt $" {
> +	    pass "determining signed-ness of bitfields"
> +	    setup_xfail "*-*-*"
> +	}
> +	-re ".*$gdb_prompt $" {
> +	    fail "determining signed-ness of bitfields"
> +	    gdb_suppress_tests
> +	}
> +	default { 
> +	    fail "determining signed-ness of bitfields" ;
> +	    gdb_suppress_tests;
> +	}
> +    }
> +
> +    if [gdb_test "print flags" "u1 = 0, u2 = 0, u3 = 0, s1 = -16384, s2 = -4294967296, s3 = -32768.*" "signed bitfields, max negative values"] {
> +        gdb_suppress_tests
> +    }
> +
> +    if [gdb_test "cont" "Break.*break4 \\(\\) at .*$srcfile:$decimal.*" "continuing to break4 #2"] {
> +	gdb_suppress_tests
> +    }
> +
> +    if [gdb_test "print flags" "u1 = 0, u2 = 0, u3 = 0, s1 = -1, s2 = -1, s3 = -1.*" "signed bitfields with -1"] {
> +	gdb_suppress_tests
> +    }
> +    # Hmmmm???
> +    gdb_stop_suppressing_tests;
> +}
> +
> +
> +# Test setting of long long bit fields from within GDB.
> +
> +proc bitfield_set {} {
> +    global decimal
> +    global hex
> +    global gdb_prompt
> +    global srcfile
> +    global no_signed
> +
> +    delete_breakpoints
> +
> +    if { ! [runto break5] } {
> +	gdb_suppress_tests
> +    }
> +
> +    set big_set_failed 0
> +    set test "set long long unsigned bitfield"
> +    gdb_test_multiple "print flags.u2 = 0x100000000" $test {
> +	-re "warning: Value does not fit.*$gdb_prompt $" {
> +	    fail "$test"
> +	    gdb_suppress_tests
> +	}
> +	-re "= 4294967296.*$gdb_prompt $" {
> +	    pass "$test"
> +	}
> +    }
> +
> +    set test "set long long signed bitfield positive"
> +    gdb_test_multiple "print flags.s2 = 0x80000000" $test {
> +	-re "warning: Value does not fit.*$gdb_prompt $" {
> +	    fail "$test"
> +	    gdb_suppress_tests
> +	}
> +	-re "= 2147483648.*$gdb_prompt $" {
> +	    pass "$test"
> +	}
> +    }
> +
> +    if [gdb_test "print flags" "u1 = 0, u2 = 4294967296, u3 = 0, s1 = 0, s2 = 2147483648, s3 = 0.*" "long long bitfield values after set"] {
> +	gdb_suppress_tests
> +    }
> +
> +    if $no_signed then {
> +	setup_xfail "*-*-*"
> +    }
> +    set test "set long long signed bitfield negative"
> +    gdb_test_multiple "print flags.s2 = -1" $test {
> +	-re "warning: Value does not fit.*$gdb_prompt $" {
> +	    fail "$test"
> +	    gdb_suppress_tests
> +	}
> +	-re "= -1.*$gdb_prompt $" {
> +	    pass "$test"
> +	}
> +    }
> +
> +    if $no_signed then {
> +	setup_xfail "*-*-*"
> +    }
> +    if [gdb_test "print flags" "u1 = 0, u2 = 4294967296, u3 = 0, s1 = 0, s2 = -1, s3 = 0.*" "long long bitfield values after set negative"] {
> +	gdb_suppress_tests
> +    }
> +    gdb_stop_suppressing_tests;
> +}
> +
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}
> +
> +send_gdb "set print sevenbit-strings\n" ; gdb_expect -re "$gdb_prompt $"
> +
> +bitfield_uniqueness
> +if [istarget "mips-idt-*"] then {
> +    # Restart because IDT/SIM runs out of file descriptors.
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load ${binfile}
> +}
> +bitfield_containment
> +if [istarget "mips-idt-*"] then {
> +    # Restart because IDT/SIM runs out of file descriptors.
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load ${binfile}
> +}
> +bitfield_unsignedness
> +if [istarget "mips-idt-*"] then {
> +    # Restart because IDT/SIM runs out of file descriptors.
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load ${binfile}
> +}
> +bitfield_signedness
> +if [istarget "mips-idt-*"] then {
> +    # Restart because IDT/SIM runs out of file descriptors.
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load ${binfile}
> +}
> +bitfield_set
> +
> Index: current-public.116/gdb/testsuite/gdb.base/bitfields2.c
> --- current-public.116/gdb/testsuite/gdb.base/bitfields2.c Sun, 31 Oct 2004 01:11:22 -0700 hilfingr ()
> +++ merge.238(w)/gdb/testsuite/gdb.base/bitfields2.c Sun, 31 Oct 2004 00:51:17 -0700 hilfingr (GdbPub/q/c/37_bitfields2 1.1 644)
> @@ -0,0 +1,162 @@
> +/* Test program to test bit field operations on bit fields of large
> +   integer types.  */
> +
> +/* This file is expected to fail to compile if the type long long int
> +   is not supported, but in that case it is irrelevant.  */
> +
> +#if !defined(__STDC__) && !defined(__cplusplus)
> +#define signed  /**/
> +#endif
> +
> +struct fields
> +{
> +  unsigned long long u1 : 15;
> +  unsigned long long u2 : 33;
> +  unsigned long long u3 : 16;
> +  signed long long   s1 : 15;
> +  signed long long   s2 : 33;
> +  signed long long   s3 : 16;
> +} flags;
> +
> +void break1 ()
> +{
> +}
> +
> +void break2 ()
> +{
> +}
> +
> +void break3 ()
> +{
> +}
> +
> +void break4 ()
> +{
> +}
> +
> +void break5 ()
> +{
> +}
> +
> +void break6 ()
> +{
> +}
> +
> +void break7 ()
> +{
> +}
> +
> +void break8 ()
> +{
> +}
> +
> +void break9 ()
> +{
> +}
> +
> +void break10 ()
> +{
> +}
> +
> +/* This is used by bitfields.exp to determine if the target understands
> +   signed bitfields.  */
> +int i;
> +
> +int main ()
> +{
> +  /* For each member, set that member to 1, allow gdb to verify that the
> +     member (and only that member) is 1, and then reset it back to 0. */
> +
> +#ifdef usestubs
> +  set_debug_traps();
> +  breakpoint();
> +#endif
> +  flags.s1 = 1;
> +  break1 ();
> +  flags.s1 = 0;
> +
> +  flags.u1 = 1;
> +  break1 ();
> +  flags.u1 = 0;
> +
> +  flags.s2  = 1;
> +  break1 ();
> +  flags.s2 = 0;
> +
> +  flags.u2 = 1;
> +  break1 ();
> +  flags.u2 = 0;
> +
> +  flags.s3  = 1;
> +  break1 ();
> +  flags.s3 = 0;
> +
> +  flags.u3 = 1;
> +  break1 ();
> +  flags.u3 = 0;
> +
> +  /* Fill alternating fields with all 1's and verify that none of the bits
> +     "bleed over" to the other fields. */
> +
> +  flags.u1 = 0x7FFF;
> +  flags.u3 = 0xFFFF;
> +  flags.s2 = -1LL;
> +  break2 ();
> +  flags.u1 = 0;
> +  flags.u3 = 0;
> +  flags.s2 = 0;
> +
> +  flags.u2 = 0x1FFFFFFFFLL;
> +  flags.s1 = -1;
> +  flags.s3 = -1;
> +  break2 ();
> +
> +  flags.u2 = 0;
> +  flags.s1 = 0;
> +  flags.s3 = 0;
> +
> +  /* Fill the unsigned fields with the maximum positive value and verify
> +     that the values are printed correctly. */
> +
> +  flags.u1 = 0x7FFF;
> +  flags.u2 = 0x1FFFFFFFFLL;
> +  flags.u3 = 0xFFFF;
> +  break3 ();
> +  flags.u1 = 0;
> +  flags.u2 = 0;
> +  flags.u3 = 0;
> +
> +  /* Fill the signed fields with the maximum positive value, then the maximally
> +     negative value, then -1, and verify in each case that the values are
> +     printed correctly. */
> +
> +  /* Maximum positive values */
> +  flags.s1 = 0x3FFF;
> +  flags.s2 = 0xFFFFFFFFLL;
> +  flags.s3 = 0x7FFF;
> +  break4 ();
> +
> +  /* Maximally negative values */
> +  flags.s1 = -0x4000;
> +  flags.s2 = -0x100000000LL;
> +  flags.s3 = -0x8000;
> +
> +  /* Extract bitfield value so that bitfield.exp can check if the target
> +     understands signed bitfields.  */
> +  i = flags.s3;
> +  break4 ();
> +
> +  /* -1 */
> +  flags.s1 = -1;
> +  flags.s2 = -1;
> +  flags.s3 = -1;
> +  break4 ();
> +
> +  flags.s1 = 0;
> +  flags.s2 = 0;
> +  flags.s3 = 0;
> +
> +  break5 ();
> +
> +  return 0;
> +}