Re: [PATCH] append_composite_type_field_aligned

["Andrew Burgess" <aburgess@broadcom.com>]

ping.

If there's anything else I should do to help progress this then please 
let me know.

Thanks,

Andrew


On 09/06/2011 16:35, Andrew Burgess wrote:
> I've been trying to use the function
> append_composite_type_field_aligned from gdbtypes.c and I was not
> seeing the behaviour I was expecting.
>
> Please excuse the rather long/rambling email, but I've tried to lay
> out below the behaviour I was seeing and why this is not what I was
> expecting then someone can jump in if I've made a mistake.
>
> I have no reproducible code for this it's all just code inspection
> of the function append_composite_type_field_aligned in gdbtypes.c .
>
> I'm working with TARGET_CHAR_BIT = 8 throughout, though the values I
> calculate would obviously change I don't think it makes any other
> difference to the point I'm making.
>
> Consider creation of a composite type "CT" with type code TYPE_CODE_STRUCT.
>
> I add an initial component type "T1" of size "S1" which will be put
> at the start of CT (bit position 0).
>
> I then add another component type "T2" of size "S2".
>
> I add T1 using something like this:
>
>     append_composite_type_field_aligned (CT, "T1", T1, 0);
>
> I then add T2 using different alignment values (A) like this:
>
>     append_composite_type_field_aligned (CT, "T2", T2, A);
>
> I vary the value of A to be 0, 8, 16, 24, 32, 64 and vary the size S1
> and calculate the FIELD_BITPOS at which T2 will be placed.
>
> The FIELD_BITPOS for T1 will be 0 in all cases. The FIELD_BITPOS for
> T2 will depend on the TYPE_LENGTH of T1 and the alignment value A.
>
> I've included an example where S1 is 0, a little contrived maybe, but
> it fills out an the table, and would be of interest if we added the
> first type to the structure with a non-zero alignment (which should be
> fine.)
>
> | TYPE   |   FIELD BITPOS T2 for different alignment A   |
> | LENGTH |                                               |
> |   T1   | A == 0 | A == 8 | A == 16 | A == 24 | A == 32 |
> |--------|--------|--------|---------|---------|---------|
> | 0      | 0      | 0      | 0       | 0       | 0       |
> | 8      | 8      | 8      | 16      | 16      | 16      |
> | 16     | 16     | 16     | 16      | 32      | 32      |
> | 24     | 24     | 24     | 32      | 24      | 48      |
> | 32     | 32     | 32     | 32      | 40      | 32      |
>
>
> My belief was that:
>
>     ( FIELD_BITPOS(T2) % A ) == 0
>
> after the alignment adjustment has taken place. This is obviously not
> the case for (A == 24) and (A == 32) and the bit position has even
> gone backwards in some cases.
>
> I've included a patch below which changes the behaviour to match my
> expectations, with the patch applied the table now looks like this:
>
> | TYPE   |   FIELD BITPOS T2 for different alignment A   |
> | LENGTH |                                               |
> |   T1   | A == 0 | A == 8 | A == 16 | A == 24 | A == 32 |
> |--------|--------|--------|---------|---------|---------|
> | 0      | 0      | 0      | 0       | 0       | 0       |
> | 8      | 8      | 8      | 16      | 24      | 32      |
> | 16     | 16     | 16     | 16      | 24      | 32      |
> | 24     | 24     | 24     | 32      | 24      | 32      |
> | 32     | 32     | 32     | 32      | 48      | 32      |
>
> These values seem to make more sense to me, hopefully you'll all
> agree, though if I've misunderstood something or misread the code
> in please could someone point out what I've missed.
>
> If this looks good then am I OK to apply the patch?
>
> Thanks,
> Andrew
>
>
> gdb/ChangeLog
>
> 2011-06-09  Andrew Burgess<aburgess@broadcom.com>
>
> 	* gdbtypes.c (append_composite_type_field_aligned): Fix
>            calculation of bit position based on alignment.
>
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 2bdb4eb..ba957f9 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -3654,12 +3654,14 @@ append_composite_type_field_aligned (struct type *t, char *name,
>
>   	  if (alignment)
>   	    {
> -	      int left = FIELD_BITPOS (f[0]) % (alignment * TARGET_CHAR_BIT);
> +	      int left;
> +	      alignment *= TARGET_CHAR_BIT;
> +	      left = FIELD_BITPOS (f[0]) % alignment;
>
>   	      if (left)
>   		{
> -		  FIELD_BITPOS (f[0]) += left;
> -		  TYPE_LENGTH (t) += left / TARGET_CHAR_BIT;
> +		  FIELD_BITPOS (f[0]) += (alignment - left);
> +		  TYPE_LENGTH (t) += (alignment - left) / TARGET_CHAR_BIT;
>   		}
>   	    }
>   	}
>
>
>