Exactly what the summary says. It comes from getPropertySlot and virtualForWithFunction.
Created attachment 317650 [details] Simple bookkeeping fix in two places
I was confused in my original comment (mixed it with another test case I was looking at). The problem came from putInlineForJSObject and virtualForWithFunction; not from getPropertySlot.
Comment on attachment 317650 [details] Simple bookkeeping fix in two places View in context: https://bugs.webkit.org/attachment.cgi?id=317650&action=review > Source/JavaScriptCore/jit/JITOperations.cpp:1064 > + ASSERT(throwScope.exception() || !error); Out of curiosity, why is the throwException below needed if we're just rethrowing the exception? Or is that not what's happening?
I had not noticed that the exception being re thrown is the same that was already present. I will remove that re-throw as soon as I find a way to make the patch cleanly apply.
Comment on attachment 317650 [details] Simple bookkeeping fix in two places View in context: https://bugs.webkit.org/attachment.cgi?id=317650&action=review >> Source/JavaScriptCore/jit/JITOperations.cpp:1064 >> + ASSERT(throwScope.exception() || !error); > > Out of curiosity, why is the throwException below needed if we're just rethrowing the exception? Or is that not what's happening? There's a better way to express this assertion. See the other examples of calls to functionExecutable->prepareForExecution() in this file above.
Comment on attachment 317650 [details] Simple bookkeeping fix in two places View in context: https://bugs.webkit.org/attachment.cgi?id=317650&action=review >>> Source/JavaScriptCore/jit/JITOperations.cpp:1064 >>> + ASSERT(throwScope.exception() || !error); >> >> Out of curiosity, why is the throwException below needed if we're just rethrowing the exception? Or is that not what's happening? > > There's a better way to express this assertion. See the other examples of calls to functionExecutable->prepareForExecution() in this file above. And yes, it looks like the throwException below is actually not needed though it is harmless. You can remove it here (as well as at the other exception checks after calls to functionExecutable->prepareForExecution() above in this file. Thanks.
Created attachment 317665 [details] Cleaned up patch, also avoids unnecessary re-throws
Comment on attachment 317665 [details] Cleaned up patch, also avoids unnecessary re-throws View in context: https://bugs.webkit.org/attachment.cgi?id=317665&action=review r=me > Source/JavaScriptCore/jit/JITOperations.cpp:1061 > + ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(error)); Is this reinterpret_cast needed?
I originally put this reinterpret cast to match the similar examples higher in the file. After looking at the C++ spec, I believe it is required: - Section 5.10 of the spec says that the operands of '==' have their type converted to their composite pointer type - I do not see anything in the notes at the start of Section 5 anything to define this composite pointer type for JSValue* and Exception*, and the note 13.9 of Section 5 says that "otherwise, a program that necessitates the determination of a composite pointer type is ill-formed." Does anyone know why this patch appeared to fail to compile in several of the queues (iOS, iOS-sim, jsc, Mac, Mac-wk2, Mac-32bit). I have retested-it on top of tree on my machine and it compiles with no issues.
Created attachment 317719 [details] Fixed patch, the problem came from a variable only used in an assert
Comment on attachment 317719 [details] Fixed patch, the problem came from a variable only used in an assert r=me
Comment on attachment 317719 [details] Fixed patch, the problem came from a variable only used in an assert Clearing flags on attachment: 317719 Committed r220471: <http://trac.webkit.org/changeset/220471>
All reviewed patches have been landed. Closing bug.