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

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 315781: Patch

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




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 315781
  --> https://bugs.webkit.org/attachment.cgi?id=315781
Patch

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

I think we should be using unsigned here almost every place we are using int.

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:184
> +    const int framesToShow = 31;

Could be good to have some rationale for why we chose 31.

> Source/WTF/WTF.xcodeproj/project.pbxproj:62
> +		3337DB9CE743410FAF076E17 /* StackTrace.cpp in Sources */ = {isa
= PBXBuildFile; fileRef = 313EDEC9778E49C9BEA91CFC /* StackTrace.cpp */;
settings = {COMPILER_FLAGS = "-fno-optimize-sibling-calls"; }; };

Would be nice if we could do this in the source file with #pragma instead, but
I don’t think clang has a way to do this just for certain functions.

> Source/WTF/wtf/Assertions.cpp:248
> +    const int framesToShow = 31;

Could be good to have some rationale for why we chose 31.

> Source/WTF/wtf/Assertions.cpp:251
> +    FixedStackTrace<framesToShow, framesToSkip> stackTrace { };

No need for the { } here.

> Source/WTF/wtf/StackTrace.cpp:48
> +// We do not want to add more frames to call these functions. That's why we
define GET_BACK_TRACE as macro.

I would write:

    // We do not want to add more frames to call these functions, even when
compiling debug configurations with inlining disabled.
    // That’s why GET_BACK_TRACE is a macro rather than an inline function.

> Source/WTF/wtf/StackTrace.cpp:70
> +static ALWAYS_INLINE size_t instanceSize(int capacity)

ALWAYS_INLINE seems like overkill here. Just a normal inline should be fine,
and in fact this is even a simple enough function that the compiler would
decide to inline it even if we didn’t specify inline. We normally use
ALWAYS_INLINE only when there is a serious reason to force inlining, such as a
critical performance dependency or something else like that.

> Source/WTF/wtf/StackTrace.cpp:80
> +    ASSERT(framesToShow >= 0);
> +    ASSERT(framesToSkip >= 0);

The need for these assertions shows us that our interfaces should be using
"unsigned" rather than "int".

> Source/WTF/wtf/StackTrace.cpp:81
> +    // Skip 2 additional frames i.e. StackTrace::capture and
StackTrace::captureCurrentStack.

Should just use a colon here instead of "i.e." because "i.e." means "in other
words" and listing the actual function names is not "in other words".

> Source/WTF/wtf/StackTrace.cpp:85
>      std::unique_ptr<StackTrace> trace(new (NotNull,
fastMalloc(sizeToAllocate)) StackTrace());

No need for the "()" after "StackTrace".

Not new to this patch, but I don’t see any code that would make the delete
operator use fastFree rather than free when deallocating one of these.

> Source/WTF/wtf/StackTrace.cpp:91
> +    : StackTrace()

This should not be needed.

> Source/WTF/wtf/StackTrace.cpp:97
> +    int numberOfFrames = framesToShow + framesToSkip;

I don’t think that putting this into a local variable makes the code easier to
read.

> Source/WTF/wtf/StackTrace.h:39
> +    static const constexpr int SkippedFrames { 2 };

I’m surprised that we need both const and constexpr: I’m not enough of an
expert on constexpr to know what the correct idiom is. For an int does
constexpr do anything different from const?

Would be a little more elegant if this constant could be private rather than
public because it’s not part of the interface to this class. That might require
moving the static_assert inside a member function, but really I think that
would be good. The static_assert could go inside
StackTrace::captureCurrentStack right next to the code that depends on it. If
we did that, then instanceSize would be need to also be a private static member
function.

It’s confusing to use the almost identical names "skipped frames" and "frames
to skip" for 5 or 6 different things:

    1) Number of frames we always need to skip in StackTrace.
    2a) Number of additional skipped frames requested by the caller.
    2b) Number of additional skipped frames passed as a FixedStackTrace
template argument.
    3) Total number of frames we need to skip: (1) + (2).
    4) Storage in the StackTrace union to guarantee we have space for (1) we
always need to skip.
    5) Storage for additional skipped frames in FixedStackTrace to give use
space for (2).

I think we should have five different names. Here is my cut at suggestions (add
"m_" and capitalize as appropriate):

    1) baseFramesToSkip or internalFramesToSkip
    2) additionalFramesToSkip or just framesToSkip
    3) totalFramesToSkip
    4) internalSkippedFrameStorage
    5) additionalSkippedFrameStorage or just skippedFrameStorage

> Source/WTF/wtf/StackTrace.h:47
>      void* const * stack() const
>      {
> -	   if (!m_capacity)
> -	       return m_borrowedStack;
>	   return m_stack;
>      }

Can this be formatted as a one-liner instead of being laid out vertically?
Generally I prefer to put functions outside the class definition if they aren’t
one-liners, even if they are inline.

> Source/WTF/wtf/StackTrace.h:70
> +protected:
> +    NEVER_INLINE NOT_TAIL_CALLED void captureCurrentStack(int
numberOfFrames, int framesToskip);

Should also add a protected default constructor:

    StackTrace() = default;

Because otherwise there is a public constructor and it should be a compilation
error to try to use that.

Also should delete the copy constructor and assignment operator for the same
reason. Could use Noncopyable.h or just:

    StackTrace(const StackTrace&) = delete;
    void operator=(const StackTrace&) = delete;

> Source/WTF/wtf/StackTrace.h:85
>	   struct {
>	       void* m_skippedFrame0;
>	       void* m_skippedFrame1;
>	   };

Seems like this should be an array of size SkippedFrames rather than two
separate members. That would make the static_assert less necessary.

    void* m_skippedFrames[SkippedFrames];

> Source/WTF/wtf/StackTrace.h:88
> +static_assert(sizeof(StackTrace) == sizeof(void*) *
StackTrace::SkippedFrames, "StackTrace members should be placed in skipped 2
frames.");

I don’t understand what "placed in skipped" means here. There seems to be a
wording problem.

> Source/WTF/wtf/StackTrace.h:98
> +    ALWAYS_INLINE FixedStackTrace()

The ALWAYS_INLINE here seems like overkill. Same reason as above.