[heap] Make non-atomic markbit operations consistent with atomic ones.

src/heap/marking.h

Index: src/heap/marking.h
diff --git a/src/heap/marking.h b/src/heap/marking.h
index b20a4d86f149bb7012d648563e47b90613c9fd5b..f09dc71cb2287aa80ee25be327d177e429e8971a 100644
--- a/src/heap/marking.h
+++ b/src/heap/marking.h
@@ -57,8 +57,9 @@ class MarkBit {
 
 template <>
 inline bool MarkBit::Set<MarkBit::NON_ATOMIC>() {
-  *cell_ |= mask_;
-  return true;
+  base::Atomic32 old_value = *cell_;

[Michael Lippautz, 2017/05/03 07:48:39]

Please add a comment on the class indicating that the return values indicate
whether the transition cleared<->set was a success and not the fact that the bit
is set/cleared

[ulan, 2017/05/03 08:51:55]

Done.

+  *cell_ = old_value | mask_;
+  return (old_value & mask_) == 0;

[Hannes Payer (out of office), 2017/05/03 08:29:05]

Why did you change that? The non atomic transition will always succeed.

[ulan, 2017/05/03 08:51:55]

The success means that the bit was 0 and is changed to 1 by the Set like the
ATOMIC case. I added a comment in the class.

Set<NON_ATOMIC> and Set<ATOMIC> should differ only in the atomicity of the
access and should have the same semantic for single threaded execution.
Otherwise, we need to code paths in incremental marker depending on atomicity.

 }
 
 template <>
@@ -86,8 +87,9 @@ inline bool MarkBit::Get<MarkBit::ATOMIC>() {
 
 template <>
 inline bool MarkBit::Clear<MarkBit::NON_ATOMIC>() {
-  *cell_ &= ~mask_;
-  return true;
+  base::Atomic32 old_value = *cell_;
+  *cell_ = old_value & ~mask_;
+  return (old_value & mask_) == mask_;

[Hannes Payer (out of office), 2017/05/03 08:29:05]

Same here, this should always be true.

[ulan, 2017/05/03 08:51:55]

Replied above.

 }
 
 template <>
@@ -412,24 +414,17 @@ class Marking : public AllStatic {
 
   template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
   INLINE(static bool WhiteToGrey(MarkBit markbit)) {
-    DCHECK(mode == MarkBit::ATOMIC || IsWhite(markbit));
     return markbit.Set<mode>();
   }
 
-  // Warning: this method is not safe in general in concurrent scenarios.
-  // If you know that nobody else will change the bits on the given location
-  // then you may use it.
   template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
-  INLINE(static void WhiteToBlack(MarkBit markbit)) {
-    DCHECK(mode == MarkBit::ATOMIC || IsWhite(markbit));
-    markbit.Set<mode>();
-    markbit.Next().Set<mode>();
+  INLINE(static bool WhiteToBlack(MarkBit markbit)) {
+    return markbit.Set<mode>() && markbit.Next().Set<mode>();
   }
 
   template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
   INLINE(static bool GreyToBlack(MarkBit markbit)) {
-    DCHECK(mode == MarkBit::ATOMIC || IsGrey(markbit));
-    return markbit.Next().Set<mode>();
+    return markbit.Get<mode>() && markbit.Next().Set<mode>();
   }
 
   enum ObjectColor {