[webkit-reviews] review granted: [Bug 181542] Replace all use of ConstExprPoisoned with Poisoned. : [Attachment 331258] proposed patch.

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 181542: Replace all use of ConstExprPoisoned with Poisoned.
https://bugs.webkit.org/show_bug.cgi?id=181542

Attachment 331258: proposed patch.

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




--- Comment #12 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 331258
  --> https://bugs.webkit.org/attachment.cgi?id=331258
proposed patch.

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

Some suggestions, none major, r=me

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:486
> +    unpoison(_g_CodeBlockPoison, scratch, scratch2)

You should paranoid-zero-out scratch2 here. There's a bunch more below. Maybe
file a follow-up to do so?

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:758
> +	   xorp scratch, field

Or maybe this should just zero-out the poison? If we ever re-use it we can do
so in a different macro.

> Source/JavaScriptCore/runtime/JSCPoison.cpp:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

-2018

> Source/JavaScriptCore/runtime/JSCPoison.cpp:35
> +uintptr_t g_nativeCodePoison;

I don't understand these. They're not used with this lowercase spelling in the
code you're changing? Are they just stale?

> Source/JavaScriptCore/runtime/JSCPoison.cpp:2
> + * Copyright (C) 2017-2018 Apple Inc. All rights reserved.

Actually I don't get it, you're adding this file above *and* modifying it here?
I think this patch has some borky-bork going on!

> Source/JavaScriptCore/runtime/JSCPoison.cpp:34
> +#define INSTANTIATE_POISON(poisonID) \

C++ pedant: this is "define" (to complement header file "declare").

> Source/JavaScriptCore/runtime/JSCPoison.h:58
> +#define POISON(_poisonID_) g_##_poisonID_##Poison

I like the style, but I'd name the macro POISON_FOR so it reads better when
used.

> Source/JavaScriptCore/runtime/WriteBarrier.h:198
> +    static auto poison() { return Traits::poison(); }

Wooh! That was easy :D

> Source/WTF/wtf/Poisoned.h:69
> +enum PrePoisonedTag { PrePoisoned };

I'm not sure "pre" is super obvious. How about "AlreadyPoisoned"? Or
"DontPoison"? No strong feeling on this, feel free to ignore.

> Source/WTF/wtf/Poisoned.h:91
> +    Poisoned(Poisoned&& other)

Can this be =default?

> Source/WTF/wtf/Poisoned.h:132
> +    bool operator>=(const Poisoned& b) const { return m_poisonedBits >=
b.m_poisonedBits; }

Do we use the relational operators? I'd only add them if we do, it seems weird
to order poisoned values (as opposed to just comparing them) but sometimes you
gotta map stuff I guess. Seems like if you have them you'd want hash too (but
I'm not sure you want that either!).

> Source/WTF/wtf/Poisoned.h:209
> +    template<uintptr_t&, typename, typename> friend class Poisoned;

Not sure I get why this is friending itself.

> Source/WTF/wtf/Poisoned.h:220
>  template<uintptr_t& key, typename T>

Shouldn't this be called poison instead of key? Since we don't do keys anymore.
Here and a bunch of other places.

> Tools/TestWebKitAPI/Tests/WTF/PoisonedRef.cpp:48
> +	   // Make sure we get 2 different poison values.

?

> Tools/TestWebKitAPI/Tests/WTF/PoisonedRefPtr.cpp:50
> +	   // Make sure we get 2 different poison values.

?

> Tools/TestWebKitAPI/Tests/WTF/PoisonedUniquePtr.cpp:42
> +	   // Make sure we get 2 different poison values.

This one does what it says :)