Bug 69018 - add more stack dumping methods
Summary: add more stack dumping methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Peters
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-28 12:39 PDT by Gavin Peters
Modified: 2011-10-06 06:42 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.21 KB, patch)
2011-09-28 12:43 PDT, Gavin Peters
gavinp: review-
gavinp: commit-queue-
Details | Formatted Diff | Diff
Patch (5.49 KB, patch)
2011-09-29 08:03 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.63 KB, patch)
2011-09-29 11:45 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.66 KB, patch)
2011-09-29 12:23 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.63 KB, patch)
2011-09-29 12:31 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.65 KB, patch)
2011-09-29 13:05 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.65 KB, patch)
2011-09-29 13:10 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.76 KB, patch)
2011-09-30 07:05 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2011-09-30 08:13 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2011-09-30 08:29 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2011-09-30 10:14 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.23 KB, patch)
2011-09-30 11:16 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.81 KB, patch)
2011-10-03 10:49 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.54 KB, patch)
2011-10-03 17:32 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (2.07 KB, patch)
2011-10-06 06:35 PDT, Gavin Peters
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2011-09-28 12:39:20 PDT
add more stack dumping methods
Comment 1 Gavin Peters 2011-09-28 12:43:44 PDT
Created attachment 109057 [details]
Patch
Comment 2 Early Warning System Bot 2011-09-28 13:31:31 PDT
Comment on attachment 109057 [details]
Patch

Attachment 109057 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9884595
Comment 3 Geoffrey Garen 2011-09-28 14:27:14 PDT
Comment on attachment 109057 [details]
Patch

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

> Source/JavaScriptCore/wtf/Assertions.cpp:169
> +void WTFSaveBacktrace(void** stack, int* size)

"WTFGetBacktrace" is a better name to indicate and out parameter.

> Source/JavaScriptCore/wtf/Assertions.cpp:181
> +// ugly: returns string allocated by malloc, which must be freed.
> +char* WTFPrettifyStackframe(void* addr)

Please return a PassOwnPtr instead.

> Source/JavaScriptCore/wtf/Assertions.cpp:193
> +        char* mallocedName = (char*) malloc(strlen(mangledName)+1);

Please use fastMalloc instead of malloc. Please use C++ cast instead of C cast.

> Source/JavaScriptCore/wtf/Assertions.cpp:194
> +        strcpy(mallocedName, mangledName);

Please use strncpy.
Comment 4 Gavin Peters 2011-09-29 08:03:53 PDT
Created attachment 109164 [details]
Patch
Comment 5 Gavin Peters 2011-09-29 08:07:06 PDT
Comment on attachment 109057 [details]
Patch

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

I've redone it to make the memory allocation much more clear.

>> Source/JavaScriptCore/wtf/Assertions.cpp:169
>> +void WTFSaveBacktrace(void** stack, int* size)
> 
> "WTFGetBacktrace" is a better name to indicate and out parameter.

Yes, it is.  Done.

>> Source/JavaScriptCore/wtf/Assertions.cpp:193
>> +        char* mallocedName = (char*) malloc(strlen(mangledName)+1);
> 
> Please use fastMalloc instead of malloc. Please use C++ cast instead of C cast.

Hrm.  The only reason to use malloc() here is for paralellism with abi::__cxa_demangle() which is going to use malloc no matter what.  So now I'd have to make a copy, and if i'm not mistaken, OwnPtr/PassOwnPtr use delete and not fastFree or delete[].  So I'm going to a WTFString instead.  Let me know what you think.
Comment 6 Gavin Peters 2011-09-29 11:45:28 PDT
Created attachment 109181 [details]
Patch
Comment 7 Gavin Peters 2011-09-29 11:46:53 PDT
This most recent patch should fix the windows build, but I wonder what to do about QT.
Comment 8 Geoffrey Garen 2011-09-29 12:19:38 PDT
Comment on attachment 109181 [details]
Patch

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

