[PATCH] append_composite_type_field_aligned

[PATCH] append_composite_type_field_aligned

[Andrew Burgess]

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;
 		}
 	    }
 	}

Re: [PATCH] append_composite_type_field_aligned

[Andrew Burgess]

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;
>   		}
>   	    }
>   	}
>
>
>

Re: [PATCH] append_composite_type_field_aligned

[Yao Qi]

On 06/21/2011 05:47 PM, Andrew Burgess wrote:
> ping.
> 
> If there's anything else I should do to help progress this then please
> let me know.
> 

I can't see there is anything else you should do.  Just wait for
maintainers to review this patch.

>> 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;
>>           }
>>           }
>>       } 

I am not the people to approve this.  I read your patch, and it looks
correct.

-- 
Yao (齐尧)

Re: [PATCH] append_composite_type_field_aligned

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@broadcom.com> writes:

I'm sorry about the delay on this.

Pinging was the right thing to do -- thank you.

Andrew> 2011-06-09  Andrew Burgess  <aburgess@broadcom.com>
Andrew> 	* gdbtypes.c (append_composite_type_field_aligned): Fix
Andrew>           calculation of bit position based on alignment.

Ok with one nit..

Andrew> +	      int left;
Andrew> +	      alignment *= TARGET_CHAR_BIT;

Blank line between declarations and code.

Tom

Re: [PATCH] append_composite_type_field_aligned

[Andrew Burgess]

On 22/06/2011 20:54, Tom Tromey wrote:
>
> Ok with one nit..
>
> Andrew>  +	      int left;
> Andrew>  +	      alignment *= TARGET_CHAR_BIT;
>
> Blank line between declarations and code.
>

Committed with the extra blank line added.

Thanks,
Andrew