diff mbox series

sparse attribute packed on structures

Message ID 0e8d816a-0849-c761-e0dc-93c3e5674e62@intel.com (mailing list archive)
State Mainlined, archived
Headers show
Series sparse attribute packed on structures | expand

Commit Message

Jacob Keller Dec. 15, 2020, 6:15 p.m. UTC
Hi,

I'm looking into an issue with sparse not calculating the size of a
packed structure correctly, causing some static assertions to fail due
to an incorrect size.

With a structure like this:

struct a {
	uint32_t a;
	uint8_t b;
	uint8_t c;
} __attribute__ ((packed));

The packed attribute doesn't seem to get applied to the whole structure.
Thus, the sparse sizeof evaluation for this results in 8 bytes (64
bits), when GCC would produce a structure of size 6 bytes (48 bits).

If I use something instead like this:

struct a {
	uint32_t a __attribute__ ((packed));
	uint8_t b __attribute__ ((packed));
	uint8_t c __attribute__ ((packed));
} __attribute__ ((packed));

Then the size is calculated correctly.

I saw that there is support in parse.c for parsing attribute packed, but
it doesn't seem to have a way to propagate from a structure down to its
members.

I thought it would be relatively straight forward to implement by adding
a MOD_PACKED, but that doesn't seem to actually get assigned to the
struct symbol, so when I tried that it didn't work.

I would very much like to help get structure size packing to work properly.

The following diff is what I tried initially, but it doesn't actually
work as expected. I'm not sure what is wrong, or what is the best method
to actually get the packed modifier to save into the structure symbol so
that it can be checked when determining the structure size.

Help would be appreciated.

Thanks,
Jake

---

Comments

Luc Van Oostenryck Dec. 15, 2020, 8:56 p.m. UTC | #1
On Tue, Dec 15, 2020 at 10:15:35AM -0800, Jacob Keller wrote:
> Hi,
> 
> I'm looking into an issue with sparse not calculating the size of a
> packed structure correctly, causing some static assertions to fail due
> to an incorrect size.
> 
> With a structure like this:
> 
> struct a {
> 	uint32_t a;
> 	uint8_t b;
> 	uint8_t c;
> } __attribute__ ((packed));
> 
> The packed attribute doesn't seem to get applied to the whole structure.
> Thus, the sparse sizeof evaluation for this results in 8 bytes (64
> bits), when GCC would produce a structure of size 6 bytes (48 bits).
> 
> If I use something instead like this:
> 
> struct a {
> 	uint32_t a __attribute__ ((packed));
> 	uint8_t b __attribute__ ((packed));
> 	uint8_t c __attribute__ ((packed));
> } __attribute__ ((packed));
> 
> Then the size is calculated correctly.
> 
> I saw that there is support in parse.c for parsing attribute packed, but
> it doesn't seem to have a way to propagate from a structure down to its
> members.
> 
> I thought it would be relatively straight forward to implement by adding
> a MOD_PACKED, but that doesn't seem to actually get assigned to the
> struct symbol, so when I tried that it didn't work.
> 
> I would very much like to help get structure size packing to work properly.
> 
> The following diff is what I tried initially, but it doesn't actually
> work as expected. I'm not sure what is wrong, or what is the best method
> to actually get the packed modifier to save into the structure symbol so
> that it can be checked when determining the structure size.
> 
> Help would be appreciated.

Hi,

There is at least 3 issues with the packed attribute:
1) at parsing time, types attributes are not applied to the
   corresponding symbol,
2) the size calculation must take the attribute in account,
3) the linearization of memory access must be adapted to be able
   to access unaligned members otherwise the check access complain
   loudly.

Sorry, I don't have much time for this now but at first sight your patch
seems on the right track. I can look at it more closely this WE but
meanwhile I've pushed a branch 'packed' on
	git://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git

This branch contains an unfinished patches but it should more or less
handle the points 1) & 2) and circumvent point 3) by disabling access
checking for bitfields.

