Bug 180169 – Let's scramble MacroAssemblerCodePtr values.

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

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/35758340>

[Mark Lam]

Created attachment 327915 [details]
proposed patch.

Let's test this on the EWS.

[EWS Watchlist]

Attachment 327915 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 327916 [details]
proposed patch.

[Mark Lam]

Comment on attachment 327916 [details]
proposed patch.

Looks like I need to do more testing.

[Mark Lam]

Created attachment 327939 [details]
proposed patch.

[EWS Watchlist]

Attachment 327939 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 327949 [details]
proposed patch w/ build fixes.

[EWS Watchlist]

Attachment 327949 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 327992 [details]
proposed patch.

[EWS Watchlist]

Attachment 327992 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 327992 [details]
proposed patch.

I think I've resolved all the issues.  Let's get a review.

[Filip Pizlo]

Comment on attachment 327992 [details]
proposed patch.

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

Nice.

> Source/JavaScriptCore/assembler/CodeLocation.h:167
> -    return CodeLocationInstruction(reinterpret_cast<char*>(dataLocation()) + offset);
> +    return CodeLocationInstruction(dataLocation<char*>() + offset);

This is a really great change.

[JF Bastien]

Comment on attachment 327992 [details]
proposed patch.

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

Can you get perf numbers?

> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:322
> +    template<typename T = void*>

Is it useful to have a default here? Adding to void* is fraught with peril, so users of this shouldn't really get a void* no?

> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:486
> +    // (I guess on RVTC function pointers have a different constness to GCC/MSVC?)

There's no C-style cast here?

[Saam Barati]

Comment on attachment 327992 [details]
proposed patch.

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

r=me too with some comments

> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.cpp:32
> +#include <mutex>

Why is this include needed?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:219
> +#if USE(JSVALUE64)
> +    jit.load64(&g_masmScrambledPtrKey, GPRInfo::regT1);
> +    jit.xor64(GPRInfo::regT1, GPRInfo::regT4);
> +#endif

Isn't this value guaranteed to be initialized at this point? Why load instead of just materialize the actual value?
like:
jit.move(TrustedImm64(g_masmScrambledPtrKey), regT1)

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1168
> +#if USE(JSVALUE64)
> +    jit.load64(&g_masmScrambledPtrKey, GPRInfo::regT1);
> +    jit.xor64(GPRInfo::regT1, GPRInfo::regT0);
> +#endif

ditto

> Source/WTF/wtf/ScrambledPtr.cpp:38
> +    key |= 0x7; // Ensure that any valid pointer ^ key will not be 0.

This comment needs some more explanation. Clearly there are valid pointers that could have these bits set, e.g, char* pointers. I think you perhaps mean something more subtle, where you're at least aligned passed on 2 byte boundaries? But do x86 code pointers guarantee that?

> Source/WTF/wtf/ScrambledPtr.h:30
> +#define ENABLE_PARANOID_SCRAMBLED_PTR_ASSERTS 0

Why not define to 1 on debug builds at least?

> Source/WTF/wtf/ScrambledPtr.h:60
> +    static void paranoidAssertIsScrambled(T value) { RELEASE_ASSERT(isScrambled(value)); }

I don't see any reason to prefix this with paranoid. I'd just drop that part of it.

[Mark Lam]

Comment on attachment 327992 [details]
proposed patch.

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

>> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:322
>> +    template<typename T = void*>
> 
> Is it useful to have a default here? Adding to void* is fraught with peril, so users of this shouldn't really get a void* no?

Yes, we normally want void*.  The template argument is to make it so that the cases that want the bits as another type will get an bitcast so that the client doesn't need to do the explicit cast.

>> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:486
>> +    // (I guess on RVTC function pointers have a different constness to GCC/MSVC?)
> 
> There's no C-style cast here?

Oh my ... total cargo cult copying from FunctionPtr(FunctionType* value).  I'll remove the comments.

[Mark Lam]

Comment on attachment 327992 [details]
proposed patch.

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

>> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.cpp:32
>> +#include <mutex>
> 
> Why is this include needed?

For std::call_once().  See http://en.cppreference.com/w/cpp/thread/call_once.

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:219
>> +#endif
> 
> Isn't this value guaranteed to be initialized at this point? Why load instead of just materialize the actual value?
> like:
> jit.move(TrustedImm64(g_masmScrambledPtrKey), regT1)

Thanks for catching this.  Fixed (here and at other places where I do the same thing).

>> Source/WTF/wtf/ScrambledPtr.cpp:38
>> +    key |= 0x7; // Ensure that any valid pointer ^ key will not be 0.
> 
> This comment needs some more explanation. Clearly there are valid pointers that could have these bits set, e.g, char* pointers. I think you perhaps mean something more subtle, where you're at least aligned passed on 2 byte boundaries? But do x86 code pointers guarantee that?

I'll beef up the comment.  You are right that this "|= 0x7" is incorrect for x86, and it turns out to be unnecessary as well.  The "|= 1 << 63" below achieves the same goal, and does so correctly.

>> Source/WTF/wtf/ScrambledPtr.h:30
>> +#define ENABLE_PARANOID_SCRAMBLED_PTR_ASSERTS 0
> 
> Why not define to 1 on debug builds at least?

I think we only need this when debugging the application of ScrambledPtrs to new class of pointers.  Once we're done with debugging, I think it's rare to introduce bugs that would be caught by this.  I prefer not to slow down debug build runs unnecessarily.  We can always change our mind about this later if I'm proven wrong about this class of bugs.

>> Source/WTF/wtf/ScrambledPtr.h:60
>> +    static void paranoidAssertIsScrambled(T value) { RELEASE_ASSERT(isScrambled(value)); }
> 
> I don't see any reason to prefix this with paranoid. I'd just drop that part of it.

OK, will do.

[Mark Lam]

Created attachment 328009 [details]
patch for landing.

[EWS Watchlist]

Attachment 328009 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 328014 [details]
patch for landing.

[EWS Watchlist]

Attachment 328014 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 328024 [details]
x86_64 benchmark results.

Perf appears to be neutral.

[Mark Lam]

Thanks for the reviews.  Landed in r225363: <http://trac.webkit.org/r225363>.