Bug 132755 – WebKit2 on iOS needs to capture the main thread's floating point environment

• Component: WebKit2
• Status: RESOLVED FIXED

[Mark Lam]

For iOS, WorkerThread::workerThread()  expects to be able to initialize the worker thread's floating point environment to be the same as the one in the main thread.  The FP env of the main thread is expected to have been captured in the mainThreadFEnv global.  On WebKit2 for iOS, we failed to initialize mainThreadFEnv.

[Mark Lam]

ref: <rdar://problem/16413527>

[Mark Lam]

Created attachment 231257 [details]
the patch.

This patch has been tested on:
1. OSX x86_64 running the layout tests
2. The iOS simulator running modified versions of the 2 regression tests served up by a web server.
3. an ARMv7 iOS build running modified versions of the 2 regression tests served up by a web server.

Before this fix, the iOS simulator would crash, and the ARMv7 iOS run would yield the wrong answer for the js/floating-point-denormalized.html test.  With this fix, the iOS simulator no longer crashes, and the ARMv7 run produces the correct answer.

[Simon Fraser (smfr)]

Comment on attachment 231257 [details]
the patch.

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

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:84
> +    WebThreadCaptureMainThreadFloatingPointEnvironment();

Can we rename (or factor) this so that we don't have WK2 calling something that starts with "WebThread"?

[Geoffrey Garen]

Comment on attachment 231257 [details]
the patch.

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

> Source/WebCore/platform/ios/wak/WebCoreThread.h:73
> +void WebThreadCaptureMainThreadFloatingPointEnvironment();

I agree with Simon. In addition to the naming issue, this function doesn't belong in WebCoreThread.h, since it has very little to do with the web thread, and it is required in environments that don't use the web thread and should not #include this header.

If we're going to share this code, how about a new file with some abstraction help for sharing:

FloatingPoint.h:

class FloatingPoint {
public:
    void enableDenormals(); // Enables denormals on ARMv7. ASSERTs that denormals are enabled. Called only by WebKit2.
    void saveMainThreadEnvironment(); // ASSERTs that we're on the main thread and m_mainThreadEnvironment is 0. Initializes m_mainThreadEnvironment.
    void propogateMainThreadEnvironment(); // ASSERTs that we're not on the main thread. Does the setenv for a secondary thread. 

private:
    fenv_t m_mainThreadEnvironment;
};

FloatingPoint& floatingPoint()
{
  static NeverDestroyed<FloatingPoint> floatingPoint;
  return floatingPoint;
}

[Mark Lam]

Created attachment 231321 [details]
patch 2: applied suggested feedback

[Mark Lam]

Created attachment 231324 [details]
patch 3: removed an unneeded #import <fenv.h> in WebCoreThread.h

[Geoffrey Garen]

Comment on attachment 231324 [details]
patch 3: removed an unneeded #import <fenv.h> in WebCoreThread.h

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

> Source/WebCore/platform/ios/wak/FloatingPointEnv.cpp:47
> +void FloatingPointEnv::enableNeededFloatingPointModes()

This is a needlessly abstract name. Also, it's a false name: This function enables only one mode. Notice that, due to this bad name, you had to add a comment in your change log explaining that you called this function "to support denormalized numbers". A better way to indicate that is to put it in the function's name.

> Source/WebCore/platform/ios/wak/FloatingPointEnv.cpp:54
> +    env.__fpscr &= ~0x01000000U; // Enable denormalized numbers.

This comment duplicates the comment above it.

You should add a comment explaining that this state is the default on x86_64 and ARM64.

> Source/WebCore/platform/ios/wak/FloatingPointEnv.cpp:59
> +    RELEASE_ASSERT((env.__mxcsr & 0x1f80) == 0x1f80); // Ensure FP exceptions are disabled.

Let's not ASSERT this. This ASSERT would not have caught our bug because it only run on the main thread, and it run before a secondary thread might read the relevant data.

Also, though it would be strange, we can't guarantee that a WebKit1 client process did not enable FP exceptions. Perhaps they did.

I would suggest reworking this ASSERT, but it's actually already better covered by the m_isInitialized ASSERT, since that tests directly for passing uninitialized data to fsetenv. So, let's just remove this ASSERT.

> Source/WebCore/platform/ios/wak/FloatingPointEnv.h:35
> +class FloatingPointEnv {

I think we can afford to spell out "Environment" here. Also, you used "Environment" in the static functions below, and consistency is a goal.

> Source/WebCore/platform/ios/wak/FloatingPointEnv.h:41
> +    static void enableNeededFloatingPointModes();
> +    static void saveMainThreadEnvironment();
> +    static void propagateMainThreadEnvironment();

These functions should be member functions, and the accessor to the shared floating point environment object should be public.

It doesn't make sense to mix all static functions with per-object data.

[Geoffrey Garen]

Comment on attachment 231324 [details]
patch 3: removed an unneeded #import <fenv.h> in WebCoreThread.h

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

>> Source/WebCore/platform/ios/wak/FloatingPointEnv.cpp:54
>> +    env.__fpscr &= ~0x01000000U; // Enable denormalized numbers.
> 
> This comment duplicates the comment above it.
> 
> You should add a comment explaining that this state is the default on x86_64 and ARM64.

It would be even better to ASSERT on those platforms that denormal support was enabled.

[Mark Lam]

Created attachment 231328 [details]
patch 4: addressed Geoff's feedback and fix EFL build failure.

[Geoffrey Garen]

Comment on attachment 231328 [details]
patch 4: addressed Geoff's feedback and fix EFL build failure.

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

r=me

> Source/WebCore/platform/ios/wak/FloatingPointEnvironment.cpp:39
> +    static NeverDestroyed<FloatingPointEnvironment> floatingPointEnv;
> +    return floatingPointEnv;

Env => Environment

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:99
> +    FloatingPointEnvironment& fpenv = FloatingPointEnvironment::shared();
> +    fpenv.enableDenormalSupport();
> +    fpenv.saveMainThreadEnvironment();

fpenv => floatingPointEnvironment

[Mark Lam]

Thanks.  The patch has been updated and landed in r168676: <http://trac.webkit.org/r168676>.