I have not read the entire thread, but just in case it was not mentioned before: even when the structure elements come together very snugly on their own, without __packed(), it still is required if the structure comes over a wire or from a file, etc. as __packed() guarantees that there is no unaligned accesses.
Also, what seems to be missing in checkpatch.pl (or is ti there, I just don't know about it?) is the ability to suppress a warning for just a certain line of code, so that when the style deviations are necessary, they are clearly marked/explained.
-v
On Thu, Jul 28, 2016 at 1:14 PM, Julius Werner jwerner@chromium.org wrote:
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.
-- coreboot mailing list: coreboot@coreboot.org https://www.coreboot.org/mailman/listinfo/coreboot
-
Vadim Bendebury