svn commit: r278431 - head/sys/contrib/vchiq/interface/vchiq_arm

[Bruce Evans]

On Mon, 9 Feb 2015, Oleksandr Tymoshenko wrote:

> Log:
>  Do not mark shared structures as __packed, it leads to race condition
>
>  If structure packed as __packed clang (and probably gcc) generates
>  code that loads word fields (e.g. tx_pos)  byte-by-byte and if it's
>  modified by VideoCore in the same time as ARM loads the value result
>  is going to be mixed combination of bytes from previous value and
>  new one.

Most uses of __packed are bugs.  It gives pessimizations as well as
non-atomic accesses for sub-object accesses.

I think the full bugs only occur when arch has strict alignment
requirements and the alignment of the __packed objects is not known.
This means that only lesser bugs occur on x86 (unless you enable
alignment checking, but this arguably breaks the ABI).  The compiler
just generates possibly-misaligned full-width accesses if the arch
doesn't have strict alignment requirements.  Often the acceses turn
out to be aligned at runtime.  Otherwise, the hardware does them
atomically, with a smaller efficiency penalty than split accesses.

Many packed structs should also be declared as __aligned(N).  This is
rarely done.  Old networking code in <netinet> uses __packed just once,
and this also uses __aligned(4) (for struct ip)  Newer networking code
in <netinet> (for ipv6) is massively pessimized, with __packed used
extensively and __aligned(N) never used with __packed.  sctp is worse.
It uses its own macro that defeats grepping for __packed.  It has 82
instances of this in <netinet> where ipv6 has only 34.  Newer networking
should need __packed less than old ones, since no one would misdesign a
data struct so that it needs packing now.

gcc documents the -Wpacked flag for finding some cases of bogus packing.
It is rarely used.

Bruce