<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[165119] trunk/Source/JavaScriptCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/165119">165119</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2014-03-05 12:26:58 -0800 (Wed, 05 Mar 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>FTL loadStructure always generates invalid IR
https://bugs.webkit.org/show_bug.cgi?id=129747
Reviewed by Mark Hahnenberg.
As the comment at the top of FTL::Output states, the FTL doesn't use LLVM's notion
of pointers. LLVM's notion of pointers tries to model C, in the sense that you have
to have a pointer to a type, and you can only load things of that type from that
pointer. Pointer arithmetic is basically not possible except through the bizarre
getelementptr operator. This doesn't fit with how the JS object model works since
the JS object model doesn't consist of nice and tidy C types placed in C arrays.
Also, it would be impossible to use getelementptr and LLVM pointers for accessing
any of JSC's C or C++ objects unless we went through the exercise of redeclaring
all of our fundamental data structures in LLVM IR as LLVM types. Clang could do
this for us, but that would require that to use the FTL, JSC itself would have to
be compiled with clang. Worse, it would have to be compiled with a clang that uses
a version of LLVM that is compatible with the one against which the FTL is linked.
Yuck!
The solution is to NEVER use LLVM pointers. This has always been the case in the
FTL. But it causes some confusion.
Not using LLVM pointers means that if the FTL has a "pointer", it's actually a
pointer-wide integer (m_out.intPtr in FTL-speak). The act of "loading" and
"storing" from or to a pointer involves first bitcasting the intPtr to a real LLVM
pointer that has the type that we want. The load and store operations over pointers
are called Output::load* and Output::store*, where * is one of "8", "16", "32",
"64", "Ptr", "Float", or "Double.
There is unavoidable confusion here. It would be bizarre for the FTL to call its
"pointer-wide integers" anything other than "pointers", since they are, in all
respects that we care about, simply pointers. But they are *not* LLVM pointers and
they never will be that.
There is one exception to this "no pointers" rule. The FTL does use actual LLVM
pointers for refering to LLVM alloca's - i.e. local variables. To try to reduce
confusion, we call these "references". So an "FTL reference" is actually an "LLVM
pointer", while an "FTL pointer" is actually an "LLVM integer". FTL references have
methods for access called Output::get and Output::set. These lower to LLVM load
and store, since FTL references are just LLVM pointers.
This confusion appears to have led to incorrect code in loadStructure().
loadStructure() was using get() and set() to access FTL pointers. But those methods
don't work on FTL pointers and never will, since they are for FTL references.
The worst part of this is that it was previously impossible to have test coverage
for the relevant path (MasqueradesAsUndefined) without writing a DRT test. This
patch fixes this by introducing a Masquerader object to jsc.cpp.
* ftl/FTLAbstractHeapRepository.h: Add an abstract heap for the structure table.
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::LowerDFGToLLVM::loadStructure): This was wrong.
* ftl/FTLOutput.h: Add a comment to disuade people from using get() and set().
* jsc.cpp: Give us the power to test for MasqueradesAsUndefined.
(WTF::Masquerader::Masquerader):
(WTF::Masquerader::create):
(WTF::Masquerader::createStructure):
(GlobalObject::finishCreation):
(functionMakeMasquerader):
* tests/stress/equals-masquerader.js: Added.
(foo):
(test):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreftlFTLAbstractHeapRepositoryh">trunk/Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreftlFTLLowerDFGToLLVMcpp">trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreftlFTLOutputh">trunk/Source/JavaScriptCore/ftl/FTLOutput.h</a></li>
<li><a href="#trunkSourceJavaScriptCorejsccpp">trunk/Source/JavaScriptCore/jsc.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoretestsstressequalsmasqueraderjs">trunk/Source/JavaScriptCore/tests/stress/equals-masquerader.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (165118 => 165119)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2014-03-05 20:16:10 UTC (rev 165118)
+++ trunk/Source/JavaScriptCore/ChangeLog 2014-03-05 20:26:58 UTC (rev 165119)
</span><span class="lines">@@ -1,3 +1,68 @@
</span><ins>+2014-03-05 Filip Pizlo <fpizlo@apple.com>
+
+ FTL loadStructure always generates invalid IR
+ https://bugs.webkit.org/show_bug.cgi?id=129747
+
+ Reviewed by Mark Hahnenberg.
+
+ As the comment at the top of FTL::Output states, the FTL doesn't use LLVM's notion
+ of pointers. LLVM's notion of pointers tries to model C, in the sense that you have
+ to have a pointer to a type, and you can only load things of that type from that
+ pointer. Pointer arithmetic is basically not possible except through the bizarre
+ getelementptr operator. This doesn't fit with how the JS object model works since
+ the JS object model doesn't consist of nice and tidy C types placed in C arrays.
+ Also, it would be impossible to use getelementptr and LLVM pointers for accessing
+ any of JSC's C or C++ objects unless we went through the exercise of redeclaring
+ all of our fundamental data structures in LLVM IR as LLVM types. Clang could do
+ this for us, but that would require that to use the FTL, JSC itself would have to
+ be compiled with clang. Worse, it would have to be compiled with a clang that uses
+ a version of LLVM that is compatible with the one against which the FTL is linked.
+ Yuck!
+
+ The solution is to NEVER use LLVM pointers. This has always been the case in the
+ FTL. But it causes some confusion.
+
+ Not using LLVM pointers means that if the FTL has a "pointer", it's actually a
+ pointer-wide integer (m_out.intPtr in FTL-speak). The act of "loading" and
+ "storing" from or to a pointer involves first bitcasting the intPtr to a real LLVM
+ pointer that has the type that we want. The load and store operations over pointers
+ are called Output::load* and Output::store*, where * is one of "8", "16", "32",
+ "64", "Ptr", "Float", or "Double.
+
+ There is unavoidable confusion here. It would be bizarre for the FTL to call its
+ "pointer-wide integers" anything other than "pointers", since they are, in all
+ respects that we care about, simply pointers. But they are *not* LLVM pointers and
+ they never will be that.
+
+ There is one exception to this "no pointers" rule. The FTL does use actual LLVM
+ pointers for refering to LLVM alloca's - i.e. local variables. To try to reduce
+ confusion, we call these "references". So an "FTL reference" is actually an "LLVM
+ pointer", while an "FTL pointer" is actually an "LLVM integer". FTL references have
+ methods for access called Output::get and Output::set. These lower to LLVM load
+ and store, since FTL references are just LLVM pointers.
+
+ This confusion appears to have led to incorrect code in loadStructure().
+ loadStructure() was using get() and set() to access FTL pointers. But those methods
+ don't work on FTL pointers and never will, since they are for FTL references.
+
+ The worst part of this is that it was previously impossible to have test coverage
+ for the relevant path (MasqueradesAsUndefined) without writing a DRT test. This
+ patch fixes this by introducing a Masquerader object to jsc.cpp.
+
+ * ftl/FTLAbstractHeapRepository.h: Add an abstract heap for the structure table.
+ * ftl/FTLLowerDFGToLLVM.cpp:
+ (JSC::FTL::LowerDFGToLLVM::loadStructure): This was wrong.
+ * ftl/FTLOutput.h: Add a comment to disuade people from using get() and set().
+ * jsc.cpp: Give us the power to test for MasqueradesAsUndefined.
+ (WTF::Masquerader::Masquerader):
+ (WTF::Masquerader::create):
+ (WTF::Masquerader::createStructure):
+ (GlobalObject::finishCreation):
+ (functionMakeMasquerader):
+ * tests/stress/equals-masquerader.js: Added.
+ (foo):
+ (test):
+
</ins><span class="cx"> 2014-03-05 Anders Carlsson <andersca@apple.com>
</span><span class="cx">
</span><span class="cx"> Tweak after r165109 to avoid extra copies
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreftlFTLAbstractHeapRepositoryh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h (165118 => 165119)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h 2014-03-05 20:16:10 UTC (rev 165118)
+++ trunk/Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h 2014-03-05 20:26:58 UTC (rev 165119)
</span><span class="lines">@@ -36,6 +36,7 @@
</span><span class="cx">
</span><span class="cx"> #define FOR_EACH_ABSTRACT_HEAP(macro) \
</span><span class="cx"> macro(length) \
</span><ins>+ macro(structureTable) \
</ins><span class="cx"> macro(typedArrayProperties) \
</span><span class="cx"> macro(WriteBarrierBuffer_bufferContents)
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreftlFTLLowerDFGToLLVMcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp (165118 => 165119)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp 2014-03-05 20:16:10 UTC (rev 165118)
+++ trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp 2014-03-05 20:26:58 UTC (rev 165119)
</span><span class="lines">@@ -5172,15 +5172,6 @@
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><del>- LValue hasClassInfo(LValue cell, const ClassInfo* classInfo)
- {
- return m_out.equal(
- m_out.loadPtr(
- loadStructure(cell),
- m_heaps.Structure_classInfo),
- m_out.constIntPtr(classInfo));
- }
-
</del><span class="cx"> LValue isType(LValue cell, JSType type)
</span><span class="cx"> {
</span><span class="cx"> return m_out.equal(
</span><span class="lines">@@ -5845,8 +5836,11 @@
</span><span class="cx"> LValue loadStructure(LValue value)
</span><span class="cx"> {
</span><span class="cx"> LValue tableIndex = m_out.load32(value, m_heaps.JSCell_structureID);
</span><del>- LValue tableBase = m_out.get(m_out.constIntPtr(vm().heap.structureIDTable().base()));
- return m_out.get(m_out.baseIndex(tableBase, tableIndex, ScaleEight));
</del><ins>+ LValue tableBase = m_out.loadPtr(
+ m_out.absolute(vm().heap.structureIDTable().base()));
+ LValue pointerIntoTable = m_out.baseIndex(
+ tableBase, m_out.zeroExt(tableIndex, m_out.intPtr), ScaleEight);
+ return m_out.loadPtr(TypedPointer(m_heaps.structureTable, pointerIntoTable));
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> LValue weakPointer(JSCell* pointer)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreftlFTLOutputh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ftl/FTLOutput.h (165118 => 165119)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ftl/FTLOutput.h 2014-03-05 20:16:10 UTC (rev 165118)
+++ trunk/Source/JavaScriptCore/ftl/FTLOutput.h 2014-03-05 20:26:58 UTC (rev 165119)
</span><span class="lines">@@ -204,7 +204,15 @@
</span><span class="cx"> LValue bitCast(LValue value, LType type) { return buildBitCast(m_builder, value, type); }
</span><span class="cx">
</span><span class="cx"> LValue alloca(LType type) { return buildAlloca(m_builder, type); }
</span><ins>+
+ // Access the value of an alloca. Also used as a low-level implementation primitive for
+ // load(). Never use this to load from "pointers" in the FTL sense, since FTL pointers
+ // are actually integers. This requires an LLVM pointer. Broadly speaking, you don't
+ // have any LLVM pointers even if you really think you do. A TypedPointer is not an
+ // LLVM pointer. See comment block at top of this file to understand the distinction
+ // between LLVM pointers, FTL pointers, and FTL references.
</ins><span class="cx"> LValue get(LValue reference) { return buildLoad(m_builder, reference); }
</span><ins>+ // Similar to get() but for storing to the value in an alloca.
</ins><span class="cx"> LValue set(LValue value, LValue reference) { return buildStore(m_builder, value, reference); }
</span><span class="cx">
</span><span class="cx"> LValue load(TypedPointer, LType refType);
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorejsccpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/jsc.cpp (165118 => 165119)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/jsc.cpp 2014-03-05 20:16:10 UTC (rev 165118)
+++ trunk/Source/JavaScriptCore/jsc.cpp 2014-03-05 20:26:58 UTC (rev 165119)
</span><span class="lines">@@ -94,6 +94,7 @@
</span><span class="cx">
</span><span class="cx"> class Element;
</span><span class="cx"> class ElementHandleOwner;
</span><ins>+class Masuqerader;
</ins><span class="cx"> class Root;
</span><span class="cx">
</span><span class="cx"> class Element : public JSNonFinalObject {
</span><span class="lines">@@ -142,6 +143,35 @@
</span><span class="cx"> }
</span><span class="cx"> };
</span><span class="cx">
</span><ins>+class Masquerader : public JSNonFinalObject {
+public:
+ Masquerader(VM& vm, Structure* structure)
+ : Base(vm, structure)
+ {
+ }
+
+ typedef JSNonFinalObject Base;
+
+ static Masquerader* create(VM& vm, JSGlobalObject* globalObject)
+ {
+ globalObject->masqueradesAsUndefinedWatchpoint()->fireAll();
+ Structure* structure = createStructure(vm, globalObject, jsNull());
+ Masquerader* result = new (NotNull, allocateCell<Masquerader>(vm.heap, sizeof(Masquerader))) Masquerader(vm, structure);
+ result->finishCreation(vm);
+ return result;
+ }
+
+ static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
+ {
+ return Structure::create(vm, globalObject, prototype, TypeInfo(ObjectType, StructureFlags), info());
+ }
+
+ DECLARE_INFO;
+
+protected:
+ static const unsigned StructureFlags = JSC::MasqueradesAsUndefined | Base::StructureFlags;
+};
+
</ins><span class="cx"> class Root : public JSDestructibleObject {
</span><span class="cx"> public:
</span><span class="cx"> Root(VM& vm, Structure* structure)
</span><span class="lines">@@ -189,6 +219,7 @@
</span><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> const ClassInfo Element::s_info = { "Element", &Base::s_info, 0, 0, CREATE_METHOD_TABLE(Element) };
</span><ins>+const ClassInfo Masquerader::s_info = { "Masquerader", &Base::s_info, 0, 0, CREATE_METHOD_TABLE(Masquerader) };
</ins><span class="cx"> const ClassInfo Root::s_info = { "Root", &Base::s_info, 0, 0, CREATE_METHOD_TABLE(Root) };
</span><span class="cx">
</span><span class="cx"> ElementHandleOwner* Element::handleOwner()
</span><span class="lines">@@ -239,6 +270,7 @@
</span><span class="cx"> static NO_RETURN_WITH_VALUE EncodedJSValue JSC_HOST_CALL functionQuit(ExecState*);
</span><span class="cx"> static EncodedJSValue JSC_HOST_CALL functionFalse(ExecState*);
</span><span class="cx"> static EncodedJSValue JSC_HOST_CALL functionEffectful42(ExecState*);
</span><ins>+static EncodedJSValue JSC_HOST_CALL functionMakeMasquerader(ExecState*);
</ins><span class="cx">
</span><span class="cx"> #if ENABLE(SAMPLING_FLAGS)
</span><span class="cx"> static EncodedJSValue JSC_HOST_CALL functionSetSamplingFlags(ExecState*);
</span><span class="lines">@@ -376,6 +408,7 @@
</span><span class="cx"> putDirectNativeFunction(vm, this, Identifier(&vm, "DFGTrue"), 0, functionFalse, DFGTrue, DontEnum | JSC::Function);
</span><span class="cx">
</span><span class="cx"> addFunction(vm, "effectful42", functionEffectful42, 0);
</span><ins>+ addFunction(vm, "makeMasquerader", functionMakeMasquerader, 0);
</ins><span class="cx">
</span><span class="cx"> JSArray* array = constructEmptyArray(globalExec(), 0);
</span><span class="cx"> for (size_t i = 0; i < arguments.size(); ++i)
</span><span class="lines">@@ -747,6 +780,11 @@
</span><span class="cx"> return JSValue::encode(jsNumber(42));
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+EncodedJSValue JSC_HOST_CALL functionMakeMasquerader(ExecState* exec)
+{
+ return JSValue::encode(Masquerader::create(exec->vm(), exec->lexicalGlobalObject()));
+}
+
</ins><span class="cx"> // Use SEH for Release builds only to get rid of the crash report dialog
</span><span class="cx"> // (luckily the same tests fail in Release and Debug builds so far). Need to
</span><span class="cx"> // be in a separate main function because the jscmain function requires object
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoretestsstressequalsmasqueraderjs"></a>
<div class="addfile"><h4>Added: trunk/Source/JavaScriptCore/tests/stress/equals-masquerader.js (0 => 165119)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/tests/stress/equals-masquerader.js (rev 0)
+++ trunk/Source/JavaScriptCore/tests/stress/equals-masquerader.js 2014-03-05 20:26:58 UTC (rev 165119)
</span><span class="lines">@@ -0,0 +1,17 @@
</span><ins>+function foo(o) {
+ return o == null;
+}
+
+noInline(foo);
+
+function test(object, outcome) {
+ var result = foo(object);
+ if (result != outcome)
+ throw new Error("Bad result: " + result);
+}
+
+for (var i = 0; i < 100000; ++i) {
+ test(null, true);
+ test({}, false);
+ test(makeMasquerader(), true);
+}
</ins></span></pre>
</div>
</div>
</body>
</html>