diff mbox

[Ada] Fix to 64-bit atomic operation failures on ppc-linux

Message ID 20120717101511.GA25996@adacore.com
State New
Headers show

Commit Message

Arnaud Charlet July 17, 2012, 10:15 a.m. UTC 
This patch fixes failures due to the use of 64-bit atomic
operations on ppc-linux. Undo of the previous patch for 64-bit atomic
operations only.

Note: this does NOT address the failure on ppc-darwin, which will be addressed
by another patch soon.

Tested on x86_64-pc-linux-gnu, committed on trunk

2012-07-17  Vincent Pucci  <pucci@adacore.com>

	* s-atopri.adb (Lock_Free_Try_Write_X): Atomic_Compare_Exchange_X
	replaced by Sync_Compare_And_Swap_X.
	(Lock_Free_Try_Write_64): Removed.
	* s-atopri.ads (Sync_Compare_And_Swap_X): Replaces previous
	routine Atomic_Compare_Exchange_X.
	(Lock_Free_Read_64): Renaming of Atomic_Load_64.
	(Lock_Free_Try_Write_64): Renaming of Sync_Compare_And_Swap_64.

Comments

Andrew MacLeod July 17, 2012, 11:57 a.m. UTC | #1 
On 07/17/2012 06:15 AM, Arnaud Charlet wrote:
> This patch fixes failures due to the use of 64-bit atomic
> operations on ppc-linux. Undo of the previous patch for 64-bit atomic
> operations only.
>
> Note: this does NOT address the failure on ppc-darwin, which will be addressed
> by another patch soon.
>
> Tested on x86_64-pc-linux-gnu, committed on trunk
>
> 2012-07-17  Vincent Pucci  <pucci@adacore.com>
>
> 	* s-atopri.adb (Lock_Free_Try_Write_X): Atomic_Compare_Exchange_X
> 	replaced by Sync_Compare_And_Swap_X.
> 	(Lock_Free_Try_Write_64): Removed.
> 	* s-atopri.ads (Sync_Compare_And_Swap_X): Replaces previous
> 	routine Atomic_Compare_Exchange_X.
> 	(Lock_Free_Read_64): Renaming of Atomic_Load_64.
> 	(Lock_Free_Try_Write_64): Renaming of Sync_Compare_And_Swap_64.
>
What is the nature of the atomic failures using the 
__atomic_compare_exchange built-in?   Does it have anything to do with 
the expected value being returned by pointer?



Andrew
Richard Henderson July 17, 2012, 3:55 p.m. UTC | #2 
On 07/17/2012 03:15 AM, Arnaud Charlet wrote:
> +   function Sync_Compare_And_Swap_32
>       (Ptr      : Address;
>        Expected : uint32;
>        Desired  : uint32) return uint32;
>     pragma Import (Intrinsic,
> +                  Sync_Compare_And_Swap_32,
>                    "__sync_val_compare_and_swap_4");
>  
> +   function Sync_Compare_And_Swap_64
>       (Ptr      : Address;
>        Expected : uint64;
> -      Desired  : uint64) return uint64;
> +      Desired  : uint64) return Boolean;
>     pragma Import (Intrinsic,
> +                  Sync_Compare_And_Swap_64,
> -                  "__sync_val_compare_and_swap_8");
> +                  "__sync_bool_compare_and_swap_8");

Ignoring the name change, why return bool only for the 64-bit function?
This is surely papering over some sort of bug elsewhere...


r~
Richard Henderson July 17, 2012, 3:56 p.m. UTC | #3 
On 07/17/2012 04:57 AM, Andrew MacLeod wrote:
> What is the nature of the atomic failures using the
> __atomic_compare_exchange built-in?   Does it have anything to do
> with the expected value being returned by pointer?

It's a rue.  He never was using __atomic_compare_exchange;
the "Atomic_*" was simply the Ada symbol name.


r~
Andrew MacLeod July 17, 2012, 3:58 p.m. UTC | #4 
On 07/17/2012 11:56 AM, Richard Henderson wrote:
> On 07/17/2012 04:57 AM, Andrew MacLeod wrote:
>> What is the nature of the atomic failures using the
>> __atomic_compare_exchange built-in?   Does it have anything to do
>> with the expected value being returned by pointer?
> It's a rue.  He never was using __atomic_compare_exchange;
> the "Atomic_*" was simply the Ada symbol name.
>
>
>

