Bug 190761 - [Win] Add function call name information to stack traces
Summary: [Win] Add function call name information to stack traces
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-19 13:53 PDT by Christopher Reid
Modified: 2018-10-24 18:12 PDT (History)
12 users (show)

See Also:


Attachments
Patch (8.25 KB, patch)
2018-10-19 14:35 PDT, Christopher Reid
Hironori.Fujii: review-
Details | Formatted Diff | Diff
Updated patch (7.92 KB, patch)
2018-10-23 16:32 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Updated Patch (7.91 KB, patch)
2018-10-23 16:46 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (8.34 KB, patch)
2018-10-23 23:35 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Reid 2018-10-19 13:53:49 PDT
Currently CRASH and ASSERT only prints out the stack frame addresses. It would be nice to also show the associated function names.
Comment 1 Christopher Reid 2018-10-19 14:35:58 PDT
Created attachment 352824 [details]
Patch
Comment 2 EWS Watchlist 2018-10-19 14:40:07 PDT
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.
Comment 3 Fujii Hironori 2018-10-21 20:37:37 PDT
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.
Comment 4 Fujii Hironori 2018-10-21 20:51:29 PDT
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.
Comment 5 Fujii Hironori 2018-10-21 21:02:23 PDT
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.
Comment 6 Christopher Reid 2018-10-23 16:32:49 PDT
Created attachment 353002 [details]
Updated patch
Comment 7 EWS Watchlist 2018-10-23 16:35:06 PDT
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.
Comment 8 Christopher Reid 2018-10-23 16:44:08 PDT
(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.
Comment 9 Christopher Reid 2018-10-23 16:46:32 PDT
Created attachment 353003 [details]
Updated Patch

Fixing some style issues
Comment 10 EWS Watchlist 2018-10-23 16:49:20 PDT
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.
Comment 11 Fujii Hironori 2018-10-23 18:23:24 PDT
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.
Comment 12 Christopher Reid 2018-10-23 23:08:44 PDT
(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.
Comment 13 Christopher Reid 2018-10-23 23:35:59 PDT
Created attachment 353023 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2018-10-24 07:27:22 PDT
Comment on attachment 353023 [details]
Patch for landing

Clearing flags on attachment: 353023

Committed r237382: <https://trac.webkit.org/changeset/237382>
Comment 15 WebKit Commit Bot 2018-10-24 07:27:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-10-24 07:28:26 PDT
<rdar://problem/45519578>