[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 #24 from Jim Mason <jmason at ibinx.com> ---
You have given this a lot of thought, Mark.  Thank you!  This is becoming quite good.

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

How about a touch of option (1) and option (2)?  Instead of bringing OS-specifics into the test, we could define a new symbol in one of Platform* files, something like USE_CANONICAL_ADDRESSING.  There, it could be setup as an 'opt-in' option, like (1) above:

    #if CPU(X86_64) && (OS(SOLARIS) || ...)
    #define USE_CANONICAL_ADDRESSING
    #endif

or opt-out, like (2) above:

    #if CPU(X86_64) && !(OS(DARWIN) || ...)
    #define USE_CANONICAL_ADDRESSING
    #endif

and the code in Packed.h would read something like:
    #if USE(CANONICAL_ADDRESSING)
        if (value & 0x1ULL << 47)
            value |= 0xffffULL << 48;
    #endif

It avoids having to go touch the source file if we want to change from opt-in to opt-out, or add or remove an OS from the list.

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


What if we use EFFECTIVE_ADDRESS_WIDTH to determine the bit and mask?  e.g,

    #if USE(CANONICAL_ADDRESSING)
        if (value & 0x1ULL << (OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) - 1))
            value |= ~((1ULL << OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH)) - 1);
    #endif

The ASSERT in the setter could be updated similarly.

In this way, if a 56-bit CPU emerges down the road, no adjustment to this code would be needed.  (NaN boxing and other webkit code that rely on 48 bits is another matter... but at least this code would continue to work as expected...)

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