Ah,  I was just disturbed by the apparent switch from 'atomic' to 
'sync'... which is the wrong direction :-)

Andrew
diff mbox

Patch

Index: s-atopri.adb
===================================================================
--- s-atopri.adb	(revision 189565)
+++ s-atopri.adb	(working copy)
@@ -44,7 +44,7 @@ 
 
    begin
       if Expected /= Desired then
-         Actual := Atomic_Compare_Exchange_8 (Ptr, Expected, Desired);
+         Actual := Sync_Compare_And_Swap_8 (Ptr, Expected, Desired);
 
          if Actual /= Expected then
             Expected := Actual;
@@ -68,7 +68,7 @@ 
 
    begin
       if Expected /= Desired then
-         Actual := Atomic_Compare_Exchange_16 (Ptr, Expected, Desired);
+         Actual := Sync_Compare_And_Swap_16 (Ptr, Expected, Desired);
 
          if Actual /= Expected then
             Expected := Actual;
@@ -92,7 +92,7 @@ 
 
    begin
       if Expected /= Desired then
-         Actual := Atomic_Compare_Exchange_32 (Ptr, Expected, Desired);
+         Actual := Sync_Compare_And_Swap_32 (Ptr, Expected, Desired);
 
          if Actual /= Expected then
             Expected := Actual;
@@ -102,28 +102,4 @@ 
 
       return True;
    end Lock_Free_Try_Write_32;
-
-   ----------------------------
-   -- Lock_Free_Try_Write_64 --
-   ----------------------------
-
-   function Lock_Free_Try_Write_64
-      (Ptr      : Address;
-       Expected : in out uint64;
-       Desired  : uint64) return Boolean
-   is
-      Actual : uint64;
-
-   begin
-      if Expected /= Desired then
-         Actual := Atomic_Compare_Exchange_64 (Ptr, Expected, Desired);
-
-         if Actual /= Expected then
-            Expected := Actual;
-            return False;
-         end if;
-      end if;
-
-      return True;
-   end Lock_Free_Try_Write_64;
 end System.Atomic_Primitives;
Index: s-atopri.ads
===================================================================
--- s-atopri.ads	(revision 189565)
+++ s-atopri.ads	(working copy)
@@ -62,16 +62,36 @@ 
    -- GCC built-in atomic primitives --
    ------------------------------------
 
