Adding DECLSPEC_ALIGN(4) to structs for atomic DWORD access

Hi,

there are some places in Wine where I believe atomic access to 32bit entities
is expected by the programmer, yet I see little or no measures to ensure this.

DWORD-Alignment is essential when several threads can access
the same DWORD variable outside of a mutex (or within the implementation of the mutex).

DECLSPEC_ALIGN(4) is the declaration that could be used to enforce
it within a struct, assuming the struct itself is somehow aligned.

E.g. shouldn't CRITICAL_SECTION objects be aligned when used within other (non-public) structs?
(Presumably public MS structs cannot be augmented with alignment declarations)

DECLSPEC_ALIGN is hardly used in Wine
http://source.winehq.org/ident?i=RTL_CRITICAL_SECTION

Other examples where I'm considering adding DWORD-alignment
to read DWORD-sized objects atomically:
 - MCI position values (for MCI_STATUS_POSITION)
 - MIDI end_thread control variable
 - mmdevapi GetCurrentPadding (as part of lock-less patch)

Would such patches be welcome?  Are they superfluous, assuming
that DWORD-alignment is granted and using DECLSPEC_ALIGN() only
for larger alignments, e.g. 8 or 16 (that seems to be the current use)?

One benefit I see is to highlight those variables shared among several threads.

Regards,
    Jörg Höhle

[David Laight]

On Mon, Jan 21, 2013 at 06:52:23PM +0100, [e-mail] wrote:
> Hi,
>
> there are some places in Wine where I believe atomic access to 32bit entities
> is expected by the programmer, yet I see little or no measures to ensure this.
>
> DWORD-Alignment is essential when several threads can access
> the same DWORD variable outside of a mutex (or within the implementation of the mutex).
>
> DECLSPEC_ALIGN(4) is the declaration that could be used to enforce
> it within a struct, assuming the struct itself is somehow aligned.

32bit items will be 32bit aligned unless the structure is 'packed'
(or similar).

David

[Dmitry Timoshkov]

 wrote:
> there are some places in Wine where I believe atomic access to 32bit entities
> is expected by the programmer, yet I see little or no measures to ensure this.

Then it's a bug, and should be fixed by adding appropriate locking.