I hope this will help you,
-- Luc
Jacob Keller Dec. 15, 2020, 10:17 p.m. UTC | #2
On 12/15/2020 12:56 PM, Luc Van Oostenryck wrote:
> On Tue, Dec 15, 2020 at 10:15:35AM -0800, Jacob Keller wrote:
>> Hi,
>>
>> I'm looking into an issue with sparse not calculating the size of a
>> packed structure correctly, causing some static assertions to fail due
>> to an incorrect size.
>>
>> With a structure like this:
>>
>> struct a {
>> 	uint32_t a;
>> 	uint8_t b;
>> 	uint8_t c;
>> } __attribute__ ((packed));
>>
>> The packed attribute doesn't seem to get applied to the whole structure.
>> Thus, the sparse sizeof evaluation for this results in 8 bytes (64
>> bits), when GCC would produce a structure of size 6 bytes (48 bits).
>>
>> If I use something instead like this:
>>
>> struct a {
>> 	uint32_t a __attribute__ ((packed));
>> 	uint8_t b __attribute__ ((packed));
>> 	uint8_t c __attribute__ ((packed));
>> } __attribute__ ((packed));
>>
>> Then the size is calculated correctly.
>>
>> I saw that there is support in parse.c for parsing attribute packed, but
>> it doesn't seem to have a way to propagate from a structure down to its
>> members.
>>
>> I thought it would be relatively straight forward to implement by adding
>> a MOD_PACKED, but that doesn't seem to actually get assigned to the
>> struct symbol, so when I tried that it didn't work.
>>
>> I would very much like to help get structure size packing to work properly.
>>
>> The following diff is what I tried initially, but it doesn't actually
>> work as expected. I'm not sure what is wrong, or what is the best method
>> to actually get the packed modifier to save into the structure symbol so
>> that it can be checked when determining the structure size.
>>
>> Help would be appreciated.
> 
> Hi,
> 
> There is at least 3 issues with the packed attribute:
> 1) at parsing time, types attributes are not applied to the
>    corresponding symbol,

Yea, this is what I couldn't figure out.

> 2) the size calculation must take the attribute in account,

This is what I got working IFF I hacked the symbol to say MOD_PACKED.
But I couldn't figure out (1) so I got stuck.

> 3) the linearization of memory access must be adapted to be able>    to access unaligned members otherwise the check access complain
>    loudly.
> 

Yea I figured something would break here, I was basically mostly
interested in handling the size.

> Sorry, I don't have much time for this now but at first sight your patch
> seems on the right track. I can look at it more closely this WE but
> meanwhile I've pushed a branch 'packed' on
> 	git://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git
> 
> This branch contains an unfinished patches but it should more or less
> handle the points 1) & 2) and circumvent point 3) by disabling access
> checking for bitfields.
> 

Ok thanks! I'll take a look at the packed branch, that should help me.

Perhaps I can take a stab at (3) based on whats in that branch.

> I hope this will help you,
> -- Luc
> 

Great, Thanks!
- Jake
Jacob Keller Dec. 15, 2020, 10:44 p.m. UTC | #3
On 12/15/2020 12:56 PM, Luc Van Oostenryck wrote:
> On Tue, Dec 15, 2020 at 10:15:35AM -0800, Jacob Keller wrote:
>> Hi,
> 
> Sorry, I don't have much time for this now but at first sight your patch
> seems on the right track. I can look at it more closely this WE but
> meanwhile I've pushed a branch 'packed' on
> 	git://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git
> 
> This branch contains an unfinished patches but it should more or less
> handle the points 1) & 2) and circumvent point 3) by disabling access
> checking for bitfields.
> 

This branch solved my problem, and does it in a much more elegant way
than I was.

It still needs work for the issue of access, but the rest of the changes
did look good to me.

> I hope this will help you,
> -- Luc
>
Jacob Keller Dec. 15, 2020, 11:15 p.m. UTC | #4
On 12/15/2020 12:56 PM, Luc Van Oostenryck wrote:
> On Tue, Dec 15, 2020 at 10:15:35AM -0800, Jacob Keller wrote:
>> Hi,
>>
>> I'm looking into an issue with sparse not calculating the size of a
>> packed structure correctly, causing some static assertions to fail due
>> to an incorrect size.
>>
>> With a structure like this:
>>
>> struct a {
>> 	uint32_t a;
>> 	uint8_t b;
>> 	uint8_t c;
>> } __attribute__ ((packed));
>>
>> The packed attribute doesn't seem to get applied to the whole structure.
>> Thus, the sparse sizeof evaluation for this results in 8 bytes (64
>> bits), when GCC would produce a structure of size 6 bytes (48 bits).
>>
>> If I use something instead like this:
>>
>> struct a {
>> 	uint32_t a __attribute__ ((packed));
>> 	uint8_t b __attribute__ ((packed));
>> 	uint8_t c __attribute__ ((packed));
>> } __attribute__ ((packed));
>>
>> Then the size is calculated correctly.
>>
>> I saw that there is support in parse.c for parsing attribute packed, but
>> it doesn't seem to have a way to propagate from a structure down to its
>> members.
>>
>> I thought it would be relatively straight forward to implement by adding
>> a MOD_PACKED, but that doesn't seem to actually get assigned to the
>> struct symbol, so when I tried that it didn't work.
>>
>> I would very much like to help get structure size packing to work properly.
>>
>> The following diff is what I tried initially, but it doesn't actually
>> work as expected. I'm not sure what is wrong, or what is the best method
>> to actually get the packed modifier to save into the structure symbol so
>> that it can be checked when determining the structure size.
>>
>> Help would be appreciated.
> 
> Hi,
> 
> There is at least 3 issues with the packed attribute:
> 1) at parsing time, types attributes are not applied to the
>    corresponding symbol,
> 2) the size calculation must take the attribute in account,
> 3) the linearization of memory access must be adapted to be able
>    to access unaligned members otherwise the check access complain
>    loudly.
> 
> Sorry, I don't have much time for this now but at first sight your patch
> seems on the right track. I can look at it more closely this WE but
> meanwhile I've pushed a branch 'packed' on
> 	git://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git
> 
> This branch contains an unfinished patches but it should more or less
> handle the points 1) & 2) and circumvent point 3) by disabling access
> checking for bitfields.
> 
> I hope this will help you,
> -- Luc
> 

