Bug 182017 - Update Poisoned pointers to take a Poison class instead of a uintptr_t&.
Summary: Update Poisoned pointers to take a Poison class instead of a uintptr_t&.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-23 14:55 PST by Mark Lam
Modified: 2018-01-24 09:43 PST (History)
12 users (show)

See Also:


Attachments
proposed patch. (126.52 KB, patch)
2018-01-23 15:50 PST, Mark Lam
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2018-01-23 14:55:59 PST
This paves the way for custom poison values which we'll need for use in TypedArrays later.
Comment 1 Radar WebKit Bug Importer 2018-01-23 14:56:24 PST
<rdar://problem/36795513>
Comment 2 Mark Lam 2018-01-23 15:50:54 PST
Created attachment 332087 [details]
proposed patch.
Comment 3 EWS Watchlist 2018-01-23 15:52:58 PST
Attachment 332087 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:36:  POISON_KEY_NAME is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Filip Pizlo 2018-01-23 15:58:43 PST
Comment on attachment 332087 [details]
proposed patch.

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

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

Note sure if passing Poisoned does what you'll eventually want.

The caller that poisons and unpoisons pointers may have any amount of context information available, and that information may not be in `this`.  But, I'm OK with this change, because I can see how this would work for typed arrays.
Comment 5 JF Bastien 2018-01-23 16:02:03 PST
Comment on attachment 332087 [details]
proposed patch.

r=me assuming breakage is gone.
Comment 6 Mark Lam 2018-01-24 09:42:26 PST
(In reply to Filip Pizlo from comment #4)
> Comment on attachment 332087 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332087&action=review
> 
> > Source/WTF/wtf/Poisoned.h:203
> > +    ALWAYS_INLINE static PoisonedBits poison(const Poisoned* thisPoisoned, U ptr) { return ptr ? bitwise_cast<PoisonedBits>(ptr) ^ Poison::key(thisPoisoned) : 0; }
> 
> Note sure if passing Poisoned does what you'll eventually want.

Yes, it does what I want.  I use the Poisoned this pointer to infer the this pointer of the object embedding it.  The need for this will become clear later in my implementation for TypeArray poisoning.  The poisoning there isn't based on a C++ compile time type, but rather needs to be runtime determined (for reasons I'll elaborate on later in that patch).

> The caller that poisons and unpoisons pointers may have any amount of
> context information available, and that information may not be in `this`. 
> But, I'm OK with this change, because I can see how this would work for
> typed arrays.
Comment 7 Mark Lam 2018-01-24 09:43:49 PST
Thanks for the review.  Landed in r227527: <http://trac.webkit.org/r227527>.