[webkit-reviews] review granted: [Bug 174566] [WTF] Drop WTFGetBacktrace and WTFPrintBacktrace : [Attachment 316097] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 174566: [WTF] Drop WTFGetBacktrace and WTFPrintBacktrace
https://bugs.webkit.org/show_bug.cgi?id=174566

Attachment 316097: Patch

https://bugs.webkit.org/attachment.cgi?id=316097&action=review




--- Comment #13 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 316097
  --> https://bugs.webkit.org/attachment.cgi?id=316097
Patch

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

r=me with suggestions.

> Source/WTF/wtf/StackTrace.cpp:49
> +// Thatâs why GET_BACK_TRACE is a macro rather than an inline function.

Please fix non-ascii character in 'Thatâs'.

> Source/WTF/wtf/StackTrace.cpp:58
> +NEVER_INLINE NOT_TAIL_CALLED void StackTrace::captureCurrentStack(unsigned
numberOfFrames, unsigned framesToSkip)

I suggest renaming numberOfFrames to captureCapacity (see below for reasons).

> Source/WTF/wtf/StackTrace.cpp:62
> +    numberOfFrames = GET_BACK_TRACE(m_skippedFrames, numberOfFrames);

Change this to:
    unsigned numberOfFrames = GET_BACK_TRACE(m_skippedFrames, captureCapacity);

> Source/WTF/wtf/StackTrace.cpp:80
> +    unsigned numberOfFrames = framesToShow + framesToSkip;

I think a better name for "numberOfFrames" here is "maxFrameCaptureCapacity" or
simply "captureCapacity".  What we're actually communicating to
captureCurrentStack() is the capacity of the buffer that it can use to do the
capture.  We're not really indicating the numberOfFrames it will capture.

> Source/WTF/wtf/StackTrace.cpp:83
> +    trace->captureCurrentStack(numberOfFrames, framesToSkip);

Since storage capacity is allocated by StackTrace::capture(), but actual stack
capture is done by StackTrace::captureCurrentStack(), can we add a sanity check
assertion here like so to ensure that an overflow doesn't get introduced and go
un-noticed:
    ASSERT(((trace->stack() + trace->size()) -
&trace->m_skippedFrames[InternalFramesToSkip]) <= captureCapacity);

> Source/WTF/wtf/StackTrace.cpp:89
> +    captureCurrentStack(framesToShow + framesToSkip, framesToSkip);

While Darin said having a "numberOfFrames = framesToShow + framesToSkip"
beforehand makes the code harder to read (and I agree that "numberOfFrames"
doesn't add anything), I think precomputing "captureCapacity = framesToShow +
framesToSkip" is good because it communicates the meaning of the sum being the
capacity of the buffer.

> Source/WTF/wtf/StackTrace.h:91
> +public:

Let's make this protected so that FixedStackTraceBase cannot be instantiated
except via its subclass that allocates the inline storage.

> Source/WTF/wtf/StackTrace.h:100
> +    {

Since storage capacity is allocated by FixedStackTrace, but actual stack
capture is done by FixedStackTraceBase, can we add a sanity check assertion
here like so to ensure that an overflow doesn't get introduced and go
undetected:
    ASSERT(((m_stack + m_size) - &storage) * sizeof(void*) <=
sizeof(m_storage));

... or:
    ASSERT(((m_stack + m_size) - &storage) <= capacity());

... and add a private method in FixedStackTraceBase:
   constexpr unsigned capacity() const { return sizeof(m_storage) /
sizeof(void*); }