Bug 69390 – JSC objects need to know their own cell size at runtime.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

This information is necessary for the GC work that Oliver is doing.  We'll add the relevant information to the ClassInfo struct.

[Mark Hahnenberg]

Created attachment 109828 [details]
Patch

[Oliver Hunt]

Comment on attachment 109828 [details]
Patch

I think a simpler approach would just be to make the method table contain this information, as CREATE_METHOD_TABLE  already has the class name

[Mark Hahnenberg]

> I think a simpler approach would just be to make the method table contain this information, as CREATE_METHOD_TABLE  already has the class name

:-(

[Mark Hahnenberg]

Created attachment 109834 [details]
Patch

[Oliver Hunt]

Comment on attachment 109834 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109834&action=review

> Source/JavaScriptCore/runtime/WriteBarrier.h:50
> +    ASSERT(WTF::RemovePointer<T>::Type::s_info.cellSize == sizeof(typename WTF::RemovePointer<T>::Type));

This isn't a safe assertion

class A {
 ...
}

class B : A {
  int aNewField;
}

validateCell<A*>(&someB)

would fail this assertion as B's classinfo will (correctly) report a larger cell size than A's.

[Geoffrey Garen]

Comment on attachment 109834 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109834&action=review

> Source/JavaScriptCore/runtime/JSCell.h:66
> +        size_t cellSize() const;

I don't think JSCell needs this helper function. We can write classInfo()->cellSize at the call site.

>> Source/JavaScriptCore/runtime/WriteBarrier.h:50
>> +    ASSERT(WTF::RemovePointer<T>::Type::s_info.cellSize == sizeof(typename WTF::RemovePointer<T>::Type));
> 
> This isn't a safe assertion
> 
> class A {
>  ...
> }
> 
> class B : A {
>   int aNewField;
> }
> 
> validateCell<A*>(&someB)
> 
> would fail this assertion as B's classinfo will (correctly) report a larger cell size than A's.

Maybe you can turn this ASSERT into >=? That would still catch some bugs.

A better place for an ASSERT might be allocateCell<T>: ASSERT(T::s_info.cellSize == sizeof(T)).

[Oliver Hunt]

(In reply to comment #6)
> (From update of attachment 109834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109834&action=review
> 
> > Source/JavaScriptCore/runtime/JSCell.h:66
> > +        size_t cellSize() const;
> 
> I don't think JSCell needs this helper function. We can write classInfo()->cellSize at the call site.
> 
> >> Source/JavaScriptCore/runtime/WriteBarrier.h:50
> >> +    ASSERT(WTF::RemovePointer<T>::Type::s_info.cellSize == sizeof(typename WTF::RemovePointer<T>::Type));
> > 
> > This isn't a safe assertion
> > 
> > class A {
> >  ...
> > }
> > 
> > class B : A {
> >   int aNewField;
> > }
> > 
> > validateCell<A*>(&someB)
> > 
> > would fail this assertion as B's classinfo will (correctly) report a larger cell size than A's.
> 
> Maybe you can turn this ASSERT into >=? That would still catch some bugs.
> 
> A better place for an ASSERT might be allocateCell<T>: ASSERT(T::s_info.cellSize == sizeof(T)).
Yeah i spoke to mark and that's what he's doing :D

[Mark Hahnenberg]

Created attachment 109855 [details]
Patch

[Geoffrey Garen]

Comment on attachment 109855 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109855&action=review

> Source/JavaScriptCore/runtime/JSObject.h:494
> +inline size_t JSCell::cellSize() const
> +{
> +    return classInfo()->cellSize;
> +}

This function is unused. I still think it can be inlined at the call site when used.

[Mark Hahnenberg]

Created attachment 109860 [details]
Patch

[Geoffrey Garen]

Comment on attachment 109860 [details]
Patch

r=me

[Mark Hahnenberg]

Created attachment 109904 [details]
Fixing windows

[Geoffrey Garen]

Comment on attachment 109904 [details]
Fixing windows

r=me

[WebKit Review Bot]

Comment on attachment 109904 [details]
Fixing windows

Clearing flags on attachment: 109904

Committed r96831: <http://trac.webkit.org/changeset/96831>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.