* Julius Werner jwerner@chromium.org [160727 23:41]:
typedef struct dmar_atsr_entry { u16 type; u16 length; u8 flags; u8 reserved; u16 segment; } __attribute__ ((packed)) dmar_atsr_entry_t;
Looking at the original example here, I would still recommend not to use the packed attribute. It forces the compiler to use accesses that would work on unaligned data... for x86 that doesn't matter (and granted, this sounds x86-specific, but it's worth applying the same principles to all code), but for other architectures it may generate very inefficient code (even on ARM where unaligned accesses are okay after you turn on caching, GCC keeps doing this for all packed structures and there's no way to convince it otherwise). In this case, the members are all correctly aligned so there's no need to insert padding, so you can (and therefore should) leave out the packed attribute.
The point is, that without ((packed)) there is no guarantee that gcc won't choose a different alignment over what you and I think would make sense.
Stefan
The point is, that without ((packed)) there is no guarantee that gcc won't choose a different alignment over what you and I think would make sense.
In practice it is very predictable and there should be no sane reason to do it otherwise. Like with many of the other "it's not guaranteed in the standard but GCC and Clang have been doing it this way since forever" issues where being standards-compliant would incur a serious performance or readability hit, I'd advocate being pragmatic. Everyone knows the standard is stupid, but we still need to generate the binaries we want somehow. (And, of course, __attribute__((packed)) is also a GCC extension anyway.)
What other changes to checkpatch are needed?
I'd really suggest that we stick with the upstream checkpatch.pl and use command line flags to tune it to coreboot (submitting patches for new flags or cleaner detection upstream if necessary). The tool gets updated frequently and we'd quickly fall out of sync with that if we don't keep a clean copy that can just be uprevved regularly. Also, this is what the Chromium tree does so maintaining a different fork of checkpatch.pl for upstream coreboot would just make lives harder for everyone working on Chromium (or, more likely, would just make them commit with --no-verify all the time). Working together to improve a common upstream would make all our lives easier.
-
Julius Werner -
Stefan Reinauer