> Source/JavaScriptCore/wtf/Assertions.cpp:77
> +    if (mangledName) {
> +        char* cxaDemangled = abi::__cxa_demangle(mangledName, 0, 0, 0);
> +        if (cxaDemangled) {
> +            CString demangledName = cxaDemangled;
> +            cxaDemangled = 0;
> +            return demangledName;
> +        }
> +        return CString(mangledName);

CString is a good choice, since want to pass the string to printf.

However, you have a memory leak here. You need to free the buffer returned by abi::__cxa_demangle. Assigning 0 to it is meaningless.
Comment 9 Gavin Peters 2011-09-29 12:23:09 PDT
Created attachment 109186 [details]
Patch
Comment 10 Gavin Peters 2011-09-29 12:24:00 PDT
Comment on attachment 109181 [details]
Patch

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

>> Source/JavaScriptCore/wtf/Assertions.cpp:77
>> +        return CString(mangledName);
> 
> CString is a good choice, since want to pass the string to printf.
> 
> However, you have a memory leak here. You need to free the buffer returned by abi::__cxa_demangle. Assigning 0 to it is meaningless.

Ugh.  I don't know how that got there.
Done.
Comment 11 Gavin Peters 2011-09-29 12:31:33 PDT
Created attachment 109187 [details]
Patch
Comment 12 Gavin Peters 2011-09-29 12:32:00 PDT
new patch tosses the zeroing of the automatic, since it's not clarifying or helpful.
Comment 13 Geoffrey Garen 2011-09-29 12:33:50 PDT
Comment on attachment 109187 [details]
Patch

Ready to go, but needs a solution to keep Qt building.
Comment 14 Gavin Peters 2011-09-29 13:05:34 PDT
Created attachment 109193 [details]
Patch
Comment 15 Gavin Peters 2011-09-29 13:06:00 PDT
Comment on attachment 109193 [details]
Patch

This patch should fix the QT.
Comment 16 Gavin Peters 2011-09-29 13:10:25 PDT
Created attachment 109194 [details]
Patch
Comment 17 Gavin Peters 2011-09-29 13:12:51 PDT
Comment on attachment 109194 [details]
Patch

Now using && and ||.
Comment 18 Geoffrey Garen 2011-09-29 13:32:34 PDT
Comment on attachment 109194 [details]
Patch

r=me
Comment 19 Early Warning System Bot 2011-09-29 14:27:14 PDT
Comment on attachment 109194 [details]
Patch

Attachment 109194 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9889493
Comment 20 Gavin Peters 2011-09-30 07:05:42 PDT
Created attachment 109295 [details]
Patch
Comment 21 Gavin Peters 2011-09-30 07:07:10 PDT
Comment on attachment 109295 [details]
Patch

This patch much more aggressively excludes itself from QT.
Comment 22 Gavin Peters 2011-09-30 08:13:56 PDT
Created attachment 109299 [details]
Patch
Comment 23 Gavin Peters 2011-09-30 08:15:04 PDT
Comment on attachment 109299 [details]
Patch

Windows failed, again, so I tightened up the restrictions there.  Let's wait on bots before landing this.

Plenty of code doesn't like seeing <CString.h> included in Assertions.h.
Comment 24 Gavin Peters 2011-09-30 08:29:02 PDT
Created attachment 109300 [details]
Patch
Comment 25 Gavin Peters 2011-09-30 08:29:47 PDT
Comment on attachment 109300 [details]
Patch

This (hopefully final) revision of the bug removes the CString, since it was a big part of our problem on both QT and Windows platforms.

Let's wait for the builders...
Comment 26 Gavin Peters 2011-09-30 10:14:12 PDT
Created attachment 109304 [details]
Patch
Comment 27 Gavin Peters 2011-09-30 10:14:55 PDT
Comment on attachment 109304 [details]
Patch

Apparent problem in winbase.h, trying to change the include ordering.  Let's wait for the windows builder...
Comment 28 WebKit Review Bot 2011-09-30 10:16:06 PDT
Attachment 109304 [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/wtf/Assertions.cpp:50:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Gavin Peters 2011-09-30 11:16:53 PDT
Created attachment 109316 [details]
Patch
Comment 30 Gavin Peters 2011-09-30 11:19:52 PDT
Comment on attachment 109316 [details]
Patch

it seems capturestackbacktrace is in dbghelp.h on some machines.  let's see if the builders get happy.
Comment 31 WebKit Review Bot 2011-09-30 11:20:04 PDT
Attachment 109316 [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/wtf/Assertions.cpp:50:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/wtf/Assertions.cpp:51:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 Gavin Peters 2011-10-03 10:49:10 PDT
Created attachment 109494 [details]
Patch
Comment 33 Gavin Peters 2011-10-03 10:52:17 PDT
Comment on attachment 109494 [details]
Patch

Thanks aroben for helping me sort out trouble with Windows builders.  This upload should (let's wait and see...) fix the issues there.

RtlCaptureStackBackTrace was in kernel32.dll back to XP, but isn't in the SDK used for the WebKit windows port; the new approach of finding the symbol in the DLL should cope with that.
Comment 34 Darin Adler 2011-10-03 14:02:28 PDT
Comment on attachment 109494 [details]
Patch

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

> Source/JavaScriptCore/wtf/Assertions.cpp:145
>  }
>  
> +
> +
>  void WTFReportAssertionFailure(const char* file, int line, const char* function, const char* assertion)

Please don’t add these extra blank lines.

> Source/JavaScriptCore/wtf/Assertions.cpp:198
> -    static const int maxFrames = 32;
> +    enum { maxFrames = 32 };

Why did you change this to an enum?

> Source/JavaScriptCore/wtf/Assertions.cpp:221
> +        if (cxaDemangled)
> +            free(cxaDemangled);

Since you can call free on a null pointer and it is a no-op, I suggest removing the if statement.

> Source/JavaScriptCore/wtf/Assertions.h:156
> +WTF_EXPORT_PRIVATE void WTFReportBacktrace();
> +
> +
>  #ifdef __cplusplus

We normally use just one blank line, not two.
Comment 35 Gavin Peters 2011-10-03 17:30:11 PDT
Comment on attachment 109494 [details]
Patch

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

>> Source/JavaScriptCore/wtf/Assertions.cpp:145
>>  void WTFReportAssertionFailure(const char* file, int line, const char* function, const char* assertion)
> 
> Please don’t add these extra blank lines.

Fixed.

>> Source/JavaScriptCore/wtf/Assertions.cpp:198
>> +    enum { maxFrames = 32 };
> 
> Why did you change this to an enum?

Two reasons:

1. I'm more confident that an enum value is a constant expression in all applications, which is good, since array bounds require a constant expression.  On the other hand, it was working as array bounds before.

2. I know that enums will never be in the data segment, or generate code in the text segment.  That's not always true of const values, particularly static const values.  It's probably usually true of them, though.

Both of those reasons combine to suggest enum values are slightly favoured for this type of integer use.

>> Source/JavaScriptCore/wtf/Assertions.cpp:221
>> +            free(cxaDemangled);
> 
> Since you can call free on a null pointer and it is a no-op, I suggest removing the if statement.

Done.

>> Source/JavaScriptCore/wtf/Assertions.h:156
>>  #ifdef __cplusplus
> 
> We normally use just one blank line, not two.

Fixed.
Comment 36 Gavin Peters 2011-10-03 17:32:04 PDT
Created attachment 109560 [details]
Patch
Comment 37 Darin Adler 2011-10-04 07:45:18 PDT
(In reply to comment #35)
> >> Source/JavaScriptCore/wtf/Assertions.cpp:198
> >> +    enum { maxFrames = 32 };
> > 
> > Why did you change this to an enum?
> 
> Two reasons:
> 
> 1. I'm more confident that an enum value is a constant expression in all applications, which is good, since array bounds require a constant expression.  On the other hand, it was working as array bounds before.
> 
> 2. I know that enums will never be in the data segment, or generate code in the text segment.  That's not always true of const values, particularly static const values.  It's probably usually true of them, though.
> 
> Both of those reasons combine to suggest enum values are slightly favoured for this type of integer use.

Those reasons are both wrong. Integer constants work fine in C++. Switching to enum for an integer constant reflects a habit that dates back to the properties of C, does not apply in C++, and is not appropriate for WebKit code.

A minore pont, but a gratuitous change.
Comment 38 Darin Adler 2011-10-04 07:47:01 PDT
Comment on attachment 109560 [details]
Patch

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

> Source/JavaScriptCore/wtf/Assertions.cpp:196
> -    static const int maxFrames = 32;
> +    enum { maxFrames = 32 };

As mentioned in my comment in this bug, I strongly discourage the C-style use of enum here. Reflects FUD about C++ integer constants that is not founded.
Comment 39 Gavin Peters 2011-10-04 08:06:18 PDT
Comment on attachment 109560 [details]
Patch

I'll land it myself after removing the enum.
Comment 40 Gavin Peters 2011-10-04 08:39:00 PDT
Committed r96595: <http://trac.webkit.org/changeset/96595>
Comment 41 Ryosuke Niwa 2011-10-04 12:19:31 PDT
This patch appears to have broken GTK builds:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/26610/steps/compile-webkit/logs/stdio
Comment 42 Gavin Peters 2011-10-04 13:41:11 PDT
Fix coming shortly.
Comment 43 Adam Roben (:aroben) 2011-10-05 07:40:37 PDT
This caused bug 69424.
Comment 44 Gavin Peters 2011-10-06 06:35:15 PDT
Created attachment 109956 [details]
Patch
Comment 45 Adam Roben (:aroben) 2011-10-06 06:38:00 PDT
Comment on attachment 109956 [details]
Patch

I think it would be slightly better to add these exports as part of a patch that makes use of them. But doing it this way is OK too.
Comment 46 Gavin Peters 2011-10-06 06:42:22 PDT
Committed r96809: <http://trac.webkit.org/changeset/96809>