[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