Bug 180514 - Refactoring: Rename ScrambledPtr to Poisoned.
Summary: Refactoring: Rename ScrambledPtr to Poisoned.
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: 2017-12-06 17:29 PST by Mark Lam
Modified: 2017-12-07 11:06 PST (History)
13 users (show)

See Also:


Attachments
proposed patch. (75.21 KB, patch)
2017-12-06 17:36 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
patch for landing + cmake build fix. (76.13 KB, patch)
2017-12-06 17:53 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing + speculative fix for Windows. (76.13 KB, patch)
2017-12-06 19:37 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing + 32-bit windows build fix. (76.25 KB, patch)
2017-12-06 20:34 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch for re-landing. (75.92 KB, patch)
2017-12-07 10:11 PST, Mark Lam
jfbastien: 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 2017-12-06 17:29:48 PST
Let's switch our nomenclature to "poisoning" instead of "scrambling" pointers.  This allows us to use shorter names.
Comment 1 Mark Lam 2017-12-06 17:36:10 PST
Created attachment 328665 [details]
proposed patch.
Comment 2 EWS Watchlist 2017-12-06 17:40:40 PST
Attachment 328665 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:31:  g_classInfoPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:32:  g_masmPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2017-12-06 17:53:57 PST
Created attachment 328669 [details]
patch for landing + cmake build fix.
Comment 4 EWS Watchlist 2017-12-06 17:56:51 PST
Attachment 328669 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:31:  g_classInfoPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:32:  g_masmPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Mark Lam 2017-12-06 19:37:46 PST
Created attachment 328672 [details]
patch for landing + speculative fix for Windows.
Comment 6 EWS Watchlist 2017-12-06 19:39:19 PST
Attachment 328672 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:31:  g_classInfoPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:32:  g_masmPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Mark Lam 2017-12-06 20:34:24 PST
Created attachment 328677 [details]
patch for landing + 32-bit windows build fix.
Comment 8 EWS Watchlist 2017-12-06 20:38:21 PST
Attachment 328677 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:31:  g_classInfoPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:32:  g_masmPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Mark Lam 2017-12-06 22:10:51 PST
Thanks for the review.  Landed in r225620: <http://trac.webkit.org/r225620>.
Comment 10 Radar WebKit Bug Importer 2017-12-06 22:11:23 PST
<rdar://problem/35901694>
Comment 11 Michael Catanzaro 2017-12-07 08:28:41 PST
Comment on attachment 328677 [details]
patch for landing + 32-bit windows build fix.

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

> Source/WTF/wtf/Poisoned.h:44
> -template<typename T, uintptr_t& key, typename = std::enable_if_t<std::is_pointer<T>::value>>
> -class ScrambledPtr {
> +template<typename KeyType, KeyType key, typename T, typename = std::enable_if_t<std::is_pointer<T>::value>>
> +class PoisonedImpl {

Adding KeyType to the template parameters has broken the build with GCC 7. I don't know how to fix it, so I'm going to roll it out... sorry. I guess GCC 6 works, because EWS and all the bots are fine.

In file included from ../../Source/JavaScriptCore/runtime/JSCPoisonedPtr.h:28:0,
                 from ../../Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:29,
                 from ../../Source/JavaScriptCore/interpreter/AbstractPC.h:28,
                 from ../../Source/JavaScriptCore/interpreter/CallFrame.h:25,
                 from ../../Source/JavaScriptCore/runtime/ClassInfo.h:25,
                 from ../../Source/JavaScriptCore/runtime/Structure.h:28,
                 from ../../Source/JavaScriptCore/bytecode/ArrayProfile.h:29,
                 from ../../Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:28:
../../Source/WTF/wtf/Poisoned.h: In substitution of ‘template<uintptr_t& key, class T> using Poisoned = WTF::PoisonedImpl<const long unsigned int&, ((const long unsigned int&)key), T> [with uintptr_t& key = JSC::g_classInfoPoison; T = const JSC::ClassInfo*]’:
../../Source/JavaScriptCore/runtime/JSCPoisonedPtr.h:37:74:   required from here
../../Source/WTF/wtf/Poisoned.h:122:56: error: ‘(const long unsigned int&)JSC::g_classInfoPoison’ is not a valid template argument for type ‘const long unsigned int&’ because it is not an object with linkage
 using Poisoned = PoisonedImpl<const uintptr_t&, key, T>;
                                                        ^
Comment 12 Michael Catanzaro 2017-12-07 08:32:46 PST
Committed r225629: <https://trac.webkit.org/changeset/225629>
Comment 13 Michael Catanzaro 2017-12-07 08:33:00 PST
Reopening
Comment 14 JF Bastien 2017-12-07 09:08:50 PST
Comment on attachment 328677 [details]
patch for landing + 32-bit windows build fix.

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

Here's a GCC 7 version without the bug: https://godbolt.org/g/zVx5fU

extern "C" const unsigned bar;

template<typename KeyType, KeyType key>
struct FooImpl {
    unsigned boo(unsigned v) { return v ^ key; }
};

template<const unsigned& key>
using Foo = FooImpl<const unsigned&, key>;

Foo<bar> foo;

Same thing with the bug: https://godbolt.org/g/a9UYxD

extern "C" unsigned bar;

template<typename KeyType, KeyType key>
struct FooImpl {
    unsigned boo(unsigned v) { return v ^ key; }
};

template<unsigned& key>
using Foo = FooImpl<const unsigned&, key>;

Foo<bar> foo;

> Source/JavaScriptCore/runtime/JSCPoisonedPtr.h:33
> +extern "C" JS_EXPORTDATA uintptr_t g_masmPoison;

And const here.

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

key needs to be const here.
Comment 15 JF Bastien 2017-12-07 09:13:30 PST
Or you can drop the const entirely. It'll generate the same code:

mov    0x0(%rip),%eax        # 0 is a relocation to <bar>
xor    %edi,%eax
Comment 16 Mark Lam 2017-12-07 10:11:20 PST
Created attachment 328701 [details]
patch for re-landing.
Comment 17 EWS Watchlist 2017-12-07 10:13:14 PST
Attachment 328701 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:31:  g_classInfoPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:32:  g_masmPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 JF Bastien 2017-12-07 10:17:36 PST
Comment on attachment 328701 [details]
patch for re-landing.

r=me
Comment 19 Mark Lam 2017-12-07 10:50:38 PST
Re-landed in r225632: <http://trac.webkit.org/r225632>.
Comment 20 Michael Catanzaro 2017-12-07 11:06:42 PST
It works, thanks JF!