Get GDB to display a proper backtrace through JITted functions

[Sanjoy Das]

Currently GDB cannot unwind through call frames of JS functions JIT compiled by JSCore.  One new way to fix this is to supply GDB with a plugin that knows how to unwind through such call frames.

The forthcoming patch adds such a plugin (in `Tools/gdb'), JSCPlugin.so and modifies JSCore to work with the plugin.

The patches should work with GDB 7.4 and above.  The plugin can be loaded using the `jit-reader-load', once it is copied to `$(installdir)/lib/gdb'.  ENABLE_GDB_JIT_INTEGRATION in JIT.h needs to be set to 1.

[Sanjoy Das]

Created attachment 120814 [details]
Proposed patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/jit/GDBInterface.cpp:64:  __attribute__ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/GDBInterface.cpp:66:  __jit_debug_descriptor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 11 files


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

[Early Warning System Bot]

Comment on attachment 120814 [details]
Proposed patch

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

[Gyuyoung Kim]

Comment on attachment 120814 [details]
Proposed patch

Attachment 120814 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11166233

[Sanjoy Das]

GDB needs to see both these symbols as they are so they can't be renamed.

Is there some way I can tell check-webkit-style to not check a particular file (or a particular region in a file)?

[Philippe Normand]

(In reply to comment #5)
> GDB needs to see both these symbols as they are so they can't be renamed.
> 
> Is there some way I can tell check-webkit-style to not check a particular file (or a particular region in a file)?

I'd suggest to add a new rule in the _BASE_FILTER_RULES variable of Tools/Scripts/webkitpy/style/checker.py. Can you do that in a separate patch please?

Also don't forget to set the r? and cq? flags on the patches :)

[Sanjoy Das]

Created attachment 122176 [details]
Add a webkit style checker exception for GDBInterface.cpp

I modified _PATH_RULES_SPECIFIER and added an exception for GDBInterface.cpp

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/style/checker.py:232:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 1 in 2 files


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

[Sanjoy Das]

Created attachment 122181 [details]
Add a check-webkit-style exception for GDBInterface.cpp

Fix the pep8/E202 style error in the previous patch.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/style/checker.py:232:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 1 in 2 files


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

[Sanjoy Das]

Created attachment 122185 [details]
Add a check-webkit-style exception for GDBInterface.cpp

[Sanjoy Das]

Created attachment 122261 [details]
Proposed patch

The style errors this patch has are fixed by https://bugs.webkit.org/show_bug.cgi?id=76187.  This version adds the new files to all the build systems, so there should hopefully be no more build errors.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 122261 [details]
Proposed patch

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

> Source/JavaScriptCore/CMakeLists.txt:102
> +    jit/GDBInterface.cpp

Please keep this list alphabetically sorted (that may apply to the other buildsystems touched by this patch as well).

[Sanjoy Das]

Created attachment 122538 [details]
Proposed patch

The new files added to the build scripts are now in sorted order.

[Sanjoy Das]

Created attachment 123762 [details]
Proposed patch

Older patch with edits to make sure it applies cleanly and minor cosmetic changes.

[Early Warning System Bot]

Comment on attachment 123762 [details]
Proposed patch

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

[Filip Pizlo]

Comment on attachment 123762 [details]
Proposed patch

I like this a lot.  Two issues:

1) Fix the build on Qt and Windows.  Seems like Qt doesn't like your use of void*, which should be easy to fix with some static_cast's:


../../../../Source/JavaScriptCore/jit/GDBInterface.cpp:79: error: pointer of type 'void *' used in arithmetic

And Windows is just complaining because the code using gcc/clang specific stuff; probably best to put some #if !OS(WINDOWS) or similar around the relevant code.

2) Would it be possible for you to make the JSCPlugin code use something other than a makefile? For example, that makefile would not work on Mac.  But perhaps autotools would.

Marking r- because of the Qt and Windows build.  Otherwise I think it's probably good to go.

[Sanjoy Das]

Created attachment 124434 [details]
Proposed patch

The void * error has been fixed by adding proper casts.

Changed the __attribute__((noinline)) to NEVER_INLINE, so this should now work on Windows.

jscplugin now uses scons, so it should build successfully on OSX.  I don't have access to an OSX installation so I could not test this completely, but it builds and works as expected on Debian.  I've used LoadableModule instead of SharedLibrary, which I think is the correct method on OSX.

[Sanjoy Das]

Does the current patch look good?

[Filip Pizlo]

(In reply to comment #19)
> Does the current patch look good?

Apologies for not looking at this; I've been out of town and mostly AFK.  I will look.

[Andy Wingo]

The patch does not currently apply.  Sanjoy, would you mind rebasing it?  Thanks!

[Sanjoy Das]

Created attachment 134207 [details]
Proposed patch

Sorry for the delay, have attached a patch that applies correctly.

[WebKit Review Bot]

Attachment 134207 [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/runtime/Executable.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


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

[Build Bot]

Comment on attachment 134207 [details]
Proposed patch

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

[Philippe Normand]

In bug 81659 I'm trying to add gdb support to run-webkit-tests --gtk. It'd be great if at some point I can enable this plugin as well for the crash logs reporting.

[Sanjoy Das]

Created attachment 134742 [details]
Proposed patch

[Build Bot]

Comment on attachment 134742 [details]
Proposed patch

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

[Sanjoy Das]

Created attachment 134931 [details]
Proposed patch

[Filip Pizlo]

Comment on attachment 134931 [details]
Proposed patch

We're moving onto the C stack, so this looks like it's no longer relevant.