1823880 - Make fdlibm (sin/cos/tan) fingerprinting resistance precise

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tom Schuster]

Right now we just globally switch between the plaform libm or fdlibm with JS::SetUseFdlibmForSinCosTan. Maybe there are already plans for switching to fdlibm by default? Or maybe we decide that using fdlibm is good enough when RFP (lite?) is enabled.

Making this per realm might again involve some amount of work, because it seems like the various js::math_cos_impl functions don't receive any kind of context.

Comment 1

[André Bargull [:anba]]

(In reply to Tom Schuster (MoCo) from comment #0)

Maybe there are already plans for switching to fdlibm by default?

We don't use fdlibm's implementation for sin/cos/tan, because it's too slow (see bug 933257). V8 also recently switched their implementation from fdlibm to glibc to improve the performance of sin/cos: https://bugs.chromium.org/p/v8/issues/detail?id=13477.

Comment 2

[Tom Ritter [:tjr]]

Don't we have the JSContext right above the _impl function calls? I'm probably missing some other callpath.

In any event, I don't think this is something we need to make precise. I think if RFP or RFP Lite is enabled, we can use fdlibm; and for RFPLite we can make a RFPTarget that would allow us to remove it from RFPLite if necessary. Removing it from RFPLite via Bug 1824222 would require some special handling over there, but should be doable.

Comment 3

[Tom Schuster]

I think at least here we don't have the context, but I haven't traced out the call path yet.

In any event, I don't think this is something we need to make precise. I think if RFP or RFP Lite is enabled, we can use fdlibm; and for RFPLite we can make a RFPTarget that would allow us to remove it from RFPLite if necessary. Removing it from RFPLite via Bug 1824222 would require some special handling over there, but should be doable.

Sounds good. I agree that a slightly slower math function is not a problem we need to solve soon.

Comment 4

[Tom Schuster]

Making fdlibm togglable individually and not just based on general RFP state would require some additional plumbing similar to RealmBehaviors::clampAndJitterTime. I am not sure how useful that is. I think the fdlibm and native implementations only differ in some edge cases, so the potential for compatibility issues seems low.

Comment 5

[Tom Schuster]

Attachment: Bug 1823880 - Select fdlibm version of Math builtins dynamically. r?#spidermonkey-reviewers

Comment 6

[Tom Schuster]

Attachment: Bug 1823880 - Add wasm builtin thunk versions for fdlibm. r?rhunt

Depends on D174264

Comment 7

[Tom Schuster]

Attachment: Bug 1823880 - Shell testcase. r?#spidermonkey-reviewers

Depends on D174265

Comment 8

[Tom Schuster]

https://treeherder.mozilla.org/jobs?repo=try&revision=2f063016351847735bd1708a9572cbc64af95ce0

Comment 9

[Tom Schuster]

Attachment: Bug 1823880 - Update browser test. r?tjr

Depends on D174266

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ae27d2b4eff
Select fdlibm version of Math builtins dynamically. r=nbp,spidermonkey-reviewers
https://hg.mozilla.org/integration/autoland/rev/9b5be2492da3
Add wasm builtin thunk versions for fdlibm. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/64636336485c
Shell testcase. r=spidermonkey-reviewers,iain
https://hg.mozilla.org/integration/autoland/rev/c08b278ee06e
Update browser test. r=tjr

Comment 11

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6ae27d2b4eff
https://hg.mozilla.org/mozilla-central/rev/9b5be2492da3
https://hg.mozilla.org/mozilla-central/rev/64636336485c
https://hg.mozilla.org/mozilla-central/rev/c08b278ee06e