From 455a252453eac87853acb776f08d0da773ad8079 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= <pbos@webrtc.org>
Date: Fri, 18 Dec 2015 17:00:25 +0100
Subject: [PATCH] Fix pointer compare-and-swap on Windows.

Incorrect argument order, also added unittest which should've been there
in the first place.

Also renames AtomicLoadPtr to AcquireLoadPtr to match non-ptr version.

BUG=
R=tommi@webrtc.org

Review URL: https://codereview.webrtc.org/1537923003 .

Cr-Commit-Position: refs/heads/master@{#11086}
---
 webrtc/base/atomicops.h                 |  6 +++---
 webrtc/base/criticalsection_unittest.cc | 23 +++++++++++++++++++++++
 webrtc/base/event_tracer.cc             |  2 +-
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/webrtc/base/atomicops.h b/webrtc/base/atomicops.h
index 188cd3f189..a286bf01cc 100644
--- a/webrtc/base/atomicops.h
+++ b/webrtc/base/atomicops.h
@@ -44,13 +44,13 @@ class AtomicOps {
   }
   // Pointer variants.
   template <typename T>
-  static T* AtomicLoadPtr(T* volatile* ptr) {
+  static T* AcquireLoadPtr(T* volatile* ptr) {
     return *ptr;
   }
   template <typename T>
   static T* CompareAndSwapPtr(T* volatile* ptr, T* old_value, T* new_value) {
     return static_cast<T*>(::InterlockedCompareExchangePointer(
-        reinterpret_cast<PVOID volatile*>(ptr), old_value, new_value));
+        reinterpret_cast<PVOID volatile*>(ptr), new_value, old_value));
   }
 #else
   static int Increment(volatile int* i) {
@@ -70,7 +70,7 @@ class AtomicOps {
   }
   // Pointer variants.
   template <typename T>
-  static T* AtomicLoadPtr(T* volatile* ptr) {
+  static T* AcquireLoadPtr(T* volatile* ptr) {
     return __atomic_load_n(ptr, __ATOMIC_ACQUIRE);
   }
   template <typename T>
diff --git a/webrtc/base/criticalsection_unittest.cc b/webrtc/base/criticalsection_unittest.cc
index ff4fdef948..85ef20dc40 100644
--- a/webrtc/base/criticalsection_unittest.cc
+++ b/webrtc/base/criticalsection_unittest.cc
@@ -14,6 +14,7 @@
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/event.h"
 #include "webrtc/base/gunit.h"
+#include "webrtc/base/scoped_ptr.h"
 #include "webrtc/base/scopedptrcollection.h"
 #include "webrtc/base/thread.h"
 #include "webrtc/test/testsupport/gtest_disable.h"
@@ -220,6 +221,28 @@ TEST(AtomicOpsTest, Simple) {
   EXPECT_EQ(0, value);
 }
 
+TEST(AtomicOpsTest, SimplePtr) {
+  class Foo {};
+  Foo* volatile foo = nullptr;
+  scoped_ptr<Foo> a(new Foo());
+  scoped_ptr<Foo> b(new Foo());
+  // Reading the initial value should work as expected.
+  EXPECT_TRUE(rtc::AtomicOps::AcquireLoadPtr(&foo) == nullptr);
+  // Setting using compare and swap should work.
+  EXPECT_TRUE(rtc::AtomicOps::CompareAndSwapPtr(
+                  &foo, static_cast<Foo*>(nullptr), a.get()) == nullptr);
+  EXPECT_TRUE(rtc::AtomicOps::AcquireLoadPtr(&foo) == a.get());
+  // Setting another value but with the wrong previous pointer should fail
+  // (remain a).
+  EXPECT_TRUE(rtc::AtomicOps::CompareAndSwapPtr(
+                  &foo, static_cast<Foo*>(nullptr), b.get()) == a.get());
+  EXPECT_TRUE(rtc::AtomicOps::AcquireLoadPtr(&foo) == a.get());
+  // Replacing a with b should work.
+  EXPECT_TRUE(rtc::AtomicOps::CompareAndSwapPtr(&foo, a.get(), b.get()) ==
+              a.get());
+  EXPECT_TRUE(rtc::AtomicOps::AcquireLoadPtr(&foo) == b.get());
+}
+
 TEST(AtomicOpsTest, Increment) {
   // Create and start lots of threads.
   AtomicOpRunner<IncrementOp, UniqueValueVerifier> runner(0);
diff --git a/webrtc/base/event_tracer.cc b/webrtc/base/event_tracer.cc
index 4f3df82a59..4174589d36 100644
--- a/webrtc/base/event_tracer.cc
+++ b/webrtc/base/event_tracer.cc
@@ -257,7 +257,7 @@ void StopInternalCapture() {
 
 void ShutdownInternalTracer() {
   StopInternalCapture();
-  EventLogger* old_logger = rtc::AtomicOps::AtomicLoadPtr(&g_event_logger);
+  EventLogger* old_logger = rtc::AtomicOps::AcquireLoadPtr(&g_event_logger);
   RTC_DCHECK(old_logger);
   RTC_CHECK(rtc::AtomicOps::CompareAndSwapPtr(
                 &g_event_logger, old_logger,