[webkit-reviews] review denied: [Bug 214142] [WTF] Fix PackedAlignedPtr for X86_64 canonical addresses : [Attachment 404143] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Jim Mason <jmason at ibinx.com>'s request
for review:
Bug 214142: [WTF] Fix PackedAlignedPtr for X86_64 canonical addresses
https://bugs.webkit.org/show_bug.cgi?id=214142

Attachment 404143: Patch

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




--- Comment #29 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 404143
  --> https://bugs.webkit.org/attachment.cgi?id=404143
Patch

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

> Source/WTF/wtf/Packed.h:147
> +#if HAVE(CANONICAL_ADDRESSING)

This is wrong because the name does not match what the code is trying to do. 
x86_64 on OS(DARWIN), OS(LINUX), and OS(WINDOWS) all have canonical addresses,
but none of them want this.  Really, this is truly an OS(SOLARIS) thing. 
There's currently no evidence that any other OS would lay out memory this way. 
Declaring this HAVE(CANONICAL_ADDRESSING) condition is just opening a door for
confusion later because it doesn't really guard what it says.

Please just use #if OS(SOLARIS) here.

> Source/WTF/wtf/Packed.h:151
> +	   //

You can add a comment here saying: "No other OS needs to do this because they
do not map user space data into any address range with bit 47 set."

> Source/WTF/wtf/Packed.h:172
> +#if HAVE(CANONICAL_ADDRESSING)
> +	   // Ensure the address is in canonical form (see note above).
> +	   ASSERT(bitwise_cast<intptr_t>(value) ==
(bitwise_cast<intptr_t>(value)
> +	       << (64 - OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH)))
> +	       >> (64 - OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH)));
> +#elif CPU(X86_64)
> +	   // Ensure the address is in the lower half.
> +	   // If this assert fails, you may need to enable
CANONICAL_ADDRESSING.
> +	   ASSERT(value < 1ULL << (OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) - 1));
> +#endif

Let's remove this.  There's a much easier way to do this assert.

After the memcpy below, add:
    ASSERT(bitwise_cast<uintptr_t>(get()) == value);

This is also an improvement because it tests how we will actually encode the
address bits in the packed pointer, not how we "expect" we will encode the
address bits.  And there's no need for any conditionals.  The assertion should
always be true.

> Source/WTF/wtf/PlatformHave.h:678
> +#if CPU(X86_64) && !(OS(DARWIN) || OS(LINUX) || OS(WINDOWS))
> +#define HAVE_CANONICAL_ADDRESSING 1
> +#endif

Please remove this.  Instead, please add the following to PlatformOS.h:

#if defined(__sun)
#define WTF_OS_SOLARIS 1
#endif

> Tools/TestWebKitAPI/Tests/WTF/Packed.cpp:69
> +#if HAVE(CANONICAL_ADDRESSING)

Let's use `#if OS(SOLARIS)` instead.

> Tools/TestWebKitAPI/Tests/WTF/Packed.cpp:72
> +#elif !CPU(X86_64)

Use `#else`

> Tools/TestWebKitAPI/Tests/WTF/Packed.cpp:76
> +	       // Caution: This test should be processor-specific.
> +	       //
> +	       // In absence of a specific test, assume that all
> +	       // bits of the EFFECTIVE_ADDRESS_WIDTH can be used.

Just say "Other OSes will never allocate user space addresses with bit 47 (i.e.
OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) - 1) set."