Bug 213757 – [Win] Stub out bmalloc implementation

• Component: bmalloc
• Status: NEW

[Don Olmstead]

Prepare bmalloc to be cross platform

[Don Olmstead]

Created attachment 403134 [details]
WIP Patch

[Don Olmstead]

Created attachment 403137 [details]
WIP Patch

[Don Olmstead]

Created attachment 403140 [details]
WIP Patch

[Don Olmstead]

Created attachment 403147 [details]
xcode

[Don Olmstead]

Created attachment 403150 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 403150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403150&action=review

r=me, but I think we should get Geoff's review too.

> Source/bmalloc/ChangeLog:13
> +        Add stubs for an implementation of bmalloc on Windows. Split the CryptoRandom
> +        implementation into a Unix and Windows definition. Add a small abstraction around
> +        threads to allow a Windows definition. Add the Builtins.h file which will contain
> +        any GCC compatible __builtin implementations on Windows.
> +
> +        The actual implementations will be coming in subsequent patches.

It is not enabled in Windows yet, right? Then, it is OK.

> Source/bmalloc/bmalloc/BAssert.h:70
> +    *(int *)(uintptr_t)0xbbadbeef = 0; \

Let's remove this change.

> Source/bmalloc/bmalloc/Builtins.h:41
> +{
> +    return 0;
> +}
> +
> +BINLINE int __builtin_usub_overflow(unsigned int a, unsigned int b, unsigned int *res)
> +{
> +    return 0;
> +}

Can you add FIXME?

> Source/bmalloc/bmalloc/DebugHeap.cpp:112
> +#if BOS(WINDOWS)
> +    result = _aligned_malloc(size, alignment);
> +    RELEASE_BASSERT(action == FailureAction::ReturnNull || result);
> +#else
>      if (posix_memalign(&result, alignment, size))
>          RELEASE_BASSERT(action == FailureAction::ReturnNull || result);
> +#endif

We should have FIXME to call _aligned_free since Windows requires paired calls of this instead of using free.

> Source/bmalloc/bmalloc/IsoTLS.cpp:38
> +#if BOS(WINDOWS)
> +#define strcasecmp _stricmp
> +#else
>  #include <stdio.h>
> +#endif

I think we can include `stdio.h` regardless of whether this is Windows or not.

> Source/bmalloc/bmalloc/VMAllocate.h:65
> +        // @TODO

Can you put FIXME comment with bugzilla URL about cache size retrieval?

> Source/bmalloc/bmalloc/VMAllocate.h:135
> +    // @TODO

Can you add FIXME comment with bugzilla URL about it?

> Source/bmalloc/bmalloc/VMAllocate.h:156
> +    // @TODO

Ditto.

> Source/bmalloc/bmalloc/VMAllocate.h:166
> +    // @TODO

Ditto.

> Source/bmalloc/bmalloc/VMAllocate.h:176
> +    // @TODO

Ditto.

> Source/bmalloc/bmalloc/VMAllocate.h:231
> +    // @TODO

Ditto.

> Source/bmalloc/bmalloc/VMAllocate.h:246
> +    // @TODO

Ditto.

> Source/bmalloc/bmalloc/win/CryptoRandomWin.cpp:31
> +{

Can you add a comment with FIXME?

[Geoffrey Garen]

Comment on attachment 403150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403150&action=review

> Source/bmalloc/bmalloc/DebugHeap.cpp:108
> +#if BOS(WINDOWS)
> +    result = _aligned_malloc(size, alignment);
> +    RELEASE_BASSERT(action == FailureAction::ReturnNull || result);

This is incorrect because it does not pair with _aligned_free.

I think Windows just shouldn't use the debug heap.

[Geoffrey Garen]

Generally, I don't love the idea of landing non-functional pieces of a port without testing. Is there a way to do this development locally or on a branch until it's more complete? Alternatively, do you have a complete patch that you can post for reference?