610946 - Assertions about the instance sizes of native classes are too strong

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

Consider these search results:

ClassClosure.cpp:         
  AvmAssert(traits()->getSizeOfInstance() >= sizeof(ClassClosure));

MethodClosure.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(MethodClosure));

MethodClosure.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(MethodClosureClass));

ArrayClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(ArrayClass));

ArrayObject.cpp:         
  AvmAssert(traits()->getSizeOfInstance() >= sizeof(ArrayObject));

ClassClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(ClassClass));

DateObject.h:             
  AvmAssert(traits()->getSizeOfInstance() == sizeof(DateObject));

DateObject.h:             
  AvmAssert(traits()->getSizeOfInstance() == sizeof(DateObject));

ErrorClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(ErrorClass));

ErrorClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(ErrorObject));

ErrorClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(ErrorClass));

FunctionClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(FunctionClass));

MathClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(MathClass));

NamespaceClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(NamespaceClass));

ObjectClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(ObjectClass));

RegExpClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(RegExpClass));

RegExpObject.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(RegExpObject));

XMLClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(XMLClass));

XMLClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(QNameClass));

XMLListClass.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(XMLListClass));

XMLObject.cpp:         
  AvmAssert(traits()->getSizeOfInstance() == sizeof(XMLObject));

It's not clear why these assertions are here, but their effect is to prevent native subclasses of these native classes to add fields or even padding.

(I found this in ErrorClass.cpp because a ReferenceErrorObject is larger than an ErrorObject due to some changes in nativegen.py introduced by the exact tracing work; the size increase appears to be padding inserted by the C++ compiler.)

Note that in the case of ClassClosure and ArrayObject the test is >=, which it has to be in order for Tamarin to work at all - these classes /are/ subclassed by other native classes that add fields.

The assertions have been in the code since the tamarin repository was created.

Comment 1

[Lars T Hansen]

Why would we want there to be a guard against subclassing one native class by another native class?

- layout restrictions that used to exist but are now gone - plausible at least.

- native methods that are virtual on the AS3 level are called directly in the
  native code implementation, ie it's a guard against breaking some sort of
  contract - not very satisfactory as it's possible to add methods without
  increasing object size.

Edwin suggests that it could just be crypto-selftest code inserted during work on nativegen, ie, verify that nativegen computes the right result, without considering the possibility of subclasses.

In that vein, it could actually be an embedded assumption about the class hierarchy combined with a sanity check on nativegen: since ReferenceErrorObject is a native-class leaf and adds no slots relative to its parent its instance size should be no larger than its parent, ie, ErrorObject.  That is, the assertions are correct and my change to nativegen.py is actually the bug.

Comment 2

[Lars T Hansen]

Attachment: Patch

In the absence of any clear evidence to the contrary, turn '==' into '>=' in cases where that seems appropriate, and annotate all instances with a reference to this bug.

Comment 3

[Lars T Hansen]

Something deeper is going on here.  Even with the softer assertions in place there are assertions in the Traits code that fail.  It could appear that there's a dependency between the VM and nativegen that is subtle.  I've worked around the problem by fixing nativegen so that it does not generate empty structures just for the purpose of generating trivial tracer functions, but instead only generates tracers when they are needed; an #ifdef-based interlock with the code generated by exactgc.as makes this possible, if ugly.

Will cancel review request for now but keep the bug open, it would be useful to track the issue down so that it can be documented.

Comment 4

[Steven Johnson]

(In reply to comment #1)
> Edwin suggests that it could just be crypto-selftest code inserted during work
> on nativegen, ie, verify that nativegen computes the right result, without
> considering the possibility of subclasses.

Unlikely, since these asserts all predate the existence of nativegen by a wide margin IIRC.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.