Fix std::make_unique / new[] using system malloc
Created attachment 334274 [details] Patch
Created attachment 334285 [details] Patch
Created attachment 334345 [details] Patch
Created attachment 334351 [details] Patch
Created attachment 334355 [details] Patch
Created attachment 334357 [details] Patch
Created attachment 334378 [details] Patch
Created attachment 334385 [details] Patch
Comment on attachment 334385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334385&action=review > Source/WTF/wtf/UniqueArray.h:44 > + // If it is acceptable, we can just use Vector<T> instead. So this UniqueArray<T> only > + // accepts the type T which does not have non trivial destructor. This allows us to > + // skip calling destructors for N elements. And this allows UniqueArray<T> not to > + // store its N size. Is it worth having a variant that handles non-trivial destructors? Presumably we want those going through bmalloc as well. "type T which does not have non trivial destructor" => "type T which has a trivial destructor"
Comment on attachment 334385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334385&action=review Thank you >> Source/WTF/wtf/UniqueArray.h:44 >> + // store its N size. > > Is it worth having a variant that handles non-trivial destructors? Presumably we want those going through bmalloc as well. > > "type T which does not have non trivial destructor" => "type T which has a trivial destructor" We can implement it, but in practice, I think it is not so useful. When the class has non-trivial destructors, in WebKit, typically, it is user-defined class. So annotating the class with WTF_MAKE_FAST_ALLOCATED can achieve what we want. What do you think of? I'll fix this typo. > Source/WTF/wtf/UniqueArray.h:49 > + // more larger storage than the `sizeof(U) * size` storage since it want to store Fix "more larger" => "larger"
Created attachment 334499 [details] Patch Update
Created attachment 334538 [details] Patch
Comment on attachment 334538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334538&action=review > Source/WTF/wtf/UniqueArray.h:97 > +} I've added UniqueArray<T> implementation for types which have non-trivial-destructors too.
Created attachment 334562 [details] Patch
Created attachment 334563 [details] Patch
Comment on attachment 334563 [details] Patch Attachment 334563 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6653356 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager
Created attachment 334564 [details] Patch
Comment on attachment 334385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334385&action=review >>> Source/WTF/wtf/UniqueArray.h:44 >>> + // store its N size. >> >> Is it worth having a variant that handles non-trivial destructors? Presumably we want those going through bmalloc as well. >> >> "type T which does not have non trivial destructor" => "type T which has a trivial destructor" > > We can implement it, but in practice, I think it is not so useful. > When the class has non-trivial destructors, in WebKit, typically, it is user-defined class. > So annotating the class with WTF_MAKE_FAST_ALLOCATED can achieve what we want. > What do you think of? > > I'll fix this typo. I don't follow why that would actually make that happen. Let's say I have a type T that is marked WTF_MAKE_FAST_ALLOCATED, and it has a non trivial destructor. Won't the compiler just end up emitting placement new? E.g, wouldn't std::unique_ptr<T[]>(size) just do something like: T* p = malloc(sizeof(T)*size); for ( i = 0; i < size; ++i) new (&p[i]) T;
Comment on attachment 334385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334385&action=review >>>> Source/WTF/wtf/UniqueArray.h:44 >>>> + // store its N size. >>> >>> Is it worth having a variant that handles non-trivial destructors? Presumably we want those going through bmalloc as well. >>> >>> "type T which does not have non trivial destructor" => "type T which has a trivial destructor" >> >> We can implement it, but in practice, I think it is not so useful. >> When the class has non-trivial destructors, in WebKit, typically, it is user-defined class. >> So annotating the class with WTF_MAKE_FAST_ALLOCATED can achieve what we want. >> What do you think of? >> >> I'll fix this typo. > > I don't follow why that would actually make that happen. Let's say I have a type T that is marked WTF_MAKE_FAST_ALLOCATED, and it has a non trivial destructor. Won't the compiler just end up emitting placement new? > > E.g, wouldn't std::unique_ptr<T[]>(size) just do something like: > T* p = malloc(sizeof(T)*size); > for ( i = 0; i < size; ++i) > new (&p[i]) T; No. When WTF_MAKE_FAST_ALLOCATED is annotated, the `new[]` operator (not `new` operator. WTF_MAKE_FAST_ALLOCATED defines `new` and `new[]`) is invoked, which is defined by WTF_MAKE_FAST_ALLOCATED. Since it allocates the memory from FastMalloc, we do not use system malloc for that. So, `make_unique<T[]>(size)` works with FastMalloc if T is annotated with WTF_MAKE_FAST_ALLOCATED. BTW, just for the usability, I added makeUniqueArray<T> for T which has a non trivial destructor in the latest patch.
Comment on attachment 334564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334564&action=review > Source/JavaScriptCore/runtime/StructureIDTable.cpp:36 > + , m_table(makeUniqueArray<StructureOrOffset>(s_initialSize)) Seems like a vector would be better here since we realloc below. > Source/WTF/wtf/UniqueArray.h:46 > + using U = typename std::remove_extent<T>::type; Why remove_extent? We don't use UniqueArray<T[]> right? Should it be an error to do so? > Source/WTF/wtf/UniqueArray.h:96 > + return UniqueArrayMaker<std::is_trivially_destructible<typename std::remove_extent<T>::type>::value, T>::make(size); Same here. You also remove_extent and it seems like it should just be an error to get it. So you need std::is_same<T, remove_extent<T>> > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5933 > + i = std::min(m_buffers.size() - 1, i); Can m_buffers.size() be 0? > Source/WebCore/platform/graphics/FormatConverter.h:49 > + m_unpackedIntermediateSrcData = makeUniqueArray<uint8_t>(m_width * MaxNumberOfComponents * MaxBytesPerComponent); Can this overflow? > Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:481 > + m_formalizedRGBA8Data = makeUniqueArray<uint8_t>(m_imageWidth * m_imageHeight * 4); Can this overflow? > Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:149 > + memcpy(attachmentInfo.data(), messageData, sizeof(AttachmentInfo) * attachmentCount); Can this overflow? > Tools/TestWebKitAPI/Tests/WTF/UniqueArray.cpp:33 > +static unsigned numberOfConstrucions { 0 }; typo "constructions" > Tools/TestWebKitAPI/Tests/WTF/UniqueArray.cpp:44 > + } This isn't trivial?
Comment on attachment 334564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334564&action=review Thanks. >> Source/JavaScriptCore/runtime/StructureIDTable.cpp:36 >> + , m_table(makeUniqueArray<StructureOrOffset>(s_initialSize)) > > Seems like a vector would be better here since we realloc below. This is intentional. In `StructureIDTable::resize` function, we first allocate the table pointer, fill the values, and perform `WTF::storeStoreFence`. If the table pointer is swapped before copying the values, we encounter the issue. (And this order depends on Vector's implementation. And we do not want to limit the Vector's implementation due to StructureIDTable implementaiton.). >> Source/WTF/wtf/UniqueArray.h:46 >> + using U = typename std::remove_extent<T>::type; > > Why remove_extent? We don't use UniqueArray<T[]> right? Should it be an error to do so? OK, I'll insert `static_assert` to ensure `T[]` is not coming here. >> Source/WTF/wtf/UniqueArray.h:96 >> + return UniqueArrayMaker<std::is_trivially_destructible<typename std::remove_extent<T>::type>::value, T>::make(size); > > Same here. You also remove_extent and it seems like it should just be an error to get it. So you need std::is_same<T, remove_extent<T>> Ditto. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5933 >> + i = std::min(m_buffers.size() - 1, i); > > Can m_buffers.size() be 0? It would be 0. However, in that case, we never call this `imageBuffer` function. >> Source/WebCore/platform/graphics/FormatConverter.h:49 >> + m_unpackedIntermediateSrcData = makeUniqueArray<uint8_t>(m_width * MaxNumberOfComponents * MaxBytesPerComponent); > > Can this overflow? It would be overflow. Using Checked<> and causing abort would be safer. >> Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:481 >> + m_formalizedRGBA8Data = makeUniqueArray<uint8_t>(m_imageWidth * m_imageHeight * 4); > > Can this overflow? Ditto. >> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:149 >> + memcpy(attachmentInfo.data(), messageData, sizeof(AttachmentInfo) * attachmentCount); > > Can this overflow? No. In that case, Vector already causes abort. >> Tools/TestWebKitAPI/Tests/WTF/UniqueArray.cpp:33 >> +static unsigned numberOfConstrucions { 0 }; > > typo "constructions" Fixed. >> Tools/TestWebKitAPI/Tests/WTF/UniqueArray.cpp:44 >> + } > > This isn't trivial? Oops, I need to swap `TrivialDestructor` and `NonTrivialDestructor` here.
Created attachment 334950 [details] Patch
Created attachment 334955 [details] Patch
Comment on attachment 334955 [details] Patch r=me
Comment on attachment 334955 [details] Patch Thanks!
Comment on attachment 334955 [details] Patch Rejecting attachment 334955 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 334955, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ile Source/WebKitLegacy/win/WebPreferences.cpp patching file Source/WebKitLegacy/win/WebView.cpp patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/TestWebKitAPI/CMakeLists.txt patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj patching file Tools/TestWebKitAPI/Tests/WTF/UniqueArray.cpp Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'JF Bastien']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/6814363
Comment on attachment 334955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334955&action=review > Source/WTF/wtf/Vector.h:85 > new (NotNull, cur) T; Oops, this can be used for POD, and at that time, value is not initialized since it is not `T()`. In `initialize()` function with `bool needsInitialization = true`, we should call `T()` instead of `T` since it *wants* initialization. This is discovered by UniqueArray test. Fixed.
Committed r229309: <https://trac.webkit.org/changeset/229309>
<rdar://problem/38169866>
Committed r229324: <https://trac.webkit.org/changeset/229324>