Bug 182975 – Fix std::make_unique / new[] using system malloc

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Fix std::make_unique / new[] using system malloc

[Yusuke Suzuki]

Created attachment 334274 [details]
Patch

[Yusuke Suzuki]

Created attachment 334285 [details]
Patch

[Yusuke Suzuki]

Created attachment 334345 [details]
Patch

[Yusuke Suzuki]

Created attachment 334351 [details]
Patch

[Yusuke Suzuki]

Created attachment 334355 [details]
Patch

[Yusuke Suzuki]

Created attachment 334357 [details]
Patch

[Yusuke Suzuki]

Created attachment 334378 [details]
Patch

[Yusuke Suzuki]

Created attachment 334385 [details]
Patch

[Saam Barati]

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"

[Yusuke Suzuki]

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"

[Yusuke Suzuki]

Created attachment 334499 [details]
Patch

Update

[Yusuke Suzuki]

Created attachment 334538 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 334562 [details]
Patch

[Yusuke Suzuki]

Created attachment 334563 [details]
Patch

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 334564 [details]
Patch

[Saam Barati]

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;

[Yusuke Suzuki]

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.

[JF Bastien]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 334950 [details]
Patch

[Yusuke Suzuki]

Created attachment 334955 [details]
Patch

[JF Bastien]

Comment on attachment 334955 [details]
Patch

r=me

[Yusuke Suzuki]

Comment on attachment 334955 [details]
Patch

Thanks!

[WebKit Commit Bot]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r229309: <https://trac.webkit.org/changeset/229309>

[Radar WebKit Bug Importer]

<rdar://problem/38169866>

[Yusuke Suzuki]

Committed r229324: <https://trac.webkit.org/changeset/229324>