Re: [Patch 1/2] MdePkg/Include: Add volatile to SynchronizationLib parameters

[Laszlo Ersek <lersek@redhat.com>]

On 11/17/16 05:53, Michael Kinney wrote:
> The SpinLock functions in the SynchronicationLib use volatile
> parameters to keep compiler from optimizing these functions
> too much.  The volatile keyword is missing from the Interlocked*()
> functions in this same library class.  Update the library class
> to consistently use volatile on all functions in this class.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
> ---
>  MdePkg/Include/Library/SynchronizationLib.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/SynchronizationLib.h b/MdePkg/Include/Library/SynchronizationLib.h
> index 7b97683..4f405e2 100644
> --- a/MdePkg/Include/Library/SynchronizationLib.h
> +++ b/MdePkg/Include/Library/SynchronizationLib.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Provides synchronization functions.
>  
> -Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -157,7 +157,7 @@ ReleaseSpinLock (
>  UINT32
>  EFIAPI
>  InterlockedIncrement (
> -  IN      UINT32                    *Value
> +  IN      volatile UINT32                    *Value
>    );
>  
>  
> @@ -179,7 +179,7 @@ InterlockedIncrement (
>  UINT32
>  EFIAPI
>  InterlockedDecrement (
> -  IN      UINT32                    *Value
> +  IN      volatile UINT32                    *Value
>    );
>  
>  
> @@ -204,7 +204,7 @@ InterlockedDecrement (
>  UINT16
>  EFIAPI
>  InterlockedCompareExchange16 (
> -  IN OUT  UINT16                    *Value,
> +  IN OUT  volatile UINT16           *Value,
>    IN      UINT16                    CompareValue,
>    IN      UINT16                    ExchangeValue
>    );
> @@ -231,7 +231,7 @@ InterlockedCompareExchange16 (
>  UINT32
>  EFIAPI
>  InterlockedCompareExchange32 (
> -  IN OUT  UINT32                    *Value,
> +  IN OUT  volatile UINT32           *Value,
>    IN      UINT32                    CompareValue,
>    IN      UINT32                    ExchangeValue
>    );
> @@ -258,7 +258,7 @@ InterlockedCompareExchange32 (
>  UINT64
>  EFIAPI
>  InterlockedCompareExchange64 (
> -  IN OUT  UINT64                    *Value,
> +  IN OUT  volatile UINT64           *Value,
>    IN      UINT64                    CompareValue,
>    IN      UINT64                    ExchangeValue
>    );
> @@ -285,7 +285,7 @@ InterlockedCompareExchange64 (
>  VOID *
>  EFIAPI
>  InterlockedCompareExchangePointer (
> -  IN OUT  VOID                      **Value,
> +  IN OUT  volatile VOID             **Value,
>    IN      VOID                      *CompareValue,
>    IN      VOID                      *ExchangeValue
>    );
> 

In the last prototype, the changed parameter should be:

  IN OUT VOID * volatile * Value

The object that is pointed-to by Value ((which happens to be a pointer)
needs to get the volatile qualification, not the object that the pointer
being exchanged points to.

                    volatile
                +----------------+                +-------------+
Value --------> |     *Value     | -------------> |  **Value    |
                +----------------+                +-------------+

Thanks!
Laszlo