Bug 174921

Summary: Put the ScopedArgumentsTable's ScopeOffset array in some gigacage
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, ggaren, jfbastien, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 174917    
Attachments:
Description Flags
the patch
none
the patch
none
the patch
none
the patch
none
the patch mark.lam: review+

Description Filip Pizlo 2017-07-27 19:44:18 PDT
There's an argument to be made in favor of any of them.
Comment 1 Filip Pizlo 2017-08-12 22:22:45 PDT
Created attachment 318003 [details]
the patch
Comment 2 Filip Pizlo 2017-08-12 23:02:57 PDT
Created attachment 318004 [details]
the patch
Comment 3 Build Bot 2017-08-12 23:06:12 PDT
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.
Comment 4 Filip Pizlo 2017-08-13 11:47:50 PDT
Created attachment 318012 [details]
the patch
Comment 5 Build Bot 2017-08-13 11:50:31 PDT
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.
Comment 6 Filip Pizlo 2017-08-13 15:48:46 PDT
Created attachment 318016 [details]
the patch

Fix windows.
Comment 7 Build Bot 2017-08-13 15:51:12 PDT
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.
Comment 8 Filip Pizlo 2017-08-14 09:53:14 PDT
Created attachment 318043 [details]
the patch
Comment 9 Build Bot 2017-08-14 09:56:03 PDT
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 10 Mark Lam 2017-08-14 10:09:22 PDT
Comment on attachment 318043 [details]
the patch

r=me
Comment 11 JF Bastien 2017-08-14 10:13:51 PDT
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
Comment 12 Filip Pizlo 2017-08-14 11:29:17 PDT
(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
Comment 13 Filip Pizlo 2017-08-14 11:33:31 PDT
Landed in https://trac.webkit.org/changeset/220712/webkit
Comment 14 Radar WebKit Bug Importer 2017-08-14 11:34:45 PDT
<rdar://problem/33879627>