88692 – DFG should be able to set watchpoints on global variables

Bug 88692 - DFG should be able to set watchpoints on global variables

Summary: DFG should be able to set watchpoints on global variables

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	528+ (Nightly build)
Hardware:	All All

Importance:	P2 Normal
Assignee:	Filip Pizlo

URL:
Keywords:

Depends on:	88976
Blocks:
	Show dependency tree / graph

Reported:	2012-06-08 16:42 PDT by Filip Pizlo
Modified:	2012-06-13 13:55 PDT (History)
CC List:	7 users (show)

See Also:

Attachments
work in progress (32.33 KB, patch) 2012-06-08 16:45 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
more things (47.77 KB, patch) 2012-06-08 22:15 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
did some more stuff (52.76 KB, patch) 2012-06-08 23:33 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
even more (70.98 KB, patch) 2012-06-09 12:00 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
more code (80.26 KB, patch) 2012-06-09 17:32 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
it might be done (84.46 KB, patch) 2012-06-10 13:28 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
omg it compiles! (105.87 KB, patch) 2012-06-10 14:11 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
it does things (112.37 KB, patch) 2012-06-10 17:11 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
the patch (122.31 KB, patch) 2012-06-11 20:07 PDT, Filip Pizlo	ggaren: review+	Details \| Formatted Diff \| Diff
rebased patch (125.68 KB, patch) 2012-06-12 14:00 PDT, Filip Pizlo	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
the patch (129.86 KB, patch) 2012-06-12 16:52 PDT, Filip Pizlo	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
backup (134.19 KB, patch) 2012-06-12 18:27 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
real patch (145.99 KB, patch) 2012-06-12 22:41 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
ummmm please apply? (153.35 KB, patch) 2012-06-12 22:49 PDT, Filip Pizlo	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
attempting to fix Win (153.48 KB, patch) 2012-06-13 00:04 PDT, Filip Pizlo	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
attempt to fix the compiler's failed attempts to be smart (153.49 KB, patch) 2012-06-13 00:34 PDT, Filip Pizlo	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
fix link errors on Windows (154.17 KB, patch) 2012-06-13 01:03 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
more platform spoonfeeding (163.25 KB, patch) 2012-06-13 12:41 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (17) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Filip Pizlo 2012-06-08 16:42:55 PDT

Work in progress patch on the way.

Comment 1 Filip Pizlo 2012-06-08 16:45:50 PDT

Created attachment 146663 [details]
work in progress

Comment 2 Filip Pizlo 2012-06-08 22:15:21 PDT

Created attachment 146684 [details]
more things

Comment 3 Filip Pizlo 2012-06-08 23:33:03 PDT

Created attachment 146687 [details]
did some more stuff

Messed around with the interface between global variable creation, global variable access, and watch pointing.  It's starting to make sense.

Still more code to write, though.

Comment 4 Filip Pizlo 2012-06-09 12:00:30 PDT

Created attachment 146716 [details]
even more

I just have to link together a few more things and it should start working.

Comment 5 Filip Pizlo 2012-06-09 17:32:16 PDT

Created attachment 146721 [details]
more code

Still more stuff to do but it's getting there.

Comment 6 Filip Pizlo 2012-06-10 13:28:50 PDT

Created attachment 146747 [details]
it might be done

Now I need to compile it.  And test it.

Comment 7 Filip Pizlo 2012-06-10 14:11:50 PDT

Created attachment 146749 [details]
omg it compiles!

Comment 8 Filip Pizlo 2012-06-10 17:11:12 PDT

Created attachment 146758 [details]
it does things

It looks like I can set a watchpoint on a global variable.  Note I said "set", not "fire".  I'm guessing it'll crash in hilarious ways if I actually try to fire a watchpoint.

Comment 9 Filip Pizlo 2012-06-10 17:12:52 PDT

