BigInt should store its data in the primitive gigacage.
Created attachment 363098 [details] Patch
Created attachment 363099 [details] Patch
Created attachment 363100 [details] Patch
Comment on attachment 363100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363100&action=review > Source/JavaScriptCore/runtime/JSBigInt.h:245 > + CagedBarrierPtr<Gigacage::Primitive, Digit> m_data; Do we need to use CagedBarrierPtr instead of CagedPtr here?
Comment on attachment 363100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363100&action=review > Source/JavaScriptCore/runtime/JSBigInt.cpp:74 > + Gigacage::free(Gigacage::Primitive, thisObject->dataStorage()); If we use CagedUniquePtr, we do not need to call this in JSBigInt::destroy. We can call thisObject-~JSBigInt().
Comment on attachment 363100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363100&action=review >> Source/JavaScriptCore/runtime/JSBigInt.cpp:74 >> + Gigacage::free(Gigacage::Primitive, thisObject->dataStorage()); > > If we use CagedUniquePtr, we do not need to call this in JSBigInt::destroy. We can call thisObject-~JSBigInt(). Ohh, nice, I'll change.
Created attachment 363244 [details] Patch
Comment on attachment 363244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363244&action=review > Source/JavaScriptCore/runtime/JSBigInt.cpp:119 > JSBigInt* JSBigInt::createWithLengthUnchecked(VM& vm, unsigned length) > { > ASSERT(length <= maxLength); > - JSBigInt* bigInt = new (NotNull, allocateCell<JSBigInt>(vm.heap, allocationSize(length))) JSBigInt(vm, vm.bigIntStructure.get(), length); > + void* data = Gigacage::tryMalloc(Gigacage::Primitive, length * sizeof(Digit)); > + if (!data) > + return nullptr; > + > + JSBigInt* bigInt = new (NotNull, allocateCell<JSBigInt>(vm.heap)) JSBigInt(vm, vm.bigIntStructure.get(), reinterpret_cast<Digit*>(data), length); > bigInt->finishCreation(vm); > return bigInt; > } The point of this function is that we don't return nullptr. So this shouldn't be using tryMalloc. Perhaps somewhere earlier we should be tryMallocing and throwing an OOM? If you grep for callers of this, you'll see that we just assume it's non-null.
Created attachment 383732 [details] Patch
Comment on attachment 383732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383732&action=review r=me with fix. > Source/JavaScriptCore/runtime/JSBigInt.cpp:111 > + ASSERT(data); This is not strictly needed because malloc() should crash on failure to allocate, but I suppose it doesn't hurt. > Source/JavaScriptCore/runtime/JSBigInt.h:56 > JSBigInt(VM&, Structure*, unsigned length); Shouldn't this line be removed? This constructor doesn't exist anymore.
Comment on attachment 383732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383732&action=review Thanks! >> Source/JavaScriptCore/runtime/JSBigInt.cpp:111 >> + ASSERT(data); > > This is not strictly needed because malloc() should crash on failure to allocate, but I suppose it doesn't hurt. Yeah, removing this is also OK. >> Source/JavaScriptCore/runtime/JSBigInt.h:56 >> JSBigInt(VM&, Structure*, unsigned length); > > Shouldn't this line be removed? This constructor doesn't exist anymore. Yes, we should remove it. Fixed.
Committed r252538: <https://trac.webkit.org/changeset/252538>
<rdar://problem/57274577>