Bug 174921 – Put the ScopedArgumentsTable's ScopeOffset array in some gigacage

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

There's an argument to be made in favor of any of them.

[Filip Pizlo]

Created attachment 318003 [details]
the patch

[Filip Pizlo]

Created attachment 318004 [details]
the patch

[Build Bot]

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.

[Filip Pizlo]

Created attachment 318012 [details]
the patch

[Build Bot]

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.

[Filip Pizlo]

Created attachment 318016 [details]
the patch

Fix windows.

[Build Bot]

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.

[Filip Pizlo]

Created attachment 318043 [details]
the patch

[Build Bot]

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.

[Mark Lam]

Comment on attachment 318043 [details]
the patch

r=me

[JF Bastien]

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

[Filip Pizlo]

(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

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/220712/webkit

[Radar WebKit Bug Importer]

<rdar://problem/33879627>