I did find one bug, in your step (3), you have a check against
info->packed on symbol.c:162 in lay_out_struct, but nothing ever set the
packed value. I think you just need to initialized info->packed from
sym_packed at the top of examine_struct_union_type, i.e.


---
diff --git i/symbol.c w/symbol.c
index 4a654eea9cd0..5a2e0fcd1532 100644
--- i/symbol.c
+++ w/symbol.c
@@ -185,6 +185,8 @@ static struct symbol *
examine_struct_union_type(struct symbol *sym, int advance
        void (*fn)(struct symbol *, struct struct_union_info *);
        struct symbol *member;

+       info.packed = sym->packed;
+
        fn = advance ? lay_out_struct : lay_out_union;
        FOR_EACH_PTR(sym->symbol_list, member) {
                if (member->ctype.base_type == &autotype_ctype) {

---

Without this change, bitfield access checks aren't actually suppressed
properly.

Thanks,
Jake
Luc Van Oostenryck Dec. 16, 2020, 11:30 p.m. UTC | #5
On Tue, Dec 15, 2020 at 03:15:48PM -0800, Jacob Keller wrote:
> 
> I did find one bug, in your step (3), you have a check against
> info->packed on symbol.c:162 in lay_out_struct, but nothing ever set the
> packed value. I think you just need to initialized info->packed from
> sym_packed at the top of examine_struct_union_type, i.e.

A yes, I see, thank you. I think it was on purpose that it wasn't
yet enabled (things are it fuzzy because the code is ~2 year old)
and as I said it's unfinished.

But, with your change, does it handles 'packed' more or less
correctly?

-- Luc
Jacob Keller Dec. 17, 2020, 12:43 a.m. UTC | #6
On 12/16/2020 3:30 PM, Luc Van Oostenryck wrote:
> On Tue, Dec 15, 2020 at 03:15:48PM -0800, Jacob Keller wrote:
>>
>> I did find one bug, in your step (3), you have a check against
>> info->packed on symbol.c:162 in lay_out_struct, but nothing ever set the
>> packed value. I think you just need to initialized info->packed from
>> sym_packed at the top of examine_struct_union_type, i.e.
> 
> A yes, I see, thank you. I think it was on purpose that it wasn't
> yet enabled (things are it fuzzy because the code is ~2 year old)
> and as I said it's unfinished.
> 
> But, with your change, does it handles 'packed' more or less
> correctly?
> 
> -- Luc
> 

Yes. Obviously we're limited in that we no longer check for
out-of-bounds accesses on bitfields, but it at least produces the
correct sizes for structures, and avoids the warnings that I was running
into.

Overall, I think the changes in that branch are solid, and look correct
to me.

I'm not sure what all the limitations are of having it produce incorrect
load/store operations that don't work with the packed bitfields.. but at
least for the code I was checking, it seems to be correct now.

Thanks,
Jake
Luc Van Oostenryck Dec. 17, 2020, 12:57 a.m. UTC | #7
On Wed, Dec 16, 2020 at 04:43:24PM -0800, Jacob Keller wrote:
> On 12/16/2020 3:30 PM, Luc Van Oostenryck wrote:
> > On Tue, Dec 15, 2020 at 03:15:48PM -0800, Jacob Keller wrote:
> >>
> >> I did find one bug, in your step (3), you have a check against
> >> info->packed on symbol.c:162 in lay_out_struct, but nothing ever set the
> >> packed value. I think you just need to initialized info->packed from
> >> sym_packed at the top of examine_struct_union_type, i.e.
> > 
> > A yes, I see, thank you. I think it was on purpose that it wasn't
> > yet enabled (things are it fuzzy because the code is ~2 year old)
> > and as I said it's unfinished.
> > 
> > But, with your change, does it handles 'packed' more or less
> > correctly?
> > 
> > -- Luc
> > 
> 
> Yes. Obviously we're limited in that we no longer check for
> out-of-bounds accesses on bitfields, but it at least produces the
> correct sizes for structures, and avoids the warnings that I was running
> into.

Nice, thanks for confirming this.
 
> Overall, I think the changes in that branch are solid, and look correct
> to me.
> 
> I'm not sure what all the limitations are of having it produce incorrect
> load/store operations that don't work with the packed bitfields.. but at
> least for the code I was checking, it seems to be correct now.

Oh, it's not much, just the usual. Currently, sparse always access a
bitfield with its base type. So, if we have the following:
	struct foo {
		int a;
		int b:24;
	} __packed;
	struct foo s;

Then accessing s.b will first load the full word and then mask the
upper bits, but:
1) this will access bytes 5,6,7 & 8 but the structure is only 7 bytes
   long and so the access checking should fail.
2) accessing byte-by-byte and then assemble the result is endian
   specific and until now sparse had succeed o ignore this kind
   of 'details'.

-- Luc
diff mbox series

Patch

diff --git i/parse.c w/parse.c
index 0b2685707b82..96beae80c300 100644
--- i/parse.c
+++ w/parse.c
@@ -1086,13 +1086,6 @@  static struct token *ignore_attribute(struct
token *token, struct symbol *attr,
        return token;
 }

-static struct token *attribute_packed(struct token *token, struct
symbol *attr, struct decl_state *ctx)
-{
-       if (!ctx->ctype.alignment)
-               ctx->ctype.alignment = 1;
-       return token;
-}
-
 static struct token *attribute_aligned(struct token *token, struct
symbol *attr, struct decl_state *ctx)
 {
        int alignment = max_alignment;
@@ -1142,6 +1135,14 @@  static struct token *attribute_bitwise(struct
token *token, struct symbol *attr,
        return token;
 }

+static struct token *attribute_packed(struct token *token, struct
symbol *attr, struct decl_state *ctx)
+{
+       apply_mod(&token->pos, &ctx->ctype.modifiers, MOD_PACKED);
+       if (!ctx->ctype.alignment)
+               ctx->ctype.alignment = 1;
+       return token;
+}
+
 static struct ident *numerical_address_space(int asn)
 {
        char buff[32];
diff --git i/symbol.c w/symbol.c
index 1a083fb8432c..6e0d3ba74bd6 100644
--- i/symbol.c
+++ w/symbol.c
@@ -88,6 +88,7 @@  struct struct_union_info {
        unsigned long bit_size;
        int align_size;
        char has_flex_array;
+       char is_packed;
        struct symbol *flex_array;
 };

@@ -136,7 +137,10 @@  static void lay_out_struct(struct symbol *sym,
struct struct_union_info *info)
                info->flex_array = sym;
        }

-       align_bit_mask = bytes_to_bits(sym->ctype.alignment) - 1;
+       if (info->is_packed)
+               align_bit_mask = bytes_to_bits(1) - 1;
+       else
+               align_bit_mask = bytes_to_bits(sym->ctype.alignment) - 1;

        /*
         * Bitfields have some very special rules..
@@ -181,6 +185,8 @@  static struct symbol *
examine_struct_union_type(struct symbol *sym, int advance
        void (*fn)(struct symbol *, struct struct_union_info *);
        struct symbol *member;

+       info.is_packed = sym->ctype.modifiers & MOD_PACKED ? 1 : 0;
+
        fn = advance ? lay_out_struct : lay_out_union;
        FOR_EACH_PTR(sym->symbol_list, member) {
                if (member->ctype.base_type == &autotype_ctype) {
diff --git i/symbol.h w/symbol.h
index 5c5a7f12affa..172b70257d20 100644
--- i/symbol.h
+++ w/symbol.h
@@ -242,7 +242,7 @@  struct symbol {

 #define MOD_GNU_INLINE         0x00010000
 #define MOD_USERTYPE           0x00020000
-     // MOD UNUSED             0x00040000
+#define MOD_PACKED             0x00040000
      // MOD UNUSED             0x00080000
      // MOD UNUSED             0x00100000
      // MOD UNUSED             0x00200000