[webkit-reviews] review granted: [Bug 185110] Apply PtrTags to the MetaAllocator and friends. : [Attachment 339061] proposed patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 30 11:23:55 PDT 2018


Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 185110: Apply PtrTags to the MetaAllocator and friends.
https://bugs.webkit.org/show_bug.cgi?id=185110

Attachment 339061: proposed patch.

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




--- Comment #2 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 339061
  --> https://bugs.webkit.org/attachment.cgi?id=339061
proposed patch.

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

r=me

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:306
> +#else // !(CPU(ARM64) && USE(EXECUTE_ONLY_JIT_WRITE_FUNCTION))

Why this change? I thought we typically annotate w/ the identical condition,
not its opposite?

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:445
> +    RELEASE_ASSERT(start < resultEnd && resultEnd <= end);

I guess it's ok to we assume we always generate > 0 bytes of code?

> Source/WTF/wtf/MetaAllocator.cpp:342
> +	       ASSERT(leftNode->m_start + (leftNode->sizeInBytes() +
sizeInBytes + rightSize) == rightEnd);

Why parenthesis here?

> Source/WTF/wtf/MetaAllocator.cpp:350
> +	       leftNode->m_end += (sizeInBytes + rightSize);

ditto

> Source/WTF/wtf/MetaAllocator.cpp:368
> +	       ASSERT(start + (sizeInBytes + rightNode->sizeInBytes()) ==
rightNode->m_end);

ditto

> Source/WTF/wtf/MetaAllocator.cpp:383
> +	       node->m_end = start + sizeInBytes;

And you don't use them here. I'd just remove it everywhere since addition is
commutative.

> Source/WTF/wtf/MetaAllocator.h:141
> +	       return m_end.untaggedPtr<size_t>() -
m_start.untaggedPtr<size_t>();

This should be untaggedPtr<uintptr_t> or untaggedPtr<intptr_t> IMO

> Source/WTF/wtf/MetaAllocatorPtr.h:35
> +class MetaAllocatorPtr {

This is almost identical to MacroAssemblerCodePtr. They should probably inherit
from some common subclass. At least file a bug w/ a FIXME for this work.


More information about the webkit-reviews mailing list