Bug 175358

Summary: JSTests/stress/v8-deltablue-strict.js fails with JSC_validateExceptionChecks=1
Product: WebKit Reporter: Robin Morisset <rmorisset>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Trivial CC: buildbot, commit-queue, keith_miller, mark.lam, msaboff, rmorisset, sbarati, webkit-bug-importer
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Simple bookkeeping fix in two places
none
Cleaned up patch, also avoids unnecessary re-throws
none
Fixed patch, the problem came from a variable only used in an assert none

Description Robin Morisset 2017-08-08 16:49:02 PDT
Exactly what the summary says. It comes from getPropertySlot and virtualForWithFunction.
Comment 1 Robin Morisset 2017-08-08 16:55:20 PDT
Created attachment 317650 [details]
Simple bookkeeping fix in two places
Comment 2 Robin Morisset 2017-08-08 16:58:11 PDT
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 3 Saam Barati 2017-08-08 17:07:54 PDT
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?
Comment 4 Robin Morisset 2017-08-08 17:15:34 PDT
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 5 Mark Lam 2017-08-08 17:24:20 PDT
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 6 Mark Lam 2017-08-08 17:27:51 PDT
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.
Comment 7 Robin Morisset 2017-08-08 19:13:43 PDT
Created attachment 317665 [details]
Cleaned up patch, also avoids unnecessary re-throws
Comment 8 Saam Barati 2017-08-08 19:30:09 PDT
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?
Comment 9 Robin Morisset 2017-08-09 09:56:42 PDT
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.
Comment 10 Robin Morisset 2017-08-09 10:27:04 PDT
Created attachment 317719 [details]
Fixed patch, the problem came from a variable only used in an assert
Comment 11 Mark Lam 2017-08-09 10:31:50 PDT
Comment on attachment 317719 [details]
Fixed patch, the problem came from a variable only used in an assert

r=me
Comment 12 WebKit Commit Bot 2017-08-09 11:31:28 PDT
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>
Comment 13 WebKit Commit Bot 2017-08-09 11:31:29 PDT
All reviewed patches have been landed.  Closing bug.