(In reply to comment #8)
> Created an attachment (id=146758) [details]
> it does things
> 
> It looks like I can set a watchpoint on a global variable.  Note I said "set", not "fire".  I'm guessing it'll crash in hilarious ways if I actually try to fire a watchpoint.

Actually, it can fire the watchpoints as well.  Fantastic!

Still have to do way more tests though.

Comment 10 Filip Pizlo 2012-06-11 20:07:06 PDT

Created attachment 146996 [details]
the patch

Comment 11 Geoffrey Garen 2012-06-12 13:31:01 PDT

Comment on attachment 146996 [details]
the patch

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

r=me, please remember to ARM before landing.

> Source/JavaScriptCore/assembler/RepatchBuffer.h:125
> +    

Boo.

> Source/JavaScriptCore/assembler/X86Assembler.h:1737
> +        // FIXME: need to know the size of the last watchpoint as well.

No longer true.

> Source/JavaScriptCore/assembler/X86Assembler.h:1739
> +        while (UNLIKELY(static_cast<int>(result.m_offset) < m_indexOfTailOfLastWatchpoint)) {
> +            nop();

I think the ARMv7 code will also need to nop() pad in front of jumps, because of this case:

before jump relaxation:
watch_point
big jump
label:

after jump relaxation:
watch_point
small jump
label: [now inside the watchpoint]

> Source/JavaScriptCore/assembler/X86Assembler.h:1823
> +        int8_t *ptr = reinterpret_cast<int8_t*>(instructionStart);
> +        int8_t *dstPtr = reinterpret_cast<int8_t*>(to);
> +        intptr_t distance = (intptr_t)(dstPtr - (ptr + 5));

Move the "*", please.

Comment 12 Filip Pizlo 2012-06-12 14:00:41 PDT

Created attachment 147154 [details]
rebased patch

Putting up for EWS.

Comment 13 WebKit Review Bot 2012-06-12 14:02:44 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:103:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:120:  The parameter name "watchpointSet" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/SymbolTable.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/SymbolTable.h:69:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WTF/wtf/SegmentedVector.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:805:  cmpb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:962:  testb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:583:  The parameter name "constantMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:583:  The parameter name "functionMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/Watchpoint.h:52:  The parameter name "linkBuffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 62 files


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

Comment 14 Build Bot 2012-06-12 14:18:33 PDT

Comment on attachment 147154 [details]
rebased patch

Attachment 147154 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12945666

Comment 15 Build Bot 2012-06-12 14:40:02 PDT

Comment on attachment 147154 [details]
rebased patch

Attachment 147154 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12939819

Comment 16 Filip Pizlo 2012-06-12 16:52:36 PDT

Created attachment 147193 [details]
the patch

Fixed Mac build issues.  I think.

Comment 17 WebKit Review Bot 2012-06-12 16:54:46 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:103:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:120:  The parameter name "watchpointSet" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/SymbolTable.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/SymbolTable.h:69:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WTF/wtf/SegmentedVector.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:805:  cmpb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:962:  testb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:583:  The parameter name "constantMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:583:  The parameter name "functionMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 9 in 60 files


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

Comment 18 Build Bot 2012-06-12 17:15:55 PDT

Comment on attachment 147193 [details]
the patch

Attachment 147193 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12944699

Comment 19 Build Bot 2012-06-12 17:17:36 PDT

Comment on attachment 147193 [details]
the patch

Attachment 147193 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12951098

Comment 20 Early Warning System Bot 2012-06-12 17:31:10 PDT

Comment on attachment 147193 [details]
the patch

Attachment 147193 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12948609

Comment 21 Early Warning System Bot 2012-06-12 17:55:53 PDT

Comment on attachment 147193 [details]
the patch

Attachment 147193 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12941797

Comment 22 Filip Pizlo 2012-06-12 18:27:47 PDT

Created attachment 147207 [details]
backup

Still working on build systems, ports, crashes, etc.

Comment 23 Filip Pizlo 2012-06-12 22:41:14 PDT

Created attachment 147235 [details]
real patch

Still doing more things but it's starting to work.

Unlike the previous patch, this one actually includes all of the files I've added!

Comment 24 Filip Pizlo 2012-06-12 22:49:24 PDT

Created attachment 147238 [details]
ummmm please apply?

Rebased the patch in the hopes that the bots can apply it.

Comment 25 WebKit Review Bot 2012-06-12 22:54:52 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:103:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:120:  The parameter name "watchpointSet" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/SymbolTable.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/SymbolTable.h:69:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WTF/wtf/SegmentedVector.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:805:  cmpb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:962:  testb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:583:  The parameter name "constantMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:583:  The parameter name "functionMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/Watchpoint.h:52:  The parameter name "linkBuffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 68 files


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

Comment 26 Build Bot 2012-06-12 23:28:48 PDT

Comment on attachment 147238 [details]
ummmm please apply?

Attachment 147238 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12941861

Comment 27 Filip Pizlo 2012-06-13 00:04:58 PDT

Created attachment 147245 [details]
attempting to fix Win

Comment 28 WebKit Review Bot 2012-06-13 00:08:54 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:103:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:120:  The parameter name "watchpointSet" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/SymbolTable.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/SymbolTable.h:69:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WTF/wtf/SegmentedVector.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:805:  cmpb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:962:  testb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 7 in 68 files


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

Comment 29 Build Bot 2012-06-13 00:28:26 PDT

Comment on attachment 147245 [details]
attempting to fix Win

Attachment 147245 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12939939

Comment 30 Filip Pizlo 2012-06-13 00:34:27 PDT

Created attachment 147252 [details]
attempt to fix the compiler's failed attempts to be smart

Comment 31 WebKit Review Bot 2012-06-13 00:37:38 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:103:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:120:  The parameter name "watchpointSet" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/SymbolTable.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/SymbolTable.h:69:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WTF/wtf/SegmentedVector.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:805:  cmpb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:962:  testb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 7 in 68 files


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

Comment 32 Build Bot 2012-06-13 01:00:53 PDT

Comment on attachment 147252 [details]
attempt to fix the compiler's failed attempts to be smart

Attachment 147252 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12941882

Comment 33 Filip Pizlo 2012-06-13 01:03:00 PDT

Created attachment 147255 [details]
fix link errors on Windows

Comment 34 WebKit Review Bot 2012-06-13 01:06:39 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:103:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:120:  The parameter name "watchpointSet" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/SymbolTable.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/SymbolTable.h:69:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WTF/wtf/SegmentedVector.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:805:  cmpb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:962:  testb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 7 in 69 files


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

Comment 35 Filip Pizlo 2012-06-13 01:21:21 PDT

Landed in http://trac.webkit.org/changeset/120172

Comment 36 Milian Wolff 2012-06-13 03:33:13 PDT

This commit breaks compilation on platforms where the ENALBE_JIT and hence ENABLE_ASSEMBLER is false (e.g. QNX).

The reason is that MacroAssembler::Label is used in Watchpoint.h, but the former is not defined on such platforms (see dummy definition at the end of MacroAssembler in MacroAssembler.h).

Should I create a new bug report for this?

Comment 37 Filip Pizlo 2012-06-13 12:41:43 PDT

Created attachment 147393 [details]
more platform spoonfeeding

Comment 38 WebKit Review Bot 2012-06-13 12:46:43 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:103:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:120:  The parameter name "watchpointSet" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/SymbolTable.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/SymbolTable.h:69:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WTF/wtf/SegmentedVector.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WTF/wtf/Assertions.h:386:  UNREACHABLE_FOR_PLATFORM is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:805:  cmpb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:962:  testb_im is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 8 in 72 files


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

Comment 39 Filip Pizlo 2012-06-13 13:55:14 PDT

Relanded in http://trac.webkit.org/changeset/120244