Bug 69018 – add more stack dumping methods

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

[Gavin Peters]

add more stack dumping methods

[Gavin Peters]

Created attachment 109057 [details]
Patch

[Early Warning System Bot]

Comment on attachment 109057 [details]
Patch

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

[Geoffrey Garen]

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.

[Gavin Peters]

Created attachment 109164 [details]
Patch

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 109181 [details]
Patch

[Gavin Peters]

This most recent patch should fix the windows build, but I wonder what to do about QT.

[Geoffrey Garen]

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.

[Gavin Peters]

Created attachment 109186 [details]
Patch

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 109187 [details]
Patch

[Gavin Peters]

new patch tosses the zeroing of the automatic, since it's not clarifying or helpful.

[Geoffrey Garen]

Comment on attachment 109187 [details]
Patch

Ready to go, but needs a solution to keep Qt building.

[Gavin Peters]

Created attachment 109193 [details]
Patch

[Gavin Peters]

Comment on attachment 109193 [details]
Patch

This patch should fix the QT.

[Gavin Peters]

Created attachment 109194 [details]
Patch

[Gavin Peters]

Comment on attachment 109194 [details]
Patch

Now using && and ||.

[Geoffrey Garen]

Comment on attachment 109194 [details]
Patch

r=me

[Early Warning System Bot]

Comment on attachment 109194 [details]
Patch

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

[Gavin Peters]

Created attachment 109295 [details]
Patch

[Gavin Peters]

Comment on attachment 109295 [details]
Patch

This patch much more aggressively excludes itself from QT.

[Gavin Peters]

Created attachment 109299 [details]
Patch

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 109300 [details]
Patch

[Gavin Peters]

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...

[Gavin Peters]

Created attachment 109304 [details]
Patch

[Gavin Peters]

Comment on attachment 109304 [details]
Patch

Apparent problem in winbase.h, trying to change the include ordering.  Let's wait for the windows builder...

[WebKit Review Bot]

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.

[Gavin Peters]

Created attachment 109316 [details]
Patch

[Gavin Peters]

Comment on attachment 109316 [details]
Patch

it seems capturestackbacktrace is in dbghelp.h on some machines.  let's see if the builders get happy.

[WebKit Review Bot]

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.

[Gavin Peters]

Created attachment 109494 [details]
Patch

[Gavin Peters]

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.

[Darin Adler]

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.

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 109560 [details]
Patch

[Darin Adler]

(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.

[Darin Adler]

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.

[Gavin Peters]

Comment on attachment 109560 [details]
Patch

I'll land it myself after removing the enum.

[Gavin Peters]

Committed r96595: <http://trac.webkit.org/changeset/96595>

[Ryosuke Niwa]

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

[Gavin Peters]

Fix coming shortly.

[Adam Roben (:aroben)]

This caused bug 69424.

[Gavin Peters]

Created attachment 109956 [details]
Patch

[Adam Roben (:aroben)]

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.

[Gavin Peters]

Committed r96809: <http://trac.webkit.org/changeset/96809>