[Webkit-unassigned] [Bug 214142] [WTF] Fix PackedAlignedPtr for X86_64 canonical addresses

[bugzilla-daemon at webkit.org]

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

Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #404143|review?                     |review-
              Flags|                            |

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

-- 
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/20200713/aafd6d9f/attachment-0001.htm>