[webkit-reviews] review denied: [Bug 183407] [ARM, MIPS] Enable pointer poisoning also for 32-bit architectures : [Attachment 335195] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Dominik Inführ <dinfuehr at igalia.com>'s
request for review:
Bug 183407: [ARM, MIPS] Enable pointer poisoning also for 32-bit architectures
https://bugs.webkit.org/show_bug.cgi?id=183407

Attachment 335195: Patch

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




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

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

r- because this patch hasn't solved where to put the poison marker bit, which
is necessary for correctness.

> Source/WTF/wtf/Poisoned.cpp:45
> +#elif USE(JSVALUE32_64)

Just make this #else.  We only have JSVALUE64 or JSVALUE32_64.	There won't be
a 3rd value representation.

> Source/WTF/wtf/Poisoned.cpp:46
> +    poison = static_cast<uintptr_t>(cryptographicallyRandomNumber() << 3);

This is insufficient.  Look at the code below:

    poison |= (1 << 2);
    RELEASE_ASSERT(!(poison & 0x3)); // Read the comment in the code that
accompanies this assert.

This line sets the 3rd bit from the bottom.  This only makes sense for 64-bit
pointers which are 8 byte aligned.  32-bit pointers are 4 byte aligned.  Hence,
you'll have collision with pointer bits here.  Note: we set the 3rd bit because
we're reserving the bottom 2 bits for clients who may put flag bits down there.
 I forgot the case that motivated this but you should be able to do svn blame
on this line to find the commit that introduced it.  One of the pointers being
poisoned in that patch uses the low bits for flags.

In order to fix this, you need to solve this issue in a different way for
32-bit CPUs.  Is there a bit that is never used for normal valid pointers?  If
so, that's the bit you can use for your poison marker.	Otherwise, you'll have
to invent some scheme to solve this issue.

> Source/WTF/wtf/Poisoned.h:200
> +    ALWAYS_INLINE static U unpoison(const Poisoned* thisPoisoned,
PoisonedBits poisonedBits) { return poisonedBits ? bitwise_cast<U>(poisonedBits
^ Poison::key(thisPoisoned)) : bitwise_cast<U>(intptr_t(0)); }

While I personally like this syntax, I think it is not the WebKit style of
doing cast.  You should change this to "static_cast<intptr_t>(0)".