Summary: | [BigInt] BigInt.proptotype.toString is broken when radix is power of 2 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Lima <ticaiolima> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Caio Lima <ticaiolima> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, keith_miller, mark.lam, msaboff, realdawei, saam, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 190124 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Caio Lima
2018-09-27 04:16:15 PDT
Created attachment 350960 [details]
Patch
Created attachment 351012 [details]
Patch
Created attachment 351068 [details]
Patch
Comment on attachment 351068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351068&action=review r=me with nits. > Source/WTF/wtf/MathExtras.h:580 > +#if COMPILER(GCC_OR_CLANG) Use `COMPILER(GCC_COMPATIBLE)`. Created attachment 351216 [details]
Patch
Comment on attachment 351216 [details]
Patch
Thank you very much for the review!
Comment on attachment 351216 [details] Patch Clearing flags on attachment: 351216 Committed r236647: <https://trac.webkit.org/changeset/236647> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 190124 Created attachment 351242 [details]
Patch
(In reply to Caio Lima from comment #11) > Created attachment 351242 [details] > Patch What is the changed part from the previous one? (In reply to Yusuke Suzuki from comment #12) > (In reply to Caio Lima from comment #11) > > Created attachment 351242 [details] > > Patch > > What is the changed part from the previous one? ctz32 was wrong implemented. It was ``` if (!number) return __builtin_ctz(number); return 32; ``` Which return 32 for every number but 0. I changed to: ``` if (number) // No not "!" return __builtin_ctz(number); return 32; ``` Comment on attachment 351242 [details]
Patch
From previous patch, I changed ctz32 (Source/WTF/wtf/MathExtras.h:581) because previous version was wrong. Asking for review again before upstreaming the patch.
(In reply to WebKit Commit Bot from comment #7) > Comment on attachment 351216 [details] > Patch > > Clearing flags on attachment: 351216 > > Committed r236647: <https://trac.webkit.org/changeset/236647> 32bit JSC crashing after this revision. Sample output: https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/2647/steps/webkit-32bit-jsc-test/logs/stdio stress/big-int-to-string.js.big-int-enabled: 1 0x27dd2b WTFCrash stress/big-int-to-string.js.big-int-enabled: 2 0x280b3b WTF::CrashOnOverflow::crash() stress/big-int-to-string.js.big-int-enabled: 3 0x280b2b WTF::CrashOnOverflow::overflowed() stress/big-int-to-string.js.big-int-enabled: 4 0x2bf7f7 WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>::at(unsigned long) stress/big-int-to-string.js.big-int-enabled: 5 0x2bbcf4 WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>::operator[](unsigned long) stress/big-int-to-string.js.big-int-enabled: 6 0x12de7b1 JSC::JSBigInt::toStringBasePowerOfTwo(JSC::ExecState*, JSC::JSBigInt*, unsigned int) stress/big-int-to-string.js.big-int-enabled: 7 0x12de1a4 JSC::JSBigInt::toString(JSC::ExecState*, unsigned int) stress/big-int-to-string.js.big-int-enabled: 8 0x11ee681 JSC::bigIntProtoFuncToString(JSC::ExecState*) stress/big-int-to-string.js.big-int-enabled: 9 0x54e0f0e1 stress/big-int-to-string.js.big-int-enabled: 10 0x536057 llint_entry stress/big-int-to-string.js.big-int-enabled: 11 0x52febf vmEntryToJavaScript stress/big-int-to-string.js.big-int-enabled: 12 0xef1c49 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) stress/big-int-to-string.js.big-int-enabled: 13 0xef10a2 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) stress/big-int-to-string.js.big-int-enabled: 14 0x122b2eb JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) stress/big-int-to-string.js.big-int-enabled: 15 0x10889f runWithOptions(GlobalObject*, CommandLine&, bool&) stress/big-int-to-string.js.big-int-enabled: 16 0xd8c7a jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*, bool&) const stress/big-int-to-string.js.big-int-enabled: 17 0xba3aa int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&) stress/big-int-to-string.js.big-int-enabled: 18 0xb8b40 jscmain(int, char**) stress/big-int-to-string.js.big-int-enabled: 19 0xb8a67 main stress/big-int-to-string.js.big-int-enabled: 20 0xa73f4611 start stress/big-int-to-string.js.big-int-enabled: test_script_2250: line 2: 98930 Segmentation fault: 11 ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --validateExceptionChecks\=true --useDollarVM\=true --maxPerThreadStackUsage\=1572864 --useBigInt\=true --useFTLJIT\=true big-int-to-string.js ) stress/big-int-to-string.js.big-int-enabled: ERROR: Unexpected exit code: 139 Comment on attachment 351242 [details]
Patch
r=me
Comment on attachment 351242 [details]
Patch
Thx for the review
Comment on attachment 351242 [details] Patch Clearing flags on attachment: 351242 Committed r236737: <https://trac.webkit.org/changeset/236737> All reviewed patches have been landed. Closing bug. |