Bug 171199 - [WTF] Move JSC tools/StackTrace to WTF and unify stack trace dump code
Summary: [WTF] Move JSC tools/StackTrace to WTF and unify stack trace dump code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-23 11:00 PDT by Yusuke Suzuki
Modified: 2017-04-24 20:23 PDT (History)
1 user (show)

See Also:


Attachments
Patch (27.51 KB, patch)
2017-04-23 11:02 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (43.06 KB, patch)
2017-04-23 20:45 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (43.17 KB, patch)
2017-04-23 20:57 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-04-23 11:00:56 PDT
[WTF] Move JSC tools/StackTrace to WTF and unify stack trace dump code
Comment 1 Yusuke Suzuki 2017-04-23 11:02:17 PDT
Created attachment 307938 [details]
Patch
Comment 2 Yusuke Suzuki 2017-04-23 19:07:15 PDT
Comment on attachment 307938 [details]
Patch

We would like to unify things further.
Comment 3 Yusuke Suzuki 2017-04-23 20:45:00 PDT
Created attachment 307951 [details]
Patch
Comment 4 Yusuke Suzuki 2017-04-23 20:57:03 PDT
Created attachment 307952 [details]
Patch
Comment 5 Mark Lam 2017-04-24 10:14:37 PDT
The Windows build failure seems to be relevant. Can you fix that first?  Thanks.
Comment 6 Mark Lam 2017-04-24 11:10:47 PDT
Comment on attachment 307952 [details]
Patch

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

LGTM but need to fix broken Windows build.

> Source/JavaScriptCore/ChangeLog:8
> +        This patch adds an utility method to produce demangled names with dladdr.

typo: /an utility/a utility/.

> Source/WTF/wtf/DefaultFree.h:33
> +struct DefaultFree {

I suggest renaming this to SystemFree to match our parlance elsewhere e.g. see DebugHeap::DebugHeap()'s referral to "System Malloc"  in bmalloc/heap/DebugHeap.cpp.

> Source/WTF/wtf/Platform.h:673
> +#endif

I suggest adding // PLATFORM(GTK) after the #endif.

> Source/WTF/wtf/Platform.h:675
> +#endif

I suggest adding // OS(DARWIN) || OS(LINUX) after the #endif.

> Source/WTF/wtf/StackTrace.h:61
> +        DemangleEntry(const char* mangledName, const char* demangledName)
> +            : m_mangledName(mangledName)
> +            , m_demangledName(demangledName)
> +        { }

I suggest making this constructor private and adding StackTrace as a friend of DemangleEntry.  This way, you can control the life-cycle of DemangleEntry tightly and ensure that demangledName is indeed allocated with the system malloc to match how we free it.
Comment 7 Yusuke Suzuki 2017-04-24 19:41:20 PDT
Comment on attachment 307952 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:8
>> +        This patch adds an utility method to produce demangled names with dladdr.
> 
> typo: /an utility/a utility/.

Fixed.

>> Source/WTF/wtf/DefaultFree.h:33
>> +struct DefaultFree {
> 
> I suggest renaming this to SystemFree to match our parlance elsewhere e.g. see DebugHeap::DebugHeap()'s referral to "System Malloc"  in bmalloc/heap/DebugHeap.cpp.

This is originally aligned to std::default_delete. But, since we have bmalloc's fastMalloc along with the system malloc, SystemFree seems more descriptive.
Renamed.

>> Source/WTF/wtf/Platform.h:673
>> +#endif
> 
> I suggest adding // PLATFORM(GTK) after the #endif.

Fixed.

>> Source/WTF/wtf/Platform.h:675
>> +#endif
> 
> I suggest adding // OS(DARWIN) || OS(LINUX) after the #endif.

Fixed.

>> Source/WTF/wtf/StackTrace.h:61
>> +        { }
> 
> I suggest making this constructor private and adding StackTrace as a friend of DemangleEntry.  This way, you can control the life-cycle of DemangleEntry tightly and ensure that demangledName is indeed allocated with the system malloc to match how we free it.

Sounds very nice! Fixed.
Comment 8 Yusuke Suzuki 2017-04-24 19:53:57 PDT
Committed r215715: <http://trac.webkit.org/changeset/215715>
Comment 9 Yusuke Suzuki 2017-04-24 20:23:31 PDT
Committed r215717: <http://trac.webkit.org/changeset/215717>