Bug 185110 – Apply PtrTags to the MetaAllocator and friends.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

<rdar://problem/39533895>

[Mark Lam]

Created attachment 339061 [details]
proposed patch.

[Saam Barati]

Comment on attachment 339061 [details]
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.

[Mark Lam]

Comment on attachment 339061 [details]
proposed patch.

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

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

I'll revert this.  I still think our typical convention is confusing to annotate a block of code with the opposite of what it contains.

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

I think it doesn't make sense to ask the allocator for 0 bytes.

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

I already talked to Saam about this offline: the reason for the parens is so explicitly communicate to the compiler that we want the scalar operations to be executed first, and then only invoke MetaAllocatorPtr::operator+ once.

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

There's no order ambiguity here as to when to execute the MetaAllocatorPtr operator+.  So, parens are not needed.

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

The return type of sizeInBytes() is size_t.  So, I think we should be operating on size_t, not uintptr_t or intptr_t.

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

Will add a FIXME and reference to https://bugs.webkit.org/show_bug.cgi?id=185145.

[Mark Lam]

Thanks for the review.  Landed in r231175: <http://trac.webkit.org/r231175>.