Bug 151079 – Create an FTLExceptionHandlerManager abstraction

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

[Saam Barati]

I think some of the exception handling state and code in FTLCompile can use a nice
abstraction or just a different file to live in.

[Saam Barati]

This should also handle the case where it shouldn't bother generating call site indices for tail calls.

[Saam Barati]

Created attachment 265212 [details]
patch

[WebKit Commit Bot]

Attachment 265212 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLExceptionHandlerManager.h:45:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLExceptionHandlerManager.h:54:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 265212 [details]
patch

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

r=me with comments.

> Source/JavaScriptCore/CMakeLists.txt:-993
> +        ftl/FTLDataSection.cpp
>          ftl/FTLDWARFDebugLineInfo.cpp
>          ftl/FTLDWARFRegister.cpp
> -        ftl/FTLDataSection.cpp

nit: Technically, FTLDWARFRegister.cpp should come before FTLDataSection.cpp according to ASCII sort order.  However, I see that the vcxproj files do it differently.  I'll let you decide on which is best.

> Source/JavaScriptCore/ftl/FTLExceptionHandlerManager.cpp:122
> +    if (!exit.m_exceptionHandlerCallSiteIndex)
> +        exit.m_exceptionHandlerCallSiteIndex = m_state.jitCode->common.addUniqueCallSiteIndex(origin);

This part is new, and not present in the original generateOrGetAlreadyGeneratedCallSiteIndex().  I see that this used to be assigned at line 613 in fixFunctionBasedOnStackMaps(), and it still is assigned there.  Is there a reason that we need to have a separate assignment here?  Can it be one or the other (and not both)?

[Saam Barati]

Comment on attachment 265212 [details]
patch

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

>> Source/JavaScriptCore/ftl/FTLExceptionHandlerManager.cpp:122
>> +        exit.m_exceptionHandlerCallSiteIndex = m_state.jitCode->common.addUniqueCallSiteIndex(origin);
> 
> This part is new, and not present in the original generateOrGetAlreadyGeneratedCallSiteIndex().  I see that this used to be assigned at line 613 in fixFunctionBasedOnStackMaps(), and it still is assigned there.  Is there a reason that we need to have a separate assignment here?  Can it be one or the other (and not both)?

You're right.
We should never need to generate one here.
I'll just assert that we have a valid callsiteindex here.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/192273