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