- Revision
- 280195
- Author
- sbar...@apple.com
- Date
- 2021-07-22 13:05:09 -0700 (Thu, 22 Jul 2021)
Log Message
Fix uses of Dependency::fence with respect to the compiler outsmarting us
https://bugs.webkit.org/show_bug.cgi?id=227757
<rdar://problem/80280931>
Reviewed by Robin Morisset.
Source/_javascript_Core:
We were running into issues on arm64 with respect to the memory model
ordering of loads, and how the compiler optimized code around Dependency::fence.
The issue manifested as calls to isMarked incorrectly returning true.
To see the issue, let's consider a program like this:
a = load(p1)
b = load(p2)
if (a != b) return;
d = Dependency::fence(b)
At the point of defining the dependency, the compiler has proven
a == b. So, instead of building the dependency on the register used
for b, we end up using the register for a. So the actual compiled
code ends up with a dependency on load(p1), not load(p2).
To fix this, we end up adding a new API, Dependency::loadEndFence(pointer,
result), which is defined as:
template<typename T>
static Dependency loadAndFence(T* pointer, T& output)
{
T value = *opaque(pointer);
Dependency dependency = Dependency::fence(value);
output = opaque(value);
return dependency;
}
The reason for this is that it split "b" in the above program into two values,
and the "b" the program compares against is not known to the compiler to be
the same value that we build a dependency on.
* heap/MarkedBlock.h:
(JSC::MarkedBlock::aboutToMark):
(JSC::MarkedBlock::isMarked):
* runtime/JSObject.cpp:
(JSC::JSObject::visitButterflyImpl):
* runtime/JSObject.h:
(JSC::JSObject::fencedButterfly):
* runtime/SparseArrayValueMap.cpp:
(JSC::SparseArrayEntry::getConcurrently):
(JSC::SparseArrayEntry::getConcurrently const): Deleted.
* runtime/SparseArrayValueMap.h:
* runtime/Structure.h:
(JSC::Structure::fencedIndexingMode):
* runtime/StructureIDBlob.h:
(JSC::StructureIDBlob::fencedIndexingModeIncludingHistory):
Source/WTF:
* wtf/Atomics.h:
(WTF::opaque):
(WTF::Dependency::loadAndFence):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (280194 => 280195)
--- trunk/Source/_javascript_Core/ChangeLog 2021-07-22 20:04:10 UTC (rev 280194)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-07-22 20:05:09 UTC (rev 280195)
@@ -1,3 +1,58 @@
+2021-07-22 Saam Barati <sbar...@apple.com>
+
+ Fix uses of Dependency::fence with respect to the compiler outsmarting us
+ https://bugs.webkit.org/show_bug.cgi?id=227757
+ <rdar://problem/80280931>
+
+ Reviewed by Robin Morisset.
+
+ We were running into issues on arm64 with respect to the memory model
+ ordering of loads, and how the compiler optimized code around Dependency::fence.
+ The issue manifested as calls to isMarked incorrectly returning true.
+
+ To see the issue, let's consider a program like this:
+ a = load(p1)
+ b = load(p2)
+ if (a != b) return;
+ d = Dependency::fence(b)
+
+ At the point of defining the dependency, the compiler has proven
+ a == b. So, instead of building the dependency on the register used
+ for b, we end up using the register for a. So the actual compiled
+ code ends up with a dependency on load(p1), not load(p2).
+
+ To fix this, we end up adding a new API, Dependency::loadEndFence(pointer,
+ result), which is defined as:
+
+ template<typename T>
+ static Dependency loadAndFence(T* pointer, T& output)
+ {
+ T value = *opaque(pointer);
+ Dependency dependency = Dependency::fence(value);
+ output = opaque(value);
+ return dependency;
+ }
+
+ The reason for this is that it split "b" in the above program into two values,
+ and the "b" the program compares against is not known to the compiler to be
+ the same value that we build a dependency on.
+
+ * heap/MarkedBlock.h:
+ (JSC::MarkedBlock::aboutToMark):
+ (JSC::MarkedBlock::isMarked):
+ * runtime/JSObject.cpp:
+ (JSC::JSObject::visitButterflyImpl):
+ * runtime/JSObject.h:
+ (JSC::JSObject::fencedButterfly):
+ * runtime/SparseArrayValueMap.cpp:
+ (JSC::SparseArrayEntry::getConcurrently):
+ (JSC::SparseArrayEntry::getConcurrently const): Deleted.
+ * runtime/SparseArrayValueMap.h:
+ * runtime/Structure.h:
+ (JSC::Structure::fencedIndexingMode):
+ * runtime/StructureIDBlob.h:
+ (JSC::StructureIDBlob::fencedIndexingModeIncludingHistory):
+
2021-07-22 Keith Miller <keith_mil...@apple.com>
useProfiler option should automatically disable concurrent JIT
Modified: trunk/Source/_javascript_Core/heap/MarkedBlock.h (280194 => 280195)
--- trunk/Source/_javascript_Core/heap/MarkedBlock.h 2021-07-22 20:04:10 UTC (rev 280194)
+++ trunk/Source/_javascript_Core/heap/MarkedBlock.h 2021-07-22 20:05:09 UTC (rev 280195)
@@ -567,10 +567,11 @@
inline Dependency MarkedBlock::aboutToMark(HeapVersion markingVersion)
{
- HeapVersion version = footer().m_markingVersion;
+ HeapVersion version;
+ Dependency dependency = Dependency::loadAndFence(&footer().m_markingVersion, version);
if (UNLIKELY(version != markingVersion))
aboutToMarkSlow(markingVersion);
- return Dependency::fence(version);
+ return dependency;
}
inline void MarkedBlock::Handle::assertMarksNotStale()
@@ -585,10 +586,11 @@
inline bool MarkedBlock::isMarked(HeapVersion markingVersion, const void* p)
{
- HeapVersion version = footer().m_markingVersion;
+ HeapVersion version;
+ Dependency dependency = Dependency::loadAndFence(&footer().m_markingVersion, version);
if (UNLIKELY(version != markingVersion))
return false;
- return footer().m_marks.get(atomNumber(p), Dependency::fence(version));
+ return footer().m_marks.get(atomNumber(p), dependency);
}
inline bool MarkedBlock::isMarked(const void* p, Dependency dependency)
Modified: trunk/Source/_javascript_Core/runtime/JSObject.cpp (280194 => 280195)
--- trunk/Source/_javascript_Core/runtime/JSObject.cpp 2021-07-22 20:04:10 UTC (rev 280194)
+++ trunk/Source/_javascript_Core/runtime/JSObject.cpp 2021-07-22 20:05:09 UTC (rev 280195)
@@ -373,8 +373,8 @@
return nullptr;
structure = vm.getStructure(structureID);
maxOffset = structure->maxOffset();
- IndexingType indexingMode = structure->indexingMode();
- Dependency indexingModeDependency = Dependency::fence(indexingMode);
+ IndexingType indexingMode;
+ Dependency indexingModeDependency = structure->fencedIndexingMode(indexingMode);
Locker<JSCellLock> locker(NoLockingNecessary);
switch (indexingMode) {
case ALL_ARRAY_STORAGE_INDEXING_TYPES:
@@ -389,8 +389,7 @@
default:
break;
}
- butterfly = indexingModeDependency.consume(this)->butterfly();
- Dependency butterflyDependency = Dependency::fence(butterfly);
+ Dependency butterflyDependency = indexingModeDependency.consume(this)->fencedButterfly(butterfly);
if (!butterfly)
return structure;
if (butterflyDependency.consume(this)->structureID() != structureID)
Modified: trunk/Source/_javascript_Core/runtime/JSObject.h (280194 => 280195)
--- trunk/Source/_javascript_Core/runtime/JSObject.h 2021-07-22 20:04:10 UTC (rev 280194)
+++ trunk/Source/_javascript_Core/runtime/JSObject.h 2021-07-22 20:05:09 UTC (rev 280195)
@@ -748,6 +748,10 @@
const Butterfly* butterfly() const { return m_butterfly.get(); }
Butterfly* butterfly() { return m_butterfly.get(); }
+ Dependency fencedButterfly(Butterfly*& butterfly)
+ {
+ return Dependency::loadAndFence(static_cast<Butterfly**>(butterflyAddress()), butterfly);
+ }
ConstPropertyStorage outOfLineStorage() const { return m_butterfly->propertyStorage(); }
PropertyStorage outOfLineStorage() { return m_butterfly->propertyStorage(); }
Modified: trunk/Source/_javascript_Core/runtime/SparseArrayValueMap.cpp (280194 => 280195)
--- trunk/Source/_javascript_Core/runtime/SparseArrayValueMap.cpp 2021-07-22 20:04:10 UTC (rev 280194)
+++ trunk/Source/_javascript_Core/runtime/SparseArrayValueMap.cpp 2021-07-22 20:05:09 UTC (rev 280195)
@@ -174,8 +174,8 @@
// By emitting store-store-fence and load-load-fence between value setting and attributes setting,
// we can ensure that the value is what we want once the attributes get ReadOnly & DontDelete:
// once attributes get this state, the value should not be changed.
- unsigned attributes = m_attributes;
- Dependency attributesDependency = Dependency::fence(attributes);
+ unsigned attributes;
+ Dependency attributesDependency = Dependency::loadAndFence(&m_attributes, attributes);
if (attributes & PropertyAttribute::Accessor)
return JSValue();
Modified: trunk/Source/_javascript_Core/runtime/Structure.h (280194 => 280195)
--- trunk/Source/_javascript_Core/runtime/Structure.h 2021-07-22 20:04:10 UTC (rev 280194)
+++ trunk/Source/_javascript_Core/runtime/Structure.h 2021-07-22 20:05:09 UTC (rev 280195)
@@ -267,6 +267,12 @@
IndexingType indexingType() const { return m_blob.indexingModeIncludingHistory() & AllWritableArrayTypes; }
IndexingType indexingMode() const { return m_blob.indexingModeIncludingHistory() & AllArrayTypes; }
+ Dependency fencedIndexingMode(IndexingType& indexingType)
+ {
+ Dependency dependency = m_blob.fencedIndexingModeIncludingHistory(indexingType);
+ indexingType &= AllArrayTypes;
+ return dependency;
+ }
IndexingType indexingModeIncludingHistory() const { return m_blob.indexingModeIncludingHistory(); }
inline bool mayInterceptIndexedAccesses() const;
Modified: trunk/Source/_javascript_Core/runtime/StructureIDBlob.h (280194 => 280195)
--- trunk/Source/_javascript_Core/runtime/StructureIDBlob.h 2021-07-22 20:04:10 UTC (rev 280194)
+++ trunk/Source/_javascript_Core/runtime/StructureIDBlob.h 2021-07-22 20:05:09 UTC (rev 280195)
@@ -53,6 +53,10 @@
StructureID structureID() const { return u.fields.structureID; }
IndexingType indexingModeIncludingHistory() const { return u.fields.indexingModeIncludingHistory; }
+ Dependency fencedIndexingModeIncludingHistory(IndexingType& indexingType)
+ {
+ return Dependency::loadAndFence(&u.fields.indexingModeIncludingHistory, indexingType);
+ }
void setIndexingModeIncludingHistory(IndexingType indexingModeIncludingHistory) { u.fields.indexingModeIncludingHistory = indexingModeIncludingHistory; }
JSType type() const { return u.fields.type; }
TypeInfo::InlineTypeFlags inlineTypeFlags() const { return u.fields.inlineTypeFlags; }
Modified: trunk/Source/WTF/ChangeLog (280194 => 280195)
--- trunk/Source/WTF/ChangeLog 2021-07-22 20:04:10 UTC (rev 280194)
+++ trunk/Source/WTF/ChangeLog 2021-07-22 20:05:09 UTC (rev 280195)
@@ -1,3 +1,15 @@
+2021-07-22 Saam Barati <sbar...@apple.com>
+
+ Fix uses of Dependency::fence with respect to the compiler outsmarting us
+ https://bugs.webkit.org/show_bug.cgi?id=227757
+ <rdar://problem/80280931>
+
+ Reviewed by Robin Morisset.
+
+ * wtf/Atomics.h:
+ (WTF::opaque):
+ (WTF::Dependency::loadAndFence):
+
2021-07-22 Yusuke Suzuki <ysuz...@apple.com>
Unreviewed, follow-up after r280193
Modified: trunk/Source/WTF/wtf/Atomics.h (280194 => 280195)
--- trunk/Source/WTF/wtf/Atomics.h 2021-07-22 20:04:10 UTC (rev 280194)
+++ trunk/Source/WTF/wtf/Atomics.h 2021-07-22 20:05:09 UTC (rev 280195)
@@ -329,6 +329,15 @@
inline void dependentLoadLoadFence() { loadLoadFence(); }
#endif
+template<typename T>
+T opaque(T pointer)
+{
+#if !OS(WINDOWS)
+ asm volatile("" : "+r"(pointer) ::);
+#endif
+ return pointer;
+}
+
typedef unsigned InternalDependencyType;
inline InternalDependencyType opaqueMixture()
@@ -392,6 +401,55 @@
result.m_value = output;
return result;
}
+
+ // This function exists as a helper to aid in not making mistakes when doing a load
+ // and fencing on the result of the load. A couple examples of where things can go
+ // wrong, and how this function helps:
+ //
+ // Consider this program:
+ // ```
+ // a = load(p1)
+ // b = load(p2)
+ // if (a != b) return;
+ // d = Dependency::fence(b)
+ // ```
+ // When consuming the d dependency, the compiler can prove that a and b are the same
+ // value, and end up replacing the dependency on whatever register is allocated for `a`
+ // instead of being over `b`, leading to the dependency being on load(p1) instead of
+ // load(p2). We fix this by splitting the value feeding into the fence and the value
+ // being used:
+ // b' = load(p2)
+ // Dependency::fence(b')
+ // b = opaque(b')
+ // b' feeds into the fence, and b will be the value compared. Crucially, the compiler can't
+ // prove that b == b'.
+ //
+ // Let's consider another use case. Imagine you end up with a program like this (perhaps
+ // after some inlining or various optimizations):
+ // a = load(p1)
+ // b = load(p2)
+ // if (a != b) return;
+ // c = load(p2)
+ // d = Dependency::fence(c)
+ // Similar to the first test, the compiler can prove a and b are the same, allowing it to
+ // prove that c == a == b, allowing it to potentially have the dependency be on the wrong
+ // value, similar to above. The fix here is to obscure the pointer we're loading from from
+ // the compiler.
+ template<typename T>
+ static Dependency loadAndFence(const T* pointer, T& output)
+ {
+#if CPU(ARM64) || CPU(ARM)
+ T value = *opaque(pointer);
+ Dependency dependency = Dependency::fence(value);
+ output = opaque(value);
+ return dependency;
+#else
+ T value = *pointer;
+ Dependency dependency = Dependency::fence(value);
+ output = value;
+ return dependency;
+#endif
+ }
// On TSO architectures, this just returns the pointer you pass it. On ARM, this produces a new
// pointer that is dependent on this dependency and the input pointer.
