Re: sparse attribute packed on structures

[Jacob Keller <jacob.e.keller@intel.com>]

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