[webkit-reviews] review granted: [Bug 213757] [Win] Stub out bmalloc implementation : [Attachment 403150] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Don Olmstead
<don.olmstead at sony.com>'s request for review:
Bug 213757: [Win] Stub out bmalloc implementation
https://bugs.webkit.org/show_bug.cgi?id=213757

Attachment 403150: Patch

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




--- Comment #6 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 403150
  --> https://bugs.webkit.org/attachment.cgi?id=403150
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?