Summary: | [WTF] Use std::unique_ptr for StackTrace | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, benjamin, buildbot, cdumez, cmarcelo, commit-queue, darin, dbates, keith_miller, mark.lam, msaboff, saam, ticaiolima | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Yusuke Suzuki
2017-07-13 23:26:48 PDT
Created attachment 315405 [details]
Patch
Comment on attachment 315405 [details] Patch Clearing flags on attachment: 315405 Committed r219506: <http://trac.webkit.org/changeset/219506> All reviewed patches have been landed. Closing bug. Caio had brought up this issue before, and I told him at the time that I original chose to have captureStackTrace() return a raw pointer instead of a std::unique_ptr for a reason. I looked and found the discussion I was thinking of: https://bugs.webkit.org/show_bug.cgi?id=169454#c7. Michael had suggested using RefPtr which I think is not needed. A std::unique_ptr is indeed the right solution. The only reason that captureStackTrace() didn't return a std::unique_ptr instead of a raw pointer was because I didn't think to do so at the time and was writing bad C++ code. Thanks for the fix. Comment on attachment 315405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315405&action=review > Source/WTF/wtf/StackTrace.cpp:69 > + std::unique_ptr<StackTrace> trace(new (NotNull, fastMalloc(sizeToAllocate)) StackTrace()); How does the fastMalloc memory here get freed when the unique_ptr’s destructor is run? Maybe I’m missing something? Comment on attachment 315405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315405&action=review >> Source/WTF/wtf/StackTrace.cpp:69 >> + std::unique_ptr<StackTrace> trace(new (NotNull, fastMalloc(sizeToAllocate)) StackTrace()); > > How does the fastMalloc memory here get freed when the unique_ptr’s destructor is run? Maybe I’m missing something? Because class StackTrace is WTF_MAKE_FAST_ALLOCATED. (In reply to Mark Lam from comment #6) > Comment on attachment 315405 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315405&action=review > > >> Source/WTF/wtf/StackTrace.cpp:69 > >> + std::unique_ptr<StackTrace> trace(new (NotNull, fastMalloc(sizeToAllocate)) StackTrace()); > > > > How does the fastMalloc memory here get freed when the unique_ptr’s destructor is run? Maybe I’m missing something? > > Because class StackTrace is WTF_MAKE_FAST_ALLOCATED. In case you missed it, the purpose of the explicit call to fastMalloc() is not to choose fastMalloc vs system malloc, but to customize the allocation size. (In reply to Mark Lam from comment #4) > Caio had brought up this issue before, and I told him at the time that I > original chose to have captureStackTrace() return a raw pointer instead of a > std::unique_ptr for a reason. > > I looked and found the discussion I was thinking of: > https://bugs.webkit.org/show_bug.cgi?id=169454#c7. Michael had suggested > using RefPtr which I think is not needed. A std::unique_ptr is indeed the > right solution. The only reason that captureStackTrace() didn't return a > std::unique_ptr instead of a raw pointer was because I didn't think to do so > at the time and was writing bad C++ code. > > Thanks for the fix. It was Yusuke's suggestion BTW. Comment on attachment 315405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315405&action=review > Source/WTF/ChangeLog:12 > + And we also move WTFGetBackTrace from Assertions.cpp to > + StackTrace.cpp to consolidate stack trace related operations > + into StackTrace.cpp. OK, but it’s not great that this function is declared in Assertions.h but defined in StackTrace.cpp. (In reply to Darin Adler from comment #9) > Comment on attachment 315405 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315405&action=review > > > Source/WTF/ChangeLog:12 > > + And we also move WTFGetBackTrace from Assertions.cpp to > > + StackTrace.cpp to consolidate stack trace related operations > > + into StackTrace.cpp. > > OK, but it’s not great that this function is declared in Assertions.h but > defined in StackTrace.cpp. Let's remove WTFGetBacktrace and WTFPrintBacktrace! And move these functionalities to StackTrace. (In reply to Yusuke Suzuki from comment #10) > (In reply to Darin Adler from comment #9) > > Comment on attachment 315405 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=315405&action=review > > > > > Source/WTF/ChangeLog:12 > > > + And we also move WTFGetBackTrace from Assertions.cpp to > > > + StackTrace.cpp to consolidate stack trace related operations > > > + into StackTrace.cpp. > > > > OK, but it’s not great that this function is declared in Assertions.h but > > defined in StackTrace.cpp. > > Let's remove WTFGetBacktrace and WTFPrintBacktrace! > And move these functionalities to StackTrace. https://bugs.webkit.org/show_bug.cgi?id=174566 |