[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

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

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

Almost there.

> Source/WTF/wtf/Packed.h:147
> +#if CPU(X86_64)

Please do one of the following:
1. In PlatformOS.h, add a #define for WTF_OS_SOLARIS based on what gcc would define for Solaris.
    Change this line to `#if CPU(X86_64) && OS(SOLARIS)`

Or ...

2. Change this line to `#if CPU(X86_64) && !(OS(DARWIN) || OS(LINUX) || OS(WINDOWS))`

I prefer option (1) but if needed, I can accept option (2).

> Source/WTF/wtf/Packed.h:154
> +        // The AMD specification requires that the most significant 16
> +        // bits of any virtual address, bits 48 through 63, must be
> +        // copies of bit 47 (in a manner akin to sign extension).
> +        //
> +        // Reference: https://en.wikipedia.org/wiki/X86-64#Virtual_address_space_details
> +        if (value & 0x1ULL << 47)
> +            value |= 0xffffULL << 48;

After thinking about this some more, I realized that this quirk only applies to X86_64 implementations that have 48 bit addresses.  The spec actually allows for implementations with > 48 bit addresses.  However, I'm not aware that those implementations actually exist today.  Also, lots of WebKit code relies on addresses not exceeding 48 bits.  

Please add the following just above this if statement:
        static_assert(OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) == 48);

This will prevent this quirk from silently causing bad behavior on x86_64 implementations that have > 48 bits addressing (not that a fix will be easy).  It also documents our expectation.

> Source/WTF/wtf/Packed.h:166
> +#if CPU(X86_64)
> +        // Ensure bits 48-63 track the value of bit 47, per the AMD spec.
> +        ASSERT(bitwise_cast<intptr_t>(value) == (bitwise_cast<intptr_t>(value) << 16) >> 16);
> +#endif

The reason I asked for this before is because it will catch the issue if someone ends up building for another OS with an unusual virtual memory map like Solaris.  Having this assertion here allows us to use Option 1 above so that all ports will get the optimized behavior by default.  And if it doesn't work for any specific port, it won't fail silently anymore.

> Tools/TestWebKitAPI/Tests/WTF/Packed.cpp:67
> +        uint8_t* candidates[] =
> +            {
> +                0,

WebKit style requires that this be written as:
        uint8_t* candidates[] = {
            0,
            ...
        };

> Tools/TestWebKitAPI/Tests/WTF/Packed.cpp:83
> +#if CPU(ADDRESS64)
> +                bitwise_cast<uint8_t*>(0x123456789ULL),
> +#else
> +                bitwise_cast<uint8_t*>(0x12345678UL),
> +#endif
> +#if CPU(X86_64)
> +                bitwise_cast<uint8_t*>((1ULL << 47) - 1), // max lower half
> +                bitwise_cast<uint8_t*>(~((1ULL << 47) - 1)), // min higher half
> +                bitwise_cast<uint8_t*>(static_cast<uintptr_t>(-1)), // max higher half
> +#else
> +                // 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.
> +                bitwise_cast<uint8_t*>(static_cast<uintptr_t>((1ULL << OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH)) - 1)),
> +#endif

I would write these as:

        bitwise_cast<uint8_t*>(static_cast<uintptr_t>((1ULL << (OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) / 2)) - 1)),
        bitwise_cast<uint8_t*>(static_cast<uintptr_t>((1ULL << (OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) - 1)) - 1)),
#if CPU(X86_64) && OS(SOLARIS)
        bitwise_cast<uint8_t*>(~((1ULL << 47) - 1)), // min higher half
        bitwise_cast<uint8_t*>(std::numeric_limits<uintptr_t>::max()),
#else
        bitwise_cast<uint8_t*>(static_cast<uintptr_t>((1ULL << OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH)) - 1)),
#endif

-- 
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/20200710/0b6caac7/attachment.htm>