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