Bug 173941 – VMTraps has some races

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

VMTraps has some races

[Keith Miller]

Created attachment 314066 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:311:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 10 files


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

[Keith Miller]

Created attachment 314069 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:310:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 10 files


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

[Keith Miller]

Created attachment 314096 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:310:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 10 files


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

[Michael Saboff]

Comment on attachment 314096 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/VMTraps.cpp:275
> +                    // FIXME: We need to use the machine threads because it is the only non-TLS source
> +                    // for the stack bounds of this thread. We should keep in on the WTF::Thread instead.

Add bug id for this FIXME.

> Source/JavaScriptCore/runtime/VMTraps.cpp:292
> +        

nit: Revert whitespace.

[Keith Miller]

Committed r218936: <http://trac.webkit.org/changeset/218936>

[Saam Barati]

Comment on attachment 314096 [details]
Patch

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

LGTM too. Just a few comments.

> Source/JavaScriptCore/dfg/DFGCommonData.cpp:108
> +        LockHolder locker(pcCodeBlockMapLock);
> +        auto& map = pcCodeBlockMap(locker);
> +        for (auto& jumpReplacement : jumpReplacements)
> +            map.remove(jumpReplacement.dataLocation());

You should make a function for this since it's duplicated below.

> Source/JavaScriptCore/dfg/DFGCommonData.cpp:160
> +    if (result == map.end())
> +        return nullptr;

Seems like this should be a RELEASE_ASSERT(result != map.end()) since we always assume we find it.

> Source/JavaScriptCore/runtime/VMTraps.cpp:113
> +    auto codeBlockSetLocker = isJITPC(trapPC) ? holdLock(lock) : tryHoldLock(lock);

isJITPC should be (isJITPC || isLLIntPC)

Maybe it's worth making a function for this since you use this general concept in another place too.

> Source/JavaScriptCore/runtime/VMTraps.cpp:208
> +                SignalContext context(registers);

It seems sketchy to do the below work on a different thread than the execution thread. To be super duper safe, I'd just return to a thunk that did this for you.

> Source/JavaScriptCore/runtime/VMTraps.cpp:284
> +                            stackBounds = StackBounds(machineThread->stackBase(), machineThread->stackEnd());

why can't you just get this from the VM?