Bug 162521 – Added RETURN_IF_EXCEPTION() macro and use it for exception checks

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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()).

[Mark Lam]

Created attachment 289721 [details]
work in progress for EWS testing.

[Mark Lam]

Created attachment 289735 [details]
proposed patch.

[Saam Barati]

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.

[Geoffrey Garen]

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.

[Mark Lam]

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.

[Mark Lam]

Thanks for the reviews.  Landed in r206386: <http://trac.webkit.org/r206386>.