Bug 175358 – JSTests/stress/v8-deltablue-strict.js fails with JSC_validateExceptionChecks=1

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Morisset]

Exactly what the summary says. It comes from getPropertySlot and virtualForWithFunction.

[Robin Morisset]

Created attachment 317650 [details]
Simple bookkeeping fix in two places

[Robin Morisset]

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.

[Saam Barati]

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?

[Robin Morisset]

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.

[Mark Lam]

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.

[Mark Lam]

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.

[Robin Morisset]

Created attachment 317665 [details]
Cleaned up patch, also avoids unnecessary re-throws

[Saam Barati]

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?

[Robin Morisset]

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.

[Robin Morisset]

Created attachment 317719 [details]
Fixed patch, the problem came from a variable only used in an assert

[Mark Lam]

Comment on attachment 317719 [details]
Fixed patch, the problem came from a variable only used in an assert

r=me

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.