-   function Atomic_Compare_Exchange_8
+   function Atomic_Load_8
+     (Ptr   : Address;
+      Model : Mem_Model := Seq_Cst) return uint8;
+   pragma Import (Intrinsic, Atomic_Load_8, "__atomic_load_1");
+
+   function Atomic_Load_16
+     (Ptr   : Address;
+      Model : Mem_Model := Seq_Cst) return uint16;
+   pragma Import (Intrinsic, Atomic_Load_16, "__atomic_load_2");
+
+   function Atomic_Load_32
+     (Ptr   : Address;
+      Model : Mem_Model := Seq_Cst) return uint32;
+   pragma Import (Intrinsic, Atomic_Load_32, "__atomic_load_4");
+
+   function Atomic_Load_64
+     (Ptr   : Address;
+      Model : Mem_Model := Seq_Cst) return uint64;
+   pragma Import (Intrinsic, Atomic_Load_64, "__atomic_load_8");
+
+   function Sync_Compare_And_Swap_8
      (Ptr      : Address;
       Expected : uint8;
       Desired  : uint8) return uint8;
    pragma Import (Intrinsic,
-                  Atomic_Compare_Exchange_8,
+                  Sync_Compare_And_Swap_8,
                   "__sync_val_compare_and_swap_1");
 
    --  ??? Should use __atomic_compare_exchange_1 (doesn't work yet):
-   --  function Atomic_Compare_Exchange_8
+   --  function Sync_Compare_And_Swap_8
    --    (Ptr           : Address;
    --     Expected      : Address;
    --     Desired       : uint8;
@@ -79,53 +99,33 @@ 
    --     Success_Model : Mem_Model := Seq_Cst;
    --     Failure_Model : Mem_Model := Seq_Cst) return Boolean;
    --  pragma Import (Intrinsic,
-   --                 Atomic_Compare_Exchange_8,
+   --                 Sync_Compare_And_Swap_8,
    --                 "__atomic_compare_exchange_1");
 
-   function Atomic_Compare_Exchange_16
+   function Sync_Compare_And_Swap_16
      (Ptr      : Address;
       Expected : uint16;
       Desired  : uint16) return uint16;
    pragma Import (Intrinsic,
-                  Atomic_Compare_Exchange_16,
+                  Sync_Compare_And_Swap_16,
                   "__sync_val_compare_and_swap_2");
 
-   function Atomic_Compare_Exchange_32
+   function Sync_Compare_And_Swap_32
      (Ptr      : Address;
       Expected : uint32;
       Desired  : uint32) return uint32;
    pragma Import (Intrinsic,
-                  Atomic_Compare_Exchange_32,
+                  Sync_Compare_And_Swap_32,
                   "__sync_val_compare_and_swap_4");
 
-   function Atomic_Compare_Exchange_64
+   function Sync_Compare_And_Swap_64
      (Ptr      : Address;
       Expected : uint64;
-      Desired  : uint64) return uint64;
+      Desired  : uint64) return Boolean;
    pragma Import (Intrinsic,
-                  Atomic_Compare_Exchange_64,
-                  "__sync_val_compare_and_swap_8");
+                  Sync_Compare_And_Swap_64,
+                  "__sync_bool_compare_and_swap_8");
 
-   function Atomic_Load_8
-     (Ptr   : Address;
-      Model : Mem_Model := Seq_Cst) return uint8;
-   pragma Import (Intrinsic, Atomic_Load_8, "__atomic_load_1");
-
-   function Atomic_Load_16
-     (Ptr   : Address;
-      Model : Mem_Model := Seq_Cst) return uint16;
-   pragma Import (Intrinsic, Atomic_Load_16, "__atomic_load_2");
-
-   function Atomic_Load_32
-     (Ptr   : Address;
-      Model : Mem_Model := Seq_Cst) return uint32;
-   pragma Import (Intrinsic, Atomic_Load_32, "__atomic_load_4");
-
-   function Atomic_Load_64
-     (Ptr   : Address;
-      Model : Mem_Model := Seq_Cst) return uint64;
-   pragma Import (Intrinsic, Atomic_Load_64, "__atomic_load_8");
-
    --------------------------
    -- Lock-free operations --
    --------------------------
@@ -136,8 +136,8 @@ 
    --  * Lock_Free_Read_N atomically loads the value of the protected component
    --    accessed by the current protected operation.
 
-   --  * Lock_Free_Try_Write_N tries to write the the Desired value into Ptr
-   --    only if Expected and Desired mismatch.
+   --  * Lock_Free_Try_Write_N tries to write the Desired value into Ptr only
+   --    if Expected and Desired mismatch.
 
    function Lock_Free_Read_8 (Ptr : Address) return uint8 is
      (Atomic_Load_8 (Ptr, Acquire));
@@ -148,8 +148,9 @@ 
    function Lock_Free_Read_32 (Ptr : Address) return uint32 is
       (Atomic_Load_32 (Ptr, Acquire));
 
-   function Lock_Free_Read_64 (Ptr : Address) return uint64 is
-      (Atomic_Load_64 (Ptr, Acquire));
+   function Lock_Free_Read_64
+     (Ptr : Address;
+      Model : Mem_Model := Seq_Cst) return uint64 renames Atomic_Load_64;
 
    function Lock_Free_Try_Write_8
       (Ptr      : Address;
@@ -168,8 +169,8 @@ 
 
    function Lock_Free_Try_Write_64
       (Ptr      : Address;
-       Expected : in out uint64;
-       Desired  : uint64) return Boolean;
+       Expected : uint64;
+       Desired  : uint64) return Boolean renames Sync_Compare_And_Swap_64;
 
    pragma Inline (Lock_Free_Read_8);
    pragma Inline (Lock_Free_Read_16);