1606568 - No public API for creating/managing BigInt values.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Evan Welsh [:ewlsh]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.88 Safari/537.36

Steps to reproduce:

1. Attempt to create a BigInt value from C++ data type (uint64_t, int64_t)
2. Attempt to retrieve the C++ value from a BigInt

Actual results:

There is no publicly available API to do so.

Expected results:

Similar to how Symbol exposes the necessary functions for working with it (https://searchfox.org/mozilla-central/source/js/public/Symbol.h#32 and https://searchfox.org/mozilla-central/source/js/src/jsapi.cpp#4485), embedders need an exposed API to work with BigInts.

Ideally, at least BigInt::fromInt64, BigInt::fromUint64, BigInt::toUint64, and BigInt::toInt64 would be exposed (from https://searchfox.org/mozilla-central/source/js/src/vm/BigIntType.cpp#1751)

Comment 1

[Jeff Walden [:Waldo]]

We should have a js/public/BigInt.h similar to the existing js/public/Symbol.h.

There should be JS::BigInt* JS::NumberToBigInt(JSContext* cx, <numeric>) for all integral types (excepting bool) and all floating-point types. The integral type overloads will fail only for OOM, the floating-point will fail when the input isn't integral, consistent with the spec operation. (Assuming it hasn't changed name in the spec, I'm looking at the proposal text when I write this.) This should be implemented as one inline-defined template function, that passes along to a JS::detail template class, specialized for floating-point and bool overloads, with static member function do actually do the work -- again, inline -- by calling an exact narrow function.

There should also be an overload that accepts const char*/size_t as pure digit information and/or one that permits syntactically-valid use of numeric separators. I think requiring no separators is a better public API (and a separator-permitting overload would be an optional addition, clearly distinguished by name from the other). But in our implementation I don't know if we have an API that would implement that restriction, and it's not worth the trouble to add one IMO if we can't do it easily -- so just expose whatever's easiest for us.

JS::BigInt* JS::ToBigInt(JSContext* cx, JS::Handle<JS::Value>) should be added.

ToBigInt64 and ToBigUint64 should be exposed, accepting JS::BigInt*; because of the limited type accepted, these can be infallible.

JS::ToNumber already handles inverse conversions, so we don't need anything like that.

I think that's an adequate start, no need to write it all out all at once. It is probably desirable to expose all the mathematical operations as functions, too, taking JSContext* cx and one or two JS::Handle<JS::BigInt*>, but let's do the easiest things first and leave the full suite of operations to a followup.

Comment 2

[Philip Chimento [:ptomato]]

Attachment: Bug 1606568 - Prevent excessive instantiation of BigInt parsing template. r?jwalden

Otherwise, when instantiating the template for a signed type such as
const char, we will trip up the compiler with a signed/unsigned
comparison below.

Comment 3

[Philip Chimento [:ptomato]]

Attachment: Bug 1606568 - Add public BigInt API. r?jwalden

Depends on D82479

Comment 4

[Philip Chimento [:ptomato]]

Hopefully this is good for a first set of APIs to expose.

Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f59a9aa92d42cc8f19d2d9ba962310a555fdc4 Let me know if there are other jobs this should be tried on.

Comment 5

[Philip Chimento [:ptomato]]

Attachment: Bug 1606568 - Add JS::ConstLatin1Chars. r?jwalden

Useful for the public BigInt API which takes Range<const CharT>. Allows

JS::NumberToBigInt(JSContext*, JS::ConstLatin1Chars)

in order to match

JS::NumberToBigInt(JSContext*, JS::ConstTwoByteChars)

which already exists.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7890c93298ec
Prevent excessive instantiation of BigInt parsing template. r=jwalden

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/747640138ffb
Add JS::ConstLatin1Chars. r=jwalden

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/835cceddaf49
Add public BigInt API. r=jwalden

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 3 changesets (bug 1606568) for estBigInt.cpp related bustage and BigIntType.cpp related hazard

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=cd5b31b3b27ceac4815595b6b48e286b2c56a594&searchStr=build&selectedTaskRun=da3XaG5BTbCaDrDOlqCc4g.0&tochange=5f9a6b8dc5f2ad38bf1a7d3bc87f5f537c3b0445

Backout link: https://hg.mozilla.org/integration/autoland/rev/5f9a6b8dc5f2ad38bf1a7d3bc87f5f537c3b0445

Failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310492630&repo=autoland&lineNumber=24561

[task 2020-07-21T01:43:27.724Z] 01:43:27     INFO -  make[4]: Entering directory '/builds/worker/workspace/obj-build/js/src/jsapi-tests'
[task 2020-07-21T01:43:27.727Z] 01:43:27     INFO -  /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 -o Unified_cpp_js_src_jsapi-tests1.o -c  -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DDEBUG=1 -DMOZ_HAS_MOZGLUE -DEXPORT_JS_API -Dtopsrcdir=/builds/worker/checkouts/gecko/js/src -I/builds/worker/checkouts/gecko/js/src/jsapi-tests -I/builds/worker/workspace/obj-build/js/src/jsapi-tests -I/builds/worker/workspace/obj-build/js/src -I/builds/worker/checkouts/gecko/js/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/js/src/js-confdefs.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Werror=implicit-function-declaration -Wno-noexcept-type -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -O3 -fno-omit-frame-pointer -funwind-tables -Werror -fno-strict-aliasing -Werror=format -Wno-shadow  -MD -MP -MF .deps/Unified_cpp_js_src_jsapi-tests1.o.pp   Unified_cpp_js_src_jsapi-tests1.cpp
[task 2020-07-21T01:43:27.728Z] 01:43:27     INFO -  In file included from Unified_cpp_js_src_jsapi-tests1.cpp:20:
[task 2020-07-21T01:43:27.729Z] 01:43:27    ERROR -  /builds/worker/checkouts/gecko/js/src/jsapi-tests/testBigInt.cpp:159:18: error: no viable constructor or deduction guide for deduction of template arguments of 'Range'
[task 2020-07-21T01:43:27.729Z] 01:43:27     INFO -    mozilla::Range input{mozilla::MakeStringSpan(u"18446744073709551616")};
[task 2020-07-21T01:43:27.729Z] 01:43:27     INFO -                   ^
[task 2020-07-21T01:43:27.730Z] 01:43:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Range.h:46:16: note: candidate template ignored: couldn't infer template argument 'T'
[task 2020-07-21T01:43:27.731Z] 01:43:27     INFO -    MOZ_IMPLICIT Range(const Span<U>& aSpan)
[task 2020-07-21T01:43:27.731Z] 01:43:27     INFO -                 ^
[task 2020-07-21T01:43:27.731Z] 01:43:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Range.h:39:16: note: candidate template ignored: could not match 'Range' against 'Span'
[task 2020-07-21T01:43:27.731Z] 01:43:27     INFO -    MOZ_IMPLICIT Range(const Range<U>& aOther)
[task 2020-07-21T01:43:27.731Z] 01:43:27     INFO -                 ^
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Range.h:42:16: note: candidate template ignored: substitution failure [with T = const char16_t]
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -    MOZ_IMPLICIT Range(Span<T> aSpan) : Range(aSpan.Elements(), aSpan.Length()) {}
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -                 ^
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Range.h:20:7: note: candidate template ignored: could not match 'Range' against 'Span'
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -  class Range {
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -        ^
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Range.h:25:3: note: candidate function template not viable: requires 0 arguments, but 1 was provided
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -    Range() : mStart(nullptr, 0), mEnd(nullptr, 0) {}
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -    ^
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Range.h:26:3: note: candidate function template not viable: requires 2 arguments, but 1 was provided
[task 2020-07-21T01:43:27.734Z] 01:43:27     INFO -    Range(T* aPtr, size_t aLength)
[task 2020-07-21T01:43:27.735Z] 01:43:27     INFO -    ^
[task 2020-07-21T01:43:27.735Z] 01:43:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Range.h:29:3: note: candidate function template not viable: requires 2 arguments, but 1 was provided
[task 2020-07-21T01:43:27.735Z] 01:43:27     INFO -    Range(const RangedPtr<T>& aStart, const RangedPtr<T>& aEnd)
[task 2020-07-21T01:43:27.735Z] 01:43:27     INFO -    ^
[task 2020-07-21T01:43:27.735Z] 01:43:27     INFO -  1 error generated.
[task 2020-07-21T01:43:27.735Z] 01:43:27     INFO -  /builds/worker/checkouts/gecko/config/rules.mk:746: recipe for target 'Unified_cpp_js_src_jsapi-tests1.o' failed
[task 2020-07-21T01:43:27.736Z] 01:43:27    ERROR -  make[4]: *** [Unified_cpp_js_src_jsapi-tests1.o] Error 1
[task 2020-07-21T01:43:27.736Z] 01:43:27     INFO -  make[4]: Leaving directory '/builds/worker/workspace/obj-build/js/src/jsapi-tests'
[task 2020-07-21T01:43:27.736Z] 01:43:27     INFO -  make[4]: *** Waiting for unfinished jobs....

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310492682&repo=autoland&lineNumber=6518

[task 2020-07-21T01:44:46.112Z] Running explain to generate ('hazards.txt', 'unnecessary.txt', 'refs.txt')
[task 2020-07-21T01:44:46.112Z] LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/builds/worker/workspace/obj-haz-shell/dist/bin" PATH="/builds/worker/fetches/sixgill/usr/bin:${PATH}" XDB='/builds/worker/fetches/sixgill/usr/bin/xdb.so' SOURCE='/builds/worker/checkouts/gecko' ANALYZED_OBJDIR='/builds/worker/workspace/obj-analyzed' python2.7 /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/explain.py rootingHazards.txt gcFunctions.txt hazards.txt unnecessary.txt refs.txt
[task 2020-07-21T01:44:46.209Z] Wrote explained_hazards.tmp
[task 2020-07-21T01:44:46.209Z] Wrote unnecessary.tmp
[task 2020-07-21T01:44:46.209Z] Wrote refs.tmp
[task 2020-07-21T01:44:46.209Z] Found 3 hazards 74 unsafe references 0 missing
[task 2020-07-21T01:44:46.210Z] Running heapwrites to generate heapWriteHazards.txt
[task 2020-07-21T01:44:46.210Z] LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/builds/worker/workspace/obj-haz-shell/dist/bin" PATH="/builds/worker/fetches/sixgill/usr/bin:${PATH}" XDB='/builds/worker/fetches/sixgill/usr/bin/xdb.so' SOURCE='/builds/worker/checkouts/gecko' ANALYZED_OBJDIR='/builds/worker/workspace/obj-analyzed' /builds/worker/workspace/obj-haz-shell/dist/bin/js /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/analyzeHeapWrites.js > heapWriteHazards.txt
[task 2020-07-21T01:44:49.414Z] + check_hazards /builds/worker/workspace/analysis
[task 2020-07-21T01:44:49.415Z] + set +e
[task 2020-07-21T01:44:49.415Z] ++ grep -c 'Function.*has unrooted.*live across GC call' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2020-07-21T01:44:49.416Z] + NUM_HAZARDS=3
[task 2020-07-21T01:44:49.417Z] ++ grep -c '^Function.*takes unsafe address of unrooted' /builds/worker/workspace/analysis/refs.txt
[task 2020-07-21T01:44:49.418Z] + NUM_UNSAFE=74
[task 2020-07-21T01:44:49.418Z] ++ grep -c '^Function.* has unnecessary root' /builds/worker/workspace/analysis/unnecessary.txt
[task 2020-07-21T01:44:49.420Z] + NUM_UNNECESSARY=1124
[task 2020-07-21T01:44:49.420Z] ++ grep -c '^Dropped CFG' /builds/worker/workspace/analysis/build_xgill.log
[task 2020-07-21T01:44:49.422Z] + NUM_DROPPED=0
[task 2020-07-21T01:44:49.422Z] ++ perl -lne 'print $1 if m!found (\d+)/\d+ allowed errors!' /builds/worker/workspace/analysis/heapWriteHazards.txt
[task 2020-07-21T01:44:49.424Z] + NUM_WRITE_HAZARDS=0
[task 2020-07-21T01:44:49.424Z] ++ grep -c '^Function.*expected hazard.*but none were found' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2020-07-21T01:44:49.425Z] + NUM_MISSING=0
[task 2020-07-21T01:44:49.425Z] + set +x
[task 2020-07-21T01:44:49.425Z] TinderboxPrint: rooting hazards<br/>3
[task 2020-07-21T01:44:49.425Z] TinderboxPrint: (unsafe references to unrooted GC pointers)<br/>74
[task 2020-07-21T01:44:49.425Z] TinderboxPrint: (unnecessary roots)<br/>1124
[task 2020-07-21T01:44:49.425Z] TinderboxPrint: missing expected hazards<br/>0
[task 2020-07-21T01:44:49.425Z] TinderboxPrint: heap write hazards<br/>0
[task 2020-07-21T01:44:49.426Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'bi' of type 'JS::BigInt*' live across GC call at js/src/vm/BigIntType.cpp:3794
[task 2020-07-21T01:44:49.426Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'bi' of type 'JS::BigInt*' live across GC call at js/src/vm/BigIntType.cpp:3743
[task 2020-07-21T01:44:49.426Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'bi' of type 'JS::BigInt*' live across GC call at js/src/vm/BigIntType.cpp:3743
[task 2020-07-21T01:44:49.426Z] TEST-UNEXPECTED-FAIL | hazards | 3 rooting hazards detected
[task 2020-07-21T01:44:49.426Z] TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit "Inspect Task" link for hazard details
[task 2020-07-21T01:44:49.427Z] + onexit

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

The following seems to start with the backed out changes:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310494699&repo=autoland&lineNumber=2485

Comment 11

[Jeff Walden [:Waldo]]

Looks like I missed an un-parametrized Range variable in the test. And apparently BigInt* bi = ...; if (haveParseError) { JS_ReportErrorNumber(...); } return bi; is a hazard, because the code doesn't understand that haveParseError implies bi == nullptr. I guess parametrize the one, and change the other cases to the pattern used by js::ParseBigIntLiteral (although I'd say leave out the is-tenured assertion, which is neither obvious nor something we intend to promise to users of these functions).

Comment 12

[Philip Chimento [:ptomato]]

Oops. I will take a look at these. I wonder what incantations I should be adding to "mach try fuzzy" so that this doesn't happen in the future...

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eefb0b6651d7
Add JS::ConstLatin1Chars. r=jwalden

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18a0675bd755
Prevent excessive instantiation of BigInt parsing template. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/5bbe7e5166e7
Add public BigInt API. r=jwalden

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Philip Chimento [:ptomato] from comment #12)

Oops. I will take a look at these. I wonder what incantations I should be adding to "mach try fuzzy" so that this doesn't happen in the future...

A good starting point for SpiderMonkey changes is ./mach try --preset sm-shell. That has the SpiderMonkey builds including the rooting analysis.

Comment 16

[Philip Chimento [:ptomato]]

Oh, the --preset argument looks like it'll be quite useful, thanks.

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/eefb0b6651d7
https://hg.mozilla.org/mozilla-central/rev/18a0675bd755
https://hg.mozilla.org/mozilla-central/rev/5bbe7e5166e7

Comment 18

[Philip Chimento [:ptomato]]

Does it seem feasible that we could backport this to esr78 so embedders could use it without waiting until esr88 next year?

Comment 19

[Philip Chimento [:ptomato]]

Comment on attachment 9161795 [details]
Bug 1606568 - Prevent excessive instantiation of BigInt parsing template. r?jwalden

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch series exposes a public API for embedders to do operations with BigInt. Without it, using BigInt functionality from C++ isn't possible for software that embeds SpiderMonkey.
• User impact if declined: No impact to Firefox. Embedders will probably need to wait for the next ESR upgrade if they want to implement BigInt functionality, as it's unlikely that downstream Linux distributions would carry this patch.
• Fix Landed on Version: 80
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I believe this is low risk because the API isn't used within Firefox, so any problems ought to be evident at compile time.
• String or UUID changes made by this patch: None

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9161795 [details]
Bug 1606568 - Prevent excessive instantiation of BigInt parsing template. r?jwalden

NPOTB for Firefox but makes life better for SM embedders. Approved for 78.2esr.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/4c5276b58f50
https://hg.mozilla.org/releases/mozilla-esr78/rev/db4ac813e98b
https://hg.mozilla.org/releases/mozilla-esr78/rev/a5bcbd34ea66