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

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied 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 315607: Patch

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




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

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

I think you have bugs to fix.

> Source/WTF/wtf/StackTrace.cpp:76
> +    // StackTrace includes one frame.
>      ASSERT(capacity >= 1);
>      return sizeof(StackTrace) + (capacity - 1) * sizeof(void*);

I think this is wrong now.  m_stack used to be the extra pointer space for that
1 frame.  You've removed it for 64-bit.

> Source/WTF/wtf/StackTrace.h:73
> +    // top 2 frames which is of no interest. Setting up the fields layout
this way

/which is/which are/.

> Source/WTF/wtf/StackTrace.h:80
>	   struct {
> +	       void** m_stack;
>	       int m_size;
>	       int m_capacity;
>	   };

There are 2 bugs here:
1. This will turn into 3 words on 32-bit which won't work.  The rest of the
code seems to expect sizeof StackTrace == 2 words.
2. On 64-bit, fFor FixedStackTrace with 0 skip (i.e. you'll only skip 1 frame:
captureCurrentStack), m_skippedFrame1 is expected to be in used.  However, it
overlaps m_size and m_capacity.

I think you should add a static_assert to ensure that the size of StackTrace is
what you expect to prevent people from adding fields in the future and getting
a silent failure.

> Source/WTF/wtf/StackTrace.h:100
> +    void* m_storage[FramesToShow + FramesToSkip + 1 - 2];

Can you use constexpr ints to give names to this 1 and 2?  If not, please add a
comment to document what values they represent.  I presume the 1 is for
skipping StackTrace::captureCurrentStack, and the 2 is for sizeof StackTrace
header in frames.