[Win] Add function call name information to stack traces

• Component: Platform
• Status: RESOLVED FIXED

[Christopher Reid]

Currently CRASH and ASSERT only prints out the stack frame addresses. It would be nice to also show the associated function names.

[Christopher Reid]

Created attachment 352824 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/win/DbgHelperWin.cpp:26:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/DbgHelperWin.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/DbgHelperWin.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 6 files


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

[Fujii Hironori]

I support Michael Catanzaro's opinion.

[webkit-dev] WTFCrash()
https://lists.webkit.org/pipermail/webkit-dev/2018-March/029903.html

> Next, the low-quality backtrace. Does anyone think this is useful?

Backtrace is shown only in case of intentional crash (abort), mostly in assertion failure.
In assertion failure, the failed condition and the function name are shown.
No backtrace is shown in case of unintentional crash.

Anyway, there is StackTrace::dump in WebKit. I don't object to add Windows implementation for it if you like.

[Fujii Hironori]

Comment on attachment 352824 [details]
Patch

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

> Source/WTF/wtf/StackTrace.cpp:116
> +    SYMBOL_INFO* symbol = static_cast<SYMBOL_INFO*>(fastZeroedMalloc(sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(TCHAR)));

SYMBOL_INFO* symbol = static_cast<SYMBOL_INFO*>(..)
This is redundant.
auto symbol = static_cast<SYMBOL_INFO*>(..)

I think you don't need to allocate by using fastZeroedMalloc because it is not so big.

This is not a symbol. The variable name 'symbol' should be renamed. For example, 'symbolInfo' or 'buffer'.

> Source/WTF/wtf/win/DbgHelperWin.cpp:35
> +// It's possible for external code to call the library at the same time as webkit and cause memory corruption.

Nit: webkit → WebKit

> Source/WTF/wtf/win/DbgHelperWin.cpp:61
> +bool DbgHelper::SymFromAddress(HANDLE hProc, DWORD64 address, PDWORD64 displacement, PSYMBOL_INFO symbol)

Remove variable names instead of using UNUSED_PARAM.
bool DbgHelper::SymFromAddress(HANDLE, DWORD64, PDWORD64, PSYMBOL_INFO)

> Source/WTF/wtf/win/DbgHelperWin.h:35
> +class DbgHelper {

I think you should define a namespace instead of a class.

> Source/WTF/wtf/win/DbgHelperWin.h:39
> +    static bool SymFromAddress(HANDLE hProc, DWORD64 address, PDWORD64 displacement, PSYMBOL_INFO);

I prefer SYMBOL_INFO*, DWORD64* to PSYMBOL_INFO, PDWORD64 in WebKit.

[Fujii Hironori]

Comment on attachment 352824 [details]
Patch

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

> Source/WTF/wtf/StackTrace.cpp:136
> +        if (DbgHelper::SymFromAddress(hProc, (DWORD64)stack[i], 0, symbol))

I think you should use reinterpret_caset<DWORD64>(...) instead of C-style cast.

[Christopher Reid]

Created attachment 353002 [details]
Updated patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/win/DbgHelperWin.cpp:26:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/DbgHelperWin.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/DbgHelperWin.cpp:44:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/win/DbgHelperWin.cpp:45:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WTF/wtf/win/DbgHelperWin.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/DbgHelperWin.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/DbgHelperWin.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/win/DbgHelperWin.h:37:  The parameter name "symbolInfo" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 6 files


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

[Christopher Reid]

(In reply to Fujii Hironori from comment #4)
> Comment on attachment 352824 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352824&action=review
> 
> > Source/WTF/wtf/StackTrace.cpp:116
> > +    SYMBOL_INFO* symbol = static_cast<SYMBOL_INFO*>(fastZeroedMalloc(sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(TCHAR)));
> 
> SYMBOL_INFO* symbol = static_cast<SYMBOL_INFO*>(..)
> This is redundant.
> auto symbol = static_cast<SYMBOL_INFO*>(..)
> 
> I think you don't need to allocate by using fastZeroedMalloc because it is
> not so big.

I was using fastMalloc as this struct is a bit odd to allocate on the stack.
The structure's Name property is defined as CHAR Name[1]; I changed it to use a buffer similar to the example on https://docs.microsoft.com/en-us/windows/desktop/debug/retrieving-symbol-information-by-address. I'm not sure which method is preferred.


Also, FWIW I have found this type stack trace useful for getting a stack trace when a debugger cannot easily be attached. e.g. with asserts inconsistently seen in layout tests.

[Christopher Reid]

Created attachment 353003 [details]
Updated Patch

Fixing some style issues

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/win/DbgHelperWin.cpp:26:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/DbgHelperWin.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/DbgHelperWin.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 6 files


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

[Fujii Hironori]

Comment on attachment 353003 [details]
Updated Patch

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

Looks good to me.

> Source/WTF/ChangeLog:10
> +            - application's working directory

It soulds like it means BuildBot Layout Tests can't show the function names by this patch because it is executed from top directory while the build artifact is placed in 'build' dir.

[Christopher Reid]

(In reply to Fujii Hironori from comment #11)
> Comment on attachment 353003 [details]
> Updated Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353003&action=review
> 
> Looks good to me.
> 
> > Source/WTF/ChangeLog:10
> > +            - application's working directory
> 
> It soulds like it means BuildBot Layout Tests can't show the function names
> by this patch because it is executed from top directory while the build
> artifact is placed in 'build' dir.

I just verified it locally, SymInitialize will search for symbol files in both the current working directory and the directory containing the application executable. I'll clarify the ChangeLog before landing.

[Christopher Reid]

Created attachment 353023 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 353023 [details]
Patch for landing

Clearing flags on attachment: 353023

Committed r237382: <https://trac.webkit.org/changeset/237382>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45519578>

[Fujii Hironori]

It works.

https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Debug%20(Tests)/r237400%20(986)/results.html
https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Debug%20(Tests)/r237400%20(986)/http/tests/xmlhttprequest/response-access-on-error-stderr.txt
https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Debug%20(Tests)/r237400%20(986)/js/mozilla/strict/regress-532041-stderr.txt