add more stack dumping methods
Created attachment 109057 [details] Patch
Comment on attachment 109057 [details] Patch Attachment 109057 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9884595
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.
Created attachment 109164 [details] Patch
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.
Created attachment 109181 [details] Patch
This most recent patch should fix the windows build, but I wonder what to do about QT.
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.
Created attachment 109186 [details] Patch
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.
Created attachment 109187 [details] Patch
new patch tosses the zeroing of the automatic, since it's not clarifying or helpful.
Comment on attachment 109187 [details] Patch Ready to go, but needs a solution to keep Qt building.
Created attachment 109193 [details] Patch
Comment on attachment 109193 [details] Patch This patch should fix the QT.
Created attachment 109194 [details] Patch
Comment on attachment 109194 [details] Patch Now using && and ||.
Comment on attachment 109194 [details] Patch r=me
Comment on attachment 109194 [details] Patch Attachment 109194 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9889493
Created attachment 109295 [details] Patch
Comment on attachment 109295 [details] Patch This patch much more aggressively excludes itself from QT.
Created attachment 109299 [details] Patch
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.
Created attachment 109300 [details] Patch
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...
Created attachment 109304 [details] Patch
Comment on attachment 109304 [details] Patch Apparent problem in winbase.h, trying to change the include ordering. Let's wait for the windows builder...
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.
Created attachment 109316 [details] Patch
Comment on attachment 109316 [details] Patch it seems capturestackbacktrace is in dbghelp.h on some machines. let's see if the builders get happy.
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.
Created attachment 109494 [details] Patch
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 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 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.
Created attachment 109560 [details] Patch
(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 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 on attachment 109560 [details] Patch I'll land it myself after removing the enum.
Committed r96595: <http://trac.webkit.org/changeset/96595>
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
Fix coming shortly.
This caused bug 69424.
Created attachment 109956 [details] Patch
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.
Committed r96809: <http://trac.webkit.org/changeset/96809>