486418 - AVMPLUS_UNCHECKED_HACK needs to apply only to prototype functions

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Recent MethodSignature code unintentionally allowed this code to apply to methods as well as prototype functions, which is incorrect.

Comment 1

[Steven Johnson]

Attachment: Patch

Fix unchecked_hack to apply only to prototype functions. Includes acceptance test.

Comment 2

[Edwin Smith]

Comment on attachment 370526 [details] [diff] [review]
Patch

- "prototype functions" means functions on SomeObject.prototype, but this applies to all functions that are closures as opposed to methods.  ie all methodenv's attached to a Function (built with OP_newfunction).  So, a FUNCTION flag makes more sense than PROTOFUNC.

- we never test with !defined(AVMPLUS_UNCHECKED_HACK), if its not too much churn, we should rip out that ifdef.  (not a new bug, so its your choice).

Comment 3

[Steven Johnson]

(In reply to comment #2)
> (From update of attachment 370526 [details] [diff] [review])
> - "prototype functions" means functions on SomeObject.prototype, but this
> applies to all functions that are closures as opposed to methods.  ie all
> methodenv's attached to a Function (built with OP_newfunction).  So, a FUNCTION
> flag makes more sense than PROTOFUNC.

this flag really means "makeIntoPrototypeFunction has been called on me", not "I am a function on a prototype object" -- I'm happy to change the name but imho FUNCTION would be a poor choice (smells ripe for evil preprocessors to use).
 
> - we never test with !defined(AVMPLUS_UNCHECKED_HACK), if its not too much
> churn, we should rip out that ifdef.  (not a new bug, so its your choice).

agreed, I'll look at nukage

Comment 4

[Steven Johnson]

(1) I'm tempted to leave it named PROTOFUNC and just comment as per above to avoid confusion

(2) part of me is tempted to leave in the #ifdef AVMPLUS_UNCHECKED_HACK bits just to make clear where the extent of the hack is. but then again, it's misleading to have ifdefs that aren't ever disabled -- perhaps changing the ifdefs to comments would be more appropriate at this point.

Comment 5

[Edwin Smith]

sounds good to me

Comment 6

[Steven Johnson]

pushed to redux as changeset:   1662:fa61bc214f42

Comment 7

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.