Bug 180514 – Refactoring: Rename ScrambledPtr to Poisoned.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Let's switch our nomenclature to "poisoning" instead of "scrambling" pointers.  This allows us to use shorter names.

[Mark Lam]

Created attachment 328665 [details]
proposed patch.

[EWS Watchlist]

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.

[Mark Lam]

Created attachment 328669 [details]
patch for landing + cmake build fix.

[EWS Watchlist]

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.

[Mark Lam]

Created attachment 328672 [details]
patch for landing + speculative fix for Windows.

[EWS Watchlist]

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.

[Mark Lam]

Created attachment 328677 [details]
patch for landing + 32-bit windows build fix.

[EWS Watchlist]

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.

[Mark Lam]

Thanks for the review.  Landed in r225620: <http://trac.webkit.org/r225620>.

[Radar WebKit Bug Importer]

<rdar://problem/35901694>

[Michael Catanzaro]

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>;
                                                        ^

[Michael Catanzaro]

Committed r225629: <https://trac.webkit.org/changeset/225629>

[Michael Catanzaro]

Reopening

[JF Bastien]

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.

[JF Bastien]

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

[Mark Lam]

Created attachment 328701 [details]
patch for re-landing.

[EWS Watchlist]

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.

[JF Bastien]

Comment on attachment 328701 [details]
patch for re-landing.

r=me

[Mark Lam]

Re-landed in r225632: <http://trac.webkit.org/r225632>.

[Michael Catanzaro]

It works, thanks JF!