[WTF] Drop WTFGetBacktrace and WTFPrintBacktrace
Created attachment 315607 [details] Patch
Comment on attachment 315607 [details] Patch Attachment 315607 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4131064 New failing tests: storage/websql/execute-sql-rowsAffected.html http/tests/canvas/canvas-tainted-after-draw-image.html
Created attachment 315611 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315607 [details] 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.
Comment on attachment 315607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315607&action=review >> Source/WTF/wtf/StackTrace.cpp:76 >> 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. Oops, right. I'll change the design slightly to fix the issues. >> 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/. Right, fixed. >> Source/WTF/wtf/StackTrace.h:80 >> }; > > 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. Oops, right. I'll change the design to always skip 2 frames in both. And m_capacity is no longer necessary (nobody uses & wants it). And I'll insert static_assert(). >> 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. Now, I've changed the design. And as a result, m_storage becomes m_storage[FramesToShow + FramesToSkip]. I think this is very descriptive.
Created attachment 315781 [details] Patch
Comment on attachment 315781 [details] 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.
Comment on attachment 315781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315781&action=review >> 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. Here is the comment I would write: "StackTrace object needs to be big enough so we can temporarily overwrite it with 2 skipped frames during capture."
Comment on attachment 315781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315781&action=review I'm going to r- this. I think it's worth taking another look after all the changes has been made. Thanks. > Source/WTF/wtf/StackTrace.cpp:59 > + numberOfFrames = GET_BACK_TRACE(&m_skippedFrame0, numberOfFrames); Let's add StackTrace::InternalFramesToSkip (see below) to numberOfFrames and framesToSkip before this. >> 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. Let's rename capacity to requestedFramesToCapture. > Source/WTF/wtf/StackTrace.cpp:74 > + // StackTrace includes first two skipped frames. > + ASSERT(capacity >= StackTrace::SkippedFrames); > + return sizeof(StackTrace) + (capacity - StackTrace::SkippedFrames) * sizeof(void*); Let's rename StackTrace::SkippedFrames to StackTrace::InternalFramesToSkip. I think we can simplify this not relying on the client to add StackTrace::InternalFramesToSkip. Instead, let's just add it here. This is why I suggest renaming capacity to requestedFramesToCapture. It does not include StackTrace::InternalFramesToSkip, and therefore, we don't need this assert anymore. And the equation reduces to: sizeof(StackTrace) + requestedFramesToCapture * sizeof(void*); >> Source/WTF/wtf/StackTrace.cpp:80 >> + ASSERT(framesToSkip >= 0); > > The need for these assertions shows us that our interfaces should be using "unsigned" rather than "int". I agree that it would be nice if we can switch to using unsigned instead. > Source/WTF/wtf/StackTrace.cpp:82 > + framesToSkip += StackTrace::SkippedFrames; Remove this and the comment above it. >> 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. It will be fastFreed because the StackTrace class is WTF_MAKE_FAST_ALLOCATED. The purpose of the use of placement new here is not to select fastMalloc as the allocator, but to control the size of the allocation to add space for the captured trace. > Source/WTF/wtf/StackTrace.cpp:96 > + // Skip 2 additional frames i.e. FixedStackTraceBase and StackTrace::captureCurrentStack. > + framesToSkip += StackTrace::SkippedFrames; Remove these. >> 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 Let's rename SkippedFrames to InternalFramesToSkip. Also add the comment here that these frames are StackTrace::capture and StackTrace::captureCurrentStack. >> Source/WTF/wtf/StackTrace.h:85 >> }; > > 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]; This is a great idea. >>> 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. > > Here is the comment I would write: > > "StackTrace object needs to be big enough so we can temporarily overwrite it with 2 skipped frames during capture." Let's rename StackTrace::SkippedFrames to StackTrace::InternalFramesToSkip.
Comment on attachment 315781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315781&action=review Thank you for your reviews! >> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:184 >> + const int framesToShow = 31; > > Could be good to have some rationale for why we chose 31. Hm, this is derived from the original implementation. And I'm not sure why we choose 31 here. I guess there is no rationale, any small but enough value is ok here. >> 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. I think putting this for now is safer way. >> Source/WTF/wtf/Assertions.cpp:248 >> + const int framesToShow = 31; > > Could be good to have some rationale for why we chose 31. Ditto. >> Source/WTF/wtf/Assertions.cpp:251 >> + FixedStackTrace<framesToShow, framesToSkip> stackTrace { }; > > No need for the { } here. Fixed. >> 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. Sounds nice, fixed. >> Source/WTF/wtf/StackTrace.cpp:59 >> + numberOfFrames = GET_BACK_TRACE(&m_skippedFrame0, numberOfFrames); > > Let's add StackTrace::InternalFramesToSkip (see below) to numberOfFrames and framesToSkip before this. OK, fixed. >>> 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. > > Let's rename capacity to requestedFramesToCapture. OK, I've dropped ALWAYS_INLNINE (use inline). And use the name requestedFramesToCapture. >> Source/WTF/wtf/StackTrace.cpp:74 >> + return sizeof(StackTrace) + (capacity - StackTrace::SkippedFrames) * sizeof(void*); > > Let's rename StackTrace::SkippedFrames to StackTrace::InternalFramesToSkip. > > I think we can simplify this not relying on the client to add StackTrace::InternalFramesToSkip. Instead, let's just add it here. This is why I suggest renaming capacity to requestedFramesToCapture. It does not include StackTrace::InternalFramesToSkip, and therefore, we don't need this assert anymore. And the equation reduces to: sizeof(StackTrace) + requestedFramesToCapture * sizeof(void*); Sounds nice. Fixed. >>> Source/WTF/wtf/StackTrace.cpp:80 >>> + ASSERT(framesToSkip >= 0); >> >> The need for these assertions shows us that our interfaces should be using "unsigned" rather than "int". > > I agree that it would be nice if we can switch to using unsigned instead. OK, fixed. >> 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". Then, we dropped this comment. >> Source/WTF/wtf/StackTrace.cpp:82 >> + framesToSkip += StackTrace::SkippedFrames; > > Remove this and the comment above it. And dropped this. >>> 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. > > It will be fastFreed because the StackTrace class is WTF_MAKE_FAST_ALLOCATED. The purpose of the use of placement new here is not to select fastMalloc as the allocator, but to control the size of the allocation to add space for the captured trace. OK, dropped (). >> Source/WTF/wtf/StackTrace.cpp:91 >> + : StackTrace() > > This should not be needed. Dropped. >> Source/WTF/wtf/StackTrace.cpp:96 >> + framesToSkip += StackTrace::SkippedFrames; > > Remove these. Dropped. >> 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. OK, removed. >>> 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 > > Let's rename SkippedFrames to InternalFramesToSkip. Also add the comment here that these frames are StackTrace::capture and StackTrace::captureCurrentStack. OK, I've chose InternalFramesToSkip. And we can just use constexpr. Fixed. >> Source/WTF/wtf/StackTrace.h:47 >> } > > 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. Fixed. >> Source/WTF/wtf/StackTrace.h:70 >> + 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; Good catch! I think using WTF_MAKE_NONCOPYABLE(StackTrace); and protected StackTrace() = default is the best. >>> Source/WTF/wtf/StackTrace.h:85 >>> }; >> >> 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]; > > This is a great idea. Fixed :) >>>> 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. >> >> Here is the comment I would write: >> >> "StackTrace object needs to be big enough so we can temporarily overwrite it with 2 skipped frames during capture." > > Let's rename StackTrace::SkippedFrames to StackTrace::InternalFramesToSkip. Fixed. >> Source/WTF/wtf/StackTrace.h:98 >> + ALWAYS_INLINE FixedStackTrace() > > The ALWAYS_INLINE here seems like overkill. Same reason as above. This is necessary here. We do not want to add this FixedStackTrace in the stack trace. We would like to ensure that this is inlined.
Created attachment 316094 [details] Patch
Created attachment 316097 [details] Patch
Comment on attachment 316097 [details] 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*); }
Comment on attachment 316097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316097&action=review >> 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'. Fixed, thank you. >> 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). OK, fixed. >> Source/WTF/wtf/StackTrace.cpp:62 >> + numberOfFrames = GET_BACK_TRACE(m_skippedFrames, numberOfFrames); > > Change this to: > unsigned numberOfFrames = GET_BACK_TRACE(m_skippedFrames, captureCapacity); Fixed. >> 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. Yeah, right. Fixed. >> 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); Sounds very nice. Added the assertion. >> 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. OK, sounds reasonable. Fixed. >> 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. Nice! Fixed. > Source/WTF/wtf/StackTrace.h:99 > + ALWAYS_INLINE FixedStackTrace() > + : FixedStackTraceBase(FramesToShow, FramesToSkip) Oops. In debug build, even if we specify ALWAYS_INLINE, we can get trace in the result. I think relying this behavior is not good. Instead, I propose the function, like, static FixedStackTrace<FramesToShow, FramesToSkip>::capture() ->FixedStackTrace. >> 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*); } Good.