Bug 171199 – [WTF] Move JSC tools/StackTrace to WTF and unify stack trace dump code

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

[Yusuke Suzuki]

[WTF] Move JSC tools/StackTrace to WTF and unify stack trace dump code

[Yusuke Suzuki]

Created attachment 307938 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 307938 [details]
Patch

We would like to unify things further.

[Yusuke Suzuki]

Created attachment 307951 [details]
Patch

[Yusuke Suzuki]

Created attachment 307952 [details]
Patch

[Mark Lam]

The Windows build failure seems to be relevant. Can you fix that first?  Thanks.

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r215715: <http://trac.webkit.org/changeset/215715>

[Yusuke Suzuki]

Committed r215717: <http://trac.webkit.org/changeset/215717>