[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 #12 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 403897
  --> https://bugs.webkit.org/attachment.cgi?id=403897
Patch

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

> Source/WTF/wtf/Packed.h:166
> +        // Shift bit 47 to high order bit, then arithmetic shift back.
> +        // If value is different, address is not canonical.

Just remove these 2 lines.  It's frowned upon in WebKit code to have comments that say exactly the operations the code is doing as opposed to why it's doing it.

> Source/WTF/wtf/Packed.h:167
> +        ASSERT(bitwise_cast<intptr_t>(value) == bitwise_cast<intptr_t>(value) << 16 >> 16);

Please put parens around bitwise_cast<intptr_t>(value) << 16:
    ASSERT(bitwise_cast<intptr_t>(value) == (bitwise_cast<intptr_t>(value) << 16) >> 16);

> Tools/TestWebKitAPI/Tests/WTF/Packed.cpp:72
> +#if CPU(X86_64)
> +        // validate X86_64 "higher half" address
> +        key = max = bitwise_cast<uint8_t*>(static_cast<uintptr_t>(-1));
> +        EXPECT_EQ(key.get(), max);
> +#endif

This isn't want I was getting at.  Your earlier patch was failing the API tests (see https://ews-build.webkit.org/#/builders/3/builds/28085) with the following details:

Failed
    TestWTF.WTF_Packed.AssignAndGet

        /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WTF/Packed.cpp:67
        Expected equality of these values:
          key.get()
            Which is: 0xffffffffffffffff
          max
            Which is: 0xffffffffffff

The reason it failed is because the test fabricated an address in "max" of 0xffffffffffff, which is an invalid address according to the spec.  Naturally, the test would fail.

You should fix the test to handle the following cases:
    0xffff ffff ffff ffff
    0xffff ffff ffff if not X86_64
    0x7fff ffff ffff
    0x0001 2345 6789
    0x0000 0000 0000

Does that make sense?

Also, since the only difference here between each test case is the input pointer value, I would put these values in an array, and just run the test in a loop.

-- 
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/20200709/25aa208a/attachment.htm>