Bug 162521 - Added RETURN_IF_EXCEPTION() macro and use it for exception checks
Summary: Added RETURN_IF_EXCEPTION() macro and use it for exception checks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on: 172846
Blocks: 162351
  Show dependency treegraph
 
Reported: 2016-09-23 16:55 PDT by Mark Lam
Modified: 2017-06-01 18:58 PDT (History)
8 users (show)

See Also:


Attachments
work in progress for EWS testing. (376.42 KB, patch)
2016-09-23 17:02 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (402.11 KB, patch)
2016-09-23 19:55 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-09-23 16:55:24 PDT
And use it in all places that currently explicitly check for exceptions and return early.

Also, as much as is possible, if the return type is JSValue, change the returned value (on exception) to the empty JSValue (instead of sometimes jsUndefined() and jsNull()).
Comment 1 Mark Lam 2016-09-23 17:02:11 PDT
Created attachment 289721 [details]
work in progress for EWS testing.
Comment 2 Mark Lam 2016-09-23 19:55:00 PDT
Created attachment 289735 [details]
proposed patch.
Comment 3 Saam Barati 2016-09-24 16:45:45 PDT
Comment on attachment 289735 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=289735&action=review

r=me

> Source/JavaScriptCore/profiler/ProfilerBytecodeSequence.cpp:90
> +    RETURN_IF_EXCEPTION(scope, void());

Why not just a variant like:
RETURN_IF_EXCEPTION(scope);

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:202
> +#define EXCEPTION_RESULT std::make_pair(SpeciesConstructResult::Exception, nullptr)

Why not just make a lambda that returns this pair? I wonder what llvm would do if you just allocate the pair locally, too. Maybe it'd sink that allocation to the return points. Not 100% sure though.

> Source/JavaScriptCore/runtime/ExceptionScope.h:74
> +#define RETURN_IF_EXCEPTION(scope__, value__) do { \

I wish we also had a macro for this pattern:
scope.release();
return someOtherFunction(...);
so we could write:
SOME_MACRO_NAME(scope, someOtherFunction(...));

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:105
> +    if (!result)
> +        RETURN_IF_EXCEPTION(scope, nullptr);

I think a better translation here would be:
RETURN_IF_EXCEPTION(scope, nullptr);
ASSERT(!scope.exception());

> Source/JavaScriptCore/runtime/ProxyConstructor.cpp:73
> +result->putDirect(vm, makeIdentifier(vm, "proxy"), proxy, None);

indentation is off.
Comment 4 Geoffrey Garen 2016-09-25 09:15:34 PDT
Comment on attachment 289735 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=289735&action=review

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:202
>> +#define EXCEPTION_RESULT std::make_pair(SpeciesConstructResult::Exception, nullptr)
> 
> Why not just make a lambda that returns this pair? I wonder what llvm would do if you just allocate the pair locally, too. Maybe it'd sink that allocation to the return points. Not 100% sure though.

This should just be a static inline function. Functions are strictly better than macros.
Comment 5 Mark Lam 2016-09-26 12:08:33 PDT
Comment on attachment 289735 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=289735&action=review

>> Source/JavaScriptCore/profiler/ProfilerBytecodeSequence.cpp:90
>> +    RETURN_IF_EXCEPTION(scope, void());
> 
> Why not just a variant like:
> RETURN_IF_EXCEPTION(scope);

AFAIK, we can't overload preprocessor macros.  Hence, this is the compromise for not having to need 2 forms of the macro.

>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:202
>>> +#define EXCEPTION_RESULT std::make_pair(SpeciesConstructResult::Exception, nullptr)
>> 
>> Why not just make a lambda that returns this pair? I wonder what llvm would do if you just allocate the pair locally, too. Maybe it'd sink that allocation to the return points. Not 100% sure though.
> 
> This should just be a static inline function. Functions are strictly better than macros.

I will go with Saam's suggestion and use a lambda since this lambda function is only called in speciesConstructArray() and no other functions.  Making it static seems like more boilerplate at this time.  From what I can find online, lambdas are inlineable: see http://stackoverflow.com/questions/13722426/why-can-lambdas-be-better-optimized-by-the-compiler-than-plain-functions.

>> Source/JavaScriptCore/runtime/ExceptionScope.h:74
>> +#define RETURN_IF_EXCEPTION(scope__, value__) do { \
> 
> I wish we also had a macro for this pattern:
> scope.release();
> return someOtherFunction(...);
> so we could write:
> SOME_MACRO_NAME(scope, someOtherFunction(...));

I'll leave this for future consideration if we think of a good way to express this idea.  FYI, before we arrived on the scope.release() idiom, we had already rejected an idiom like this:

     return scope.checked(someOtherFunction(...));

... because scope.checked() here seems to be pretty long (in characters) and it slightly obscures the real action the reader probably wants to focus on i.e. the call to someOtherFunction().  I bring this up because this is similar in length of chars to read to the macro you proposed (or might even be more compact).  I'm happy to reconsider that decision if the JSC folks feel differently now that we have more code that need to do scope.release() (with a lot more to come soon).

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:105
>> +        RETURN_IF_EXCEPTION(scope, nullptr);
> 
> I think a better translation here would be:
> RETURN_IF_EXCEPTION(scope, nullptr);
> ASSERT(!scope.exception());

I'll leave this for a future patch when I make fixes for exception checks (which includes re-arranging code like this).  Right now for this patch, I'm (mostly) only doing a straightforward text replacement of the old if statements with the new RETURN_IF_EXCEPTION() macro.

>> Source/JavaScriptCore/runtime/ProxyConstructor.cpp:73
>> +result->putDirect(vm, makeIdentifier(vm, "proxy"), proxy, None);
> 
> indentation is off.

Thanks.  Fixed.
Comment 6 Mark Lam 2016-09-26 12:14:37 PDT
Thanks for the reviews.  Landed in r206386: <http://trac.webkit.org/r206386>.