Summary: | Put the ScopedArgumentsTable's ScopeOffset array in some gigacage | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, ggaren, jfbastien, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 174917 | ||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2017-07-27 19:44:18 PDT
Created attachment 318003 [details]
the patch
Created attachment 318004 [details]
the patch
Attachment 318004 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/CagedUniquePtr.h:84: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedUniquePtr.h:135: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
Total errors found: 2 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 318012 [details]
the patch
Attachment 318012 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/CagedUniquePtr.h:84: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedUniquePtr.h:135: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
Total errors found: 2 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 318016 [details]
the patch
Fix windows.
Attachment 318016 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/CagedUniquePtr.h:84: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedUniquePtr.h:135: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
Total errors found: 2 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 318043 [details]
the patch
Attachment 318043 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/CagedUniquePtr.h:84: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedUniquePtr.h:135: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
Total errors found: 2 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 318043 [details]
the patch
r=me
Comment on attachment 318043 [details] the patch iew in context: https://bugs.webkit.org/attachment.cgi?id=318043&action=review r=me > Source/WTF/wtf/CagedUniquePtr.h:82 > +class CagedUniquePtr<kind, T[], typename std::enable_if<std::is_trivially_destructible<T>::value>::type> : public CagedPtr<kind, T> { It's neat that you don't need the size here! :D > Source/WTF/wtf/CagedUniquePtr.h:104 > + new (result + count) T(std::forward<Arguments>(arguments)...); I think you want to construct in increasing order. That's usually what C++ containers do, and it's observable by the values being constructed. > Source/WTF/wtf/CagedUniquePtr.h:133 > +class CagedUniquePtr<kind, T[], typename std::enable_if<!std::is_trivially_destructible<T>::value>::type> : public CagedPtr<kind, T> { This inherits CagedPtr's operator== but does the wrong with by not comparing m_count. That should never be an issue because you shouldn't be able to have to unique pointers to the same base, but different sizes. Right? > Source/WTF/wtf/CagedUniquePtr.h:157 > + new (result + count) T(std::forward<Arguments>(arguments)...); Ditto, construct in increasing order. > Source/WTF/wtf/CagedUniquePtr.h:183 > + this->m_ptr[m_count].~T(); This reverse order destruction is good though. > Source/WTF/wtf/Gigacage.cpp:127 > + if (checkedSize.hasOverflowed()) UNLIKELY (In reply to JF Bastien from comment #11) > Comment on attachment 318043 [details] > the patch > > iew in context: > https://bugs.webkit.org/attachment.cgi?id=318043&action=review > > r=me > > > Source/WTF/wtf/CagedUniquePtr.h:82 > > +class CagedUniquePtr<kind, T[], typename std::enable_if<std::is_trivially_destructible<T>::value>::type> : public CagedPtr<kind, T> { > > It's neat that you don't need the size here! :D > > > Source/WTF/wtf/CagedUniquePtr.h:104 > > + new (result + count) T(std::forward<Arguments>(arguments)...); > > I think you want to construct in increasing order. That's usually what C++ > containers do, and it's observable by the values being constructed. I'll leave it for now. I already tested it this way, and I don't think that any of our code cares. > > > Source/WTF/wtf/CagedUniquePtr.h:133 > > +class CagedUniquePtr<kind, T[], typename std::enable_if<!std::is_trivially_destructible<T>::value>::type> : public CagedPtr<kind, T> { > > This inherits CagedPtr's operator== but does the wrong with by not comparing > m_count. That should never be an issue because you shouldn't be able to have > to unique pointers to the same base, but different sizes. Right? Right! I think it's "correct" but I put in a FIXME and cited https://bugs.webkit.org/show_bug.cgi?id=175541 > > > Source/WTF/wtf/CagedUniquePtr.h:157 > > + new (result + count) T(std::forward<Arguments>(arguments)...); > > Ditto, construct in increasing order. > > > Source/WTF/wtf/CagedUniquePtr.h:183 > > + this->m_ptr[m_count].~T(); > > This reverse order destruction is good though. > > > Source/WTF/wtf/Gigacage.cpp:127 > > + if (checkedSize.hasOverflowed()) > > UNLIKELY |