[Webkit-unassigned] [Bug 213757] [Win] Stub out bmalloc implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=213757

Yusuke Suzuki <ysuzuki at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #403150|review?                     |review+
              Flags|                            |

--- 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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200630/4f90b6e6/attachment.htm>