456852 - Different runtime errors when -Ojit set in acceptance test run

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Dan Schaffer]

on all platforms two failures because different runtime errors when vm has -Dforcemir flag set.

tests are set to expectedfail in testconfig.txt
run:
$ ./runtests.py --vmargs=-Dforcemir 
as3/RuntimeErrors/Errors1115NotAConstructor.as 
es4/vector/nonindexproperty.as

for Errors1115NotAConstructor.as: 
nonindexproperty.as: actual=RangeError: Error #1125, actual=ReferenceError: Error #1056
Errors1115NotAConstructor.as: actual=TypeError: Error #1115, expected= TypeError: Error #1007

Comment 1

[Rick Reitmaier]

still there after redux merge (using -Ojit)

Comment 2

[Rick Reitmaier]

ok, going back in tamarin-central history I'm finding this has 
occurred since the very first builds.

Dan, can you confirm and then adjust the test cases accordingly.

Comment 3

[Dan Smith]

Rick, any reason to not make the generated error consistent rather than changing the tests?

Comment 4

[Edwin Smith]

the primary bug is a VM bug: is that you get different errors with -Dinterp and -Dforcemir.  it's a VM bug either way.  

It's only a bug in the test case if the expected error (the one thrown by -Dinterp) is wrong, and the JIT error happens to be correct.

Comment 5

[Dan Schaffer]

on tamarin-redux default jit matches -Dforcemir test result. In tamarin-central default jit matches interp test result.  Wondering if default is -Dforcemir in redux?

Comment 6

[Rick Reitmaier]

Attachment: ver1 - applies against redux change 1006

change to allow redux builds to support 'mixed' (interp+jit) modes once again.

Comment 7

[Rick Reitmaier]

Attachment: ver2 - applies against redux change 1006

prior patch was incorrect file

Comment 8

[Edwin Smith]

Comment on attachment 343585 [details] [diff] [review]
ver2 - applies against redux change 1006

I'm pretty sure the old semantics for setMIREnabled(t/f) were true = mixed,
false = interp.  for example, it was being used on arm to disable the jit
entirely.  we should preserve the old semantics.  I'm marking this + anyway
because i dont need to re-review it.

can the tri-state code be encoded with flags so we can test a bit? just a hair
quicker.  not important for abcparser, but don't we also have to test this mode
in verifyEnter()?

Comment 9

[Rick Reitmaier]

Ok, fixed the mirEnabled semantics.

I'd like to keep the tri-state, since it makes it obvious which modes are supported and what are valid combinations.

On the performance front, verifyEnter() does an IsMirEnabled() call which checks runmode against RM_mixed.  RM_mixed is defined as zero which should compile down to a zero equality check, which should be at least as fast on most platforms as a bit test.

Comment 10

[Rick Reitmaier]

Dan, this change will break the existing tests so I also need to revert the change made on the testcases.  Will post another patch once I have that change and any others that result from the discussion above.

Comment 11

[Rick Reitmaier]

pushed as changeset #1015.

Comment 12

[Rick Reitmaier]

last comment should have stated into the tamarin-redux build.

Comment 13

[Rick Reitmaier]

Need to investigate the issue further.  The submitted change (along with change #1020) fix the differences between tamarin-central and tamarin-redux.

But the problem for which this bug was originally written is still there.  Also, testing with a build made from changelist #632 (tamarin-central pre-nanojit) the same results are apparent.  

So this bug has most likely been in tamarin since very early on.

Comment 14

[Brent Baker]

The current PPC nanojit implementation (redux: 1198:63bf07dcdb6d) is not having an issue with these failures like the other JIT backends...

as3/RuntimeErrors/Errors1115NotAConstructor.as 
es4/vector/nonindexproperty.as

Comment 15

[Edwin Smith]

The reason the PPC backend is not showing the error is because the JIT fails on the test function, forcing it to be interpreted which suppresses the error.

The JIT fails due to running out of stack frame space, which is not a bug but a resource limitation based on the JIT design.

The reason the 1K stack frame limit is overflowing is we allocate 120 bytes for local variables and 792 bytes for the ExceptionFrame structure, leaving not enough leftover space for spill variables, temporary outgoing args, etc.

Comment 16

[Edwin Smith]

on x86, the ExceptionFrame struct is only 96 bytes.

Comment 17

[Lars T Hansen]

32 registers of 4 bytes each should be 128 bytes; even if we figure 8 bytes for portability that would be 256.  I assume some of the rest must be FP registers... and then what?  Altivec?  Status registers?

Comment 18

[Edwin Smith]

exactly, altivec.  from setjmp.h:

/*
 *  _JBLEN is number of ints required to save the following:
 *  r1, r2, r13-r31, lr, cr, ctr, xer, sig  == 26 register_t sized
 *  fr14 -  fr31 = 18 doubles
 *  vmask, 32 vector registers = 129 ints
 *  2 ints to get all the elements aligned
 *
 *  register_t is 2 ints for ppc64 threads
 */
#define _JBLEN64    (26*2 + 18*2 + 129 + 1)
#define _JBLEN32    (26 + 18*2 + 129 + 1)
#define _JBLEN_MAX  _JBLEN64


surprising that the vector registers are not considered scratch

Comment 19

[Dan Schaffer]

the two tests as3/RuntimeErrors/Errors1115NotAConstructor.as 
es4/vector/nonindexproperty.as started to pass in -Ojit in tamarin-redux 1724.

Comment 20

[Brent Baker]

This needs to be reverted. Looking into the problem it appears that the issue is that runtests.py is no longer able to determine that the platform is PPC, and assumes that the platform is x64 instead of ppc. Prior to changeset 1724:c321a53ff488 the configuration was calculated to be 'ppc64-mac-tvm-release-Ojit' but is now being calculated as 'x64-mac-tvm-release-Ojit'.

What was removed from the testconfig was actually saying that all platforms expect this to fail EXCEPT for PPC.

Comment 21

[Dan Schaffer]

reverted and fixed.  The bug in runtestBase.py was caused by short circuiting the if/else in determinOS()  the two tests as3/RuntimeErrors/ Errors1115NotAConstructor.as 
es4/vector/nonindexproperty.as still fail as before.

Comment 22

[Edwin Smith]

status check?

Comment 23

[Brent Baker]

This is still happening in tamarin-redux 2530 on all platforms EXCEPT for PPC. PPC produces the same runtime error with and without -Ojit.

$AVM as3/Vector/nonindexproperty.abc -> Passes all tests
$AVM -Ojit as3/Vector/nonindexproperty.abc -> Fails:

index -6 throws exception because non-uint property = RangeError: Error #1125 
FAILED! expected: ReferenceError: Error #1056


$AVM as3/RuntimeErrors/Error1115NotAConstructor.abc -> Passes
$AVM -Ojit as3/RuntimeErrors/Error1115NotAConstructor.abc -> Fails

Runtime Error = TypeError: Error #1007 FAILED! 
expected: TypeError: Error #1115

Comment 24

[Rick Reitmaier]

as3/Vector/nonindexproperty.as :

The JIT specializes OP_setproperty to call ObjectVectorObject_setIntProperty, when -6 is passed in the call this results in a OutOfRangeError(1125).

In the interp case, we don't have the optimization so we go through VectorBaseObject::setAtomProperty which then throws a kWriteSealedError(1056)

On possible fix is to add code to setAtomProperty() which checks to see if property being set is an int and if so throws a OutOfRangeError.

But I'm not convinced that we should increase code size just to align our error codes so I recommend not fixing that issue.

Comment 25

[Rick Reitmaier]

Attachment: align JIT and interp error handling

The change removes an explicit check for a constructor prior to calling ctor->construct().  It then relies on the C++ virtual override of ScriptObject to route the call correctly and raise an exception if needed.

Acceptance tests run fine with this change (N.B. the JIT follows this path).

The original change (subsequently moved to instr.cpp) was made here:
http://hg.mozilla.org/tamarin-redux/diff/e8f13d20cb96/core/Toplevel.cpp 

Asking Steven and Lars for sanity check on this change.

Comment 26

[Steven Johnson]

I'm a little nervous about this change, since we're removing a possible exception we used to throw (since day 1, pretty much). If the goal is to align interp and jit it would seem to make more sense to make interp stricter rather than the jit looser.

Comment 27

[Rick Reitmaier]

Actually what the patch does is have the interpreter match the logic of the jit.
The jit only checks for null and then calls ctor->construct().

The interpreter on the other hand does these 2 extra checks that are the target of removal in the patch.

That aside, yes we are changing behaviour but only as of Aug 19th 2009 when the exception was added.  See http://hg.mozilla.org/tamarin-redux/rev/e8f13d20cb96/

Comment 28

[Steven Johnson]

Comment on attachment 413225 [details] [diff] [review]
align JIT and interp error handling

Okay then.

Comment 29

[Lars T Hansen]

Comment on attachment 413225 [details] [diff] [review]
align JIT and interp error handling

Rubber stamp.

Comment 30

[Rick Reitmaier]

pushd http://hg.mozilla.org/tamarin-redux/rev/1661b1c508fd

NOTE: as3/Vector/nonindexproperty.as will still report different exceptions between interp and jit.  This is FOL.

Comment 31

[Rick Reitmaier]

Yikes, comment #27 is wrong!  Steven was correct in that the code existed for a long time.

Thus BOTH tests cases will be different for -Ojit;
  as3/RuntimeErrors/Errors1115NotAConstructor.as 
  es4/vector/nonindexproperty.as

Reverted the change http://hg.mozilla.org/tamarin-redux/rev/15d1edd59b73

Comment 32

[Rick Reitmaier]

(In reply to comment #31)
> Yikes, comment #27 is wrong!  Steven was correct in that the code existed for a
> long time.

Adding for clarity...the confusion was due to me basing my observations on changes in the wrong chunk of http://hg.mozilla.org/tamarin-redux/rev/e8f13d20cb96/

Comment 33

[Edwin Smith]

Comment on attachment 413225 [details] [diff] [review]
align JIT and interp error handling

This makes the JIT and interpreter agree when you do 

   new <non-null-non-constructor>

but not when you call

  new <null or undefed>

in the second case, you still get #1115 from the interpreter and #1007 from the jit.

Comment 34

[Edwin Smith]

jit and interpreter still aren't the same.

Comment 35

[Trevor Baker]

Status?

Comment 36

[Edwin Smith]

no change, bug is still valid and open.  I'm clearing the assigned field, Rick was leftover from last fall when he was the last one looking at the problem.

Comment 37

[Edwin Smith]

I just noticed that this test is *asserting* now, with -Ojit, which seems like a new and different failure mode, and bad.  (noticed on debug-debugger, x64, -Ojit).

Comment 38

[William Maddox]

Attachment: Work in progress -- consistently throw RangeError for negative but otherwise valid numeric properties

Here is some work toward resolving the inconsistency between the interpreter and the JIT with respect to the exception thrown for negative indices.  The objective is to consistently throw RangeError instead of ReferenceError in this case, wherever the reference occurs.

I've streamlined and commented getVectorIndex() a bit, including raising a number of issues related both to my understanding of the semantics of vectors and their implementation, and to cruft in the existing code.

Comment 39

[Edwin Smith]

Comment on attachment 494938 [details] [diff] [review]
Work in progress -- consistently throw RangeError for negative but otherwise valid numeric properties

So far so good.  I was surprised how big the new code is, but then noticed it deletes about the same amount of old code.

Vector semantic clarifications might need to get into the AS3 spec, so I'd make sure to involve Lars for any required clarifications.

Is the patch aligning with the current JIT behavior or interpreter behavior?  It wasn't clear from the comments or looking over the code quickly.

Comment 40

[William Maddox]

(In reply to comment #39)
> Comment on attachment 494938 [details] [diff] [review]
> Work in progress -- consistently throw RangeError for negative but otherwise
> valid numeric properties

> Is the patch aligning with the current JIT behavior or interpreter behavior? 
> It wasn't clear from the comments or looking over the code quickly.

Aligning the interpreter with the JIT.  The JIT gives the more specific and informative RangeError.  It might be easier to do it the other way around, but
that seemed like a step backward.

Comment 41

[William Maddox]

Attachment: Throw consistent exception when invoking a constructor that is not an object

Make constructproperty() throw kConstructOfNonFunctionError when the constructor is not an object, including when it is null or undefined.  This follows the behavior of op_construct().

Issues:

By inspection, it looks like there may be a previously undiscovered compiler/interpreter divergence when a constructor is early-bound in CodegenLIR::emitConstruct(), and its value is null.  An explicit emitCheckNull() is performed, which generates code that will throw a null pointer exception.  Consistency would then require a specialized null check here, or changing other locations to distinguish null from the other cases. I have not constructed an actual example to confirm this conjecture.

Having removed another instance of kNotConstructorError, it appears that the only remaining cases in which it may be thrown are in ClassClass::construct() and JObjectClass::create().  The latter appears to be part of a JNI bridge that is, as far as I know, unused and unmaintained.  ScriptObject::construct(), in contrast, throws kConstructOfNonFunctionError.  Both of these construct() methods are virtual, and look to be default implementations for extension points provided to the host application.  The motivation for this difference is not obvious, nor is it clear if it might be the source of any undiscovered interpreter/compiler divergences.

Comment 42

[Rick Reitmaier]

Comment on attachment 496441 [details] [diff] [review]
Throw consistent exception when invoking a constructor that is not an object

r+, assuming the line 246 'if (!ctor)' check is not redundant.

Comment 43

[William Maddox]

(In reply to comment #42)
> Comment on attachment 496441 [details] [diff] [review]
> Throw consistent exception when invoking a constructor that is not an object
> 
> r+, assuming the line 246 'if (!ctor)' check is not redundant.

This check is essential as, with the fix enabled, we wish not to throw any exception at all at this point if ctor is a valid non-null-or-undefined object.  We leave it to the call to the construct() method to throw any additional exceptions if the object may not be invoked as a constructor, paralleling the logic of op_construct().

Comment 44

[William Maddox]

Pushed to tr-spicy (in advance of SR):

http://hg.mozilla.org/users/stejohns_adobe.com/tr-spicy/rev/d5a4637294f5

Comment 45

[William Maddox]

Attachment: Consistently throw RangeError for negative but otherwise valid numeric properties, update tests

This corrects the second of two unrelated divergences between the compiler and interpreter with respect to exceptions thrown.  Here, we report RangeError when a negative Vector index is used in interpreted code, for compatibility with compiled code that throws this more informative error when specializing for a index known to be of integer type.  The failing test is now versioned.

Also, we remove all expected fails, etc. related to bug 456852 from the test configuration.

Comment 46

[William Maddox]

Below are some relevant references to the AS3 specification.
It does not appear to address negative array indices explicitly,
but my expectation as a programmer after reading these is that
a negative index would produce a RangeError.

http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/Vector.html

http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/RangeError.html

http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/ReferenceError.html

http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/operators.html#array_access

Comment 47

[Rick Reitmaier]

Comment on attachment 496741 [details] [diff] [review]
Consistently throw RangeError for negative but otherwise valid numeric properties, update tests

The core changes look fine to me, didn't look too carefully at the test changes though.

Comment 48

[William Maddox]

Pushed to tr-spicy (in advance of SR):

http://hg.mozilla.org/users/stejohns_adobe.com/tr-spicy/rev/482f99db2c72

Comment 49

[Lars T Hansen]

(In reply to comment #46)

> Below are some relevant references to the AS3 specification.

That's not the spec, but it's probably the most authoritative user-facing documentation we have, so it carries a lot of weight.  (For a discussion about specs, start an e-mail thread :-)

> It does not appear to address negative array indices explicitly,
> but my expectation as a programmer after reading these is that
> a negative index would produce a RangeError.

I agree.

Comment 50

[Lars T Hansen]

In response to some of the questions in the comments in the patch, Vectors /do not/ follow the ECMAScript rules laid down for Arrays and this is deliberate.  The point is that Vector provides error checking where Array does not.  Thus Vector must absorb all numeric property names on read and write and not allow them to pass through to the prototype object or the non-numeric dynamic property set; non-integer numeric index values are errors; out-of-range index values are errors, except that you can set the property at v.length if the vector is extensible.  However there are some subtleties; where ECMAScript equates v[3.14] and v["3.14"], Vector does not.

The spec, such as it is, is on asteam in the repository specs: specs/speclets/Vector.html.

Comment 51

[Edwin Smith]

Comment on attachment 496441 [details] [diff] [review]
Throw consistent exception when invoking a constructor that is not an object

instr.cpp:  with some more work, we could have BUILTIN_subclass to distinguish subclasses of Class, and BUILTIN_subfunction for subclasses of Function (other than MethodSignature).  that would enable sinking the call to core() onto slow branches.

CodegenLIR.cpp:4272 does the explicit emitCheckNull() mean that we still have an interpreter/jit difference, and thus this bug isn't finished?

R+ on the optimistic guess the answer is "no", but explanation needed.

Comment 52

[Edwin Smith]

Comment on attachment 496741 [details] [diff] [review]
Consistently throw RangeError for negative but otherwise valid numeric properties, update tests

I think the code is right, but odds are I'd never spot a bug looking at the code.  Do we have sufficient coverage of both Array and Vector indexing failure modes?  What about microbenchmarks for the hot paths that are touched by this bug?

SR+ for expediency, but please ensure the edges of Array & Vector are all covered.
 * Arrays with holes?
 * fractional numeric indexes
 * string index (fractional or not)
 * negatives, negative fractions, strings of such

I think one kind of vector is sufficient since it looks like they all share the same indexing code, but i'm open to counter arguments

Comment 53

[William Maddox]

Attachment: Test exceptions thrown by read/write of vector index properties that are out of range

(In reply to comment #52)
> Do we have sufficient coverage of both Array and Vector indexing failure
> modes?

The patch was a point fix for a bug as characterized by our existing regression tests.  Looking further, however, the test coverage is awful.  In the compiler, we specialize on Vector element type in a few places, and the existing tests for out-of-range indices did not consider extreme values or statically-typed index values.  The attachment exercises these cases, as well as noting conformance issues with the Vector spec.

Comment 54

[William Maddox]

Attachment: Results of vector range exception test on Spicy

Issues:

1) Except as noted in 3), in both the interpreter and the compiler, indices with non-zero fractional part yield ReferenceError when positive, and RangeError when negative.  The Vector spec says we should throw RangeError in both cases. Integer-valued indices, including doubles with a zero fractional part, throw RangeError when negative, and otherwise no error, which is correct behavior.

2) Strings that look like numbers, but which are not valid index properties, throw exceptions when used to index a Vector.  According to the Vector spec, strings are *always* valid (non-indexed) Vector properties, unlike the case for ECMAscript arrays.  It does not look like we have made any attempt to implement this behavior, and in fact go to some trouble to be more like Arrays.

3) Index range checks for (typed) Number-valued indices are completely botched.
Invalid indices consistently result in RangeError in the interpreter and ReferenceError in the compiler.






2) The Vector spec indicates that

Comment 55

[William Maddox]

Attachment: Results of vector range exception test on TR

Comment 56

[William Maddox]

Attachment: Consistently throw RangeError for invalid Vector index

This patch causes RangeError to be thrown for any attempt to write to a Vector property named by a number (or a numeric string), other than to a defined element, or one past the last such element.  Attempts to read an undefined numeric property likewise throw RangeError, and do not search the prototype chain. The patch affects present behavior in two ways:

1) RangeError is now thrown in cases where read or write to a property named by a number (or numeric string) previously threw a ReferenceError.  This in includes negative numbers and those with non-zero fractional parts.

2) In some cases (e.g., interpreted code), properties named by negative numbers were not recognized as index properties, so they could be inherited from from the prototype chain.  This has been corrected.

3) hasProperty recognizes negative numbers as index property names as well,
and does not search the prototype chain if the property is not defined locally.

Comment 57

[William Maddox]

Attachment: Test case showing expected behavior of previous patch (498943)

Comment 58

[William Maddox]

Attachment: Consistently throw ReferenceError for Vector indices that cannot be represented as uint32

This patch causes reads and writes of Vector properties to consistently throw ReferenceError in cases where a property named by a number (or numeric string) cannot be converted to a uint32.  If the value can be so converted, RangeError is thrown if the element is undefined.  (It is also permissible to write to the index just beyond the last defined element, as usual.)

This behavior is closer to what we already do, though it does not conform to the document that is closest to an official spec.  Furthermore, it takes more code to implement.  It is generally more efficient to convert a floating point number to a signed integer than an unsigned one.  Our code takes advantage of this in several places, and the loss of range of the converted result is not really an issue, because we hit allocator limitations long before.  The semantics we implement here, however, requires that we do the conversion more precisely, recognizing when the result will fit in a uint32 even if too large for int32.

Differences from existing behavior:

1) Negative index values that were in some cases resulting in RangeError previously will now throw ReferenceError.

2) Index values as large as 2^32-1 will not result in a ReferenceError, but will result in a RangeError if an illegal access is made (e.g., to an undefined element).  Previously, large values could result in either a ReferenceError or a RangeError, depending on interpretation/compilation/specialization.

3) The prototype chain will not be searched by either getProperty or hasProperty for a negative index.

Comment 59

[William Maddox]

Attachment: Test case showing expected behavior of previous patch (498945)

Comment 60

[Lars T Hansen]

Comment on attachment 494938 [details] [diff] [review]
Work in progress -- consistently throw RangeError for negative but otherwise valid numeric properties

I assume this has been superseded by later patches.

Comment 61

[William Maddox]

(In reply to comment #60)
> Comment on attachment 494938 [details] [diff] [review]
> Work in progress -- consistently throw RangeError for negative but otherwise
> valid numeric properties
> 
> I assume this has been superseded by later patches.

Yes.  Just minute before I wrote this, an email to you and a few other language-design folks was eaten by our lovely outlook webmail gateway...  To summarize very briefly, I've floated two alternative treatments of Vector index properties, one which hews more closely to the spec and seems a bit cleaner, and another that is closer to what we actually do at present.  I'd like to get a reading on what sounds best to the language folks and is least likely to need revision when a proper spec emerges.  I'm not looking to fix up all alleged conformance problems, just to be consistent with respect to the exceptions thrown, both for compiled/interpreted code and avoiding the unmotivated distinctions based on sign.  Furthermore, hasAtomProperty and getAtomProperty are not aligned in their treatment of negative indices, and this really needs to be fixed (I broke it recently).  Both patches, but especially 498945, could use a bit more polish -- I'm looking for a reading on the semantics we want, then I'll put one or the other out for review.  The test cases are probably the best way to see precisely what the differences are.

Comment 62

[William Maddox]

Attachment: Improved Vector index exceptions test case with results for Spicy and TR

Revised test case with useful command-line options and improved coverage of properties inherited from prototypes.

Includes test results for Spicy prior to my previous attempt at correcting the interpreter/compiler divergences (http://hg.mozilla.org/users/stejohns_adobe.com/tr-spicy/rev/482f99db2c72), Spicy current with that patch, and TR.

The test case has two wired-in models of rationalized exception behavior that can be compared with actual behavior, or one can simply produce a full dump of the actual behavior without analysis.

Comment 63

[William Maddox]

Attachment: Consistently throw RangeError for invalid Vector index (revised, with new testcase)

Consistently throw RangeError for Vector indices that are numeric but invalid.

This is a minor revision of patch 498943, and includes the improved exceptions test case of attachment 499224 [details] converted to run as an automated regresssion test.  Note that the current patch is against tr-spicy, which includes an earlier (erroneous) patch purporting to address the same issue, attachment 496741 [details] [diff] [review].

Comment 64

[Lars T Hansen]

Comment on attachment 501522 [details] [diff] [review]
Consistently throw RangeError for invalid Vector index (revised, with new testcase)

VectorClass.h:

In the .cpp file you're careful to use MathUtils::real2int but here you just use casts from double to int/uint.  I don't understand this.  Can you clarify?

I don't see how the new comments in _setNativeDoubleProperty and _setDoubleProperty are correct; they bring in "negative" but there's nothing in that branch of the code that has to do with the sign of the index value.  For example, -1 converts from double to int32_t just fine and would not hit this code.  Negative values are handled in subsequent code.

VectorClass.cpp:

isNegativeVectorIndexAtom is suboptimal.  The assertion tests that the argument is of one type or the other.  Then the actual code need only one type test, but both arms are guarded.  The second guard is redundant.

Not your bug, but allowing strings representing numbers with a decimal point as indices is a bug.

Not your bug, but if we allow strings representing numbers with a decimal point as indices then we should also allow leading decimal points.

New tests look fine.  I've not looked carefully at your rewritten tests though - what a mess that was.

R+ with all of that, but I hope you can clarify why it's OK to use double->int casts in some cases and utility conversion functions in other.  (IMO we should never use casts; they're not reliably portable in too many cases in my experience.)

Comment 65

[William Maddox]

(In reply to comment #64)
> Comment on attachment 501522 [details] [diff] [review]

> VectorClass.h:
> 
> In the .cpp file you're careful to use MathUtils::real2int but here you just
> use casts from double to int/uint.  I don't understand this.  Can you clarify?

I deliberately avoided code changes inessential to the specific behavioral changes the patch is intending to make.  This is purely risk avoidance, as it is rather late in the game to be making changes to Spicy.  Note that VectorClass has been rewritten in TR, so that removing these inconsistencies here has no offsetting longer-term benefit.  Believe me, I was sorely tempted to rewrite a lot of this stuff.

> I don't see how the new comments in _setNativeDoubleProperty and
> _setDoubleProperty are correct; they bring in "negative" but there's nothing in
> that branch of the code that has to do with the sign of the index value.  For
> example, -1 converts from double to int32_t just fine and would not hit this
> code.  Negative values are handled in subsequent code.

Good catch.  I cloned the comments from another case in which the double was converted to uint32_t.  Will fix.

> VectorClass.cpp:
> 
> isNegativeVectorIndexAtom is suboptimal.  The assertion tests that the argument
> is of one type or the other.  Then the actual code need only one type test, but
> both arms are guarded.  The second guard is redundant.

Indeed.

> Not your bug, but allowing strings representing numbers with a decimal point as
> indices is a bug.

It seems this was done deliberately, otherwise the conversion in getIndexFromAtom() should have been sufficient.  I'm not sure why we diverge from ES3 Array semantics in this respect, though.

> Not your bug, but if we allow strings representing numbers with a decimal point
> as indices then we should also allow leading decimal points.

It should be sufficient to change this line in getVectorIndex()
  if (s->length() > 0 && ((c >= '0' && c <= '9') || c == '-'))
to read as follows:
  if (s->length() > 0 && ((c >= '0' && c <= '9') || c = '.' || c == '-'))

If you think it is safe to make this as an unversioned change, I'll go ahead and fix it.

> R+ with all of that, but I hope you can clarify why it's OK to use double->int
> casts in some cases and utility conversion functions in other.  (IMO we should
> never use casts; they're not reliably portable in too many cases in my
> experience.)

I'm a bit concerned by this, particularly because it is obvious that someone took the time write the utility conversion function.  I had presumed it was because the asm implementation for x86 was possibly tighter than what the compiler generated.  You imply here that there is a correctness/portability issue involved.  The cast-and-compare idiom for recognizing and converting doubles representing integral values is widely used in Tamarin at present, using the utility function is an outlier from what I've seen. In fact, TR no longer uses real2int in getVectorIndex().  Note that rounding mode issues that might normally appear with the (double) cast do not affect this idiom.  What sort of problems have you observed, or can you fill me in on some background on real2int?

Comment 66

[Lars T Hansen]

(In reply to comment #65)
> 
> > Not your bug, but if we allow strings representing numbers with a decimal
> > point as indices then we should also allow leading decimal points.
> 
> It should be sufficient to change this line in getVectorIndex()
>   if (s->length() > 0 && ((c >= '0' && c <= '9') || c == '-'))
> to read as follows:
>   if (s->length() > 0 && ((c >= '0' && c <= '9') || c = '.' || c == '-'))
> 
> If you think it is safe to make this as an unversioned change, I'll go ahead
> and fix it.

I think it's safe, but I don't think you should fix it - code could depend on this kind of obscure behavior accidentally.  I think the right fix here is to leave it alone until the language spec comes along and mops it up.

> > R+ with all of that, but I hope you can clarify why it's OK to use
> > double->int casts in some cases and utility conversion functions in
> > other.  (IMO we should never use casts; they're not reliably portable
> > in too many cases in my experience.)
> 
> I'm a bit concerned by this, particularly because it is obvious that someone
> took the time write the utility conversion function.  ...  You imply here
> that there is a correctness/portability issue involved.  The cast-and-compare
> idiom for recognizing and converting
> doubles representing integral values is widely used in Tamarin at present,
> using the utility function is an outlier from what I've seen. In fact, TR no
> longer uses real2int in getVectorIndex().  Note that rounding mode issues
> that might normally appear with the (double) cast do not affect this idiom.
> What sort of problems have you observed, or can you fill me in on some
> background on real2int?

This is dated information, and possibly not relevant, but I know for a fact that some Math emulator libraries (Symbian) did not produce the same values as other platforms when casting to int, partly as a result of outright bugs -- in some cases, negative numbers were not supported IIRC.  Out-of-range values produced unusual error values.  In a previous life I ended up reengineering a JS engine to avoid floating-to-int casts everywhere for that reason.

BTW the C standard states (6.3.1.4 #1) that "If the value of the integral part cannot be represented by the integer type, the behavior is undefined," so no matter how you slice this we're on thin ice unless we've range checked the input.

Comment 67

[William Maddox]

Attachment: Consistently throw RangeError for invalid Vector index (with minor fixes)

This is essentially the same as the previous version R+'d by Lars, with corrected comments and with isNegativeVectorIndexAtom() streamlined slightly per Lars' comments.  I also strengthened the assertion in isNegativeVectorIndexAtom() to make sure that a string atom is non-empty, though this would have been assured by the previous logic when isNegativeVectorIndexAtom() was used as intended.

This is effectively a request for SR.

Comment 68

[William Maddox]

Attachment: Consistently throw RangeError for invalid Vector index (with minor fixes)

Post correct version of patch.

Comment 69

[William Maddox]

Pushed to tr-spicy:

http://hg.mozilla.org/users/stejohns_adobe.com/tr-spicy/rev/c3d6e7839a98

This incorporates minor changes in response to feedback along with Lars' R+. I'd still appreciate it if Rick and Edwin could take a look at attachment 502988 [details] [diff] [review] before I commit this to TR.

Comment 70

[Rick Reitmaier]

Comment on attachment 502988 [details] [diff] [review]
Consistently throw RangeError for invalid Vector index (with minor fixes)

I'm having trouble reviewing this patch as it doesn't apply to the latest tamarin.

Comment 71

[William Maddox]

(In reply to comment #70)
> Comment on attachment 502988 [details] [diff] [review]
> Consistently throw RangeError for invalid Vector index (with minor fixes)
> 
> I'm having trouble reviewing this patch as it doesn't apply to the latest
> tamarin.

It's a Spicy (er, Spicier) patch only.  The Vector code has changed substantially in TR, which will require a new patch, not just a rebasing, so that will be another patch, to be separately reviewed.

The patch applies cleanly to tr-spicy.

Comment 72

[Rick Reitmaier]

Comment on attachment 502988 [details] [diff] [review]
Consistently throw RangeError for invalid Vector index (with minor fixes)

Found a spicy repo to apply patch against...

r+ with nits for TR version; fine for spicy... in the header there looks to be 4 cases where we could consolidate identical(?) error handling into a single function.  Ditto for the cpp file.

Also, could we assert / comment that it is legal to fall-through the if (isNumber) statement in getAtomProperty().  The other places where we throw one or the other type of error do not allow fall-through and so this one seems odd and its not immediately clear whether its a bug or not.

Comment 73

[Tamarin Bot]

changeset: 5826:c72c9b1c20a9
user:      William Maddox <[e-mail]>
summary:   Bug 456852 - Different runtime error for invalid constructor call when -Ojit set (r=rreitmai)

This fixes one of two unrelated interpreter/compiler divergences reported by this bug,
affecting acceptance test as3/RuntimeErrors/Errors1115NotAConstructor.as .

http://hg.mozilla.org/tamarin-redux/rev/c72c9b1c20a9

Comment 74

[William Maddox]

This bug needs to remain open until a patch for the Vector index exception issue is committed to TR in the Serrano timeframe.  As Wasabi will be derived from the Spicy branch, we should be set for that release. See comment 69 and comment 71.  Retargetting for Serrano.

Comment 75

[Edwin Smith]

Comment on attachment 502988 [details] [diff] [review]
Consistently throw RangeError for invalid Vector index (with minor fixes)

nit

the new comment at VectorClass.cpp:183, and 677:

> Call below will throw RangeError if index is too large.

is referring to version checked logic that acutally either throws RangeError or ReferenceError depending on the version check.

Comment 76

[William Maddox]

Attachment: Vector exception fixes and indexing optimizations for TR -- work in progress

Comment 77

[William Maddox]

Attachment: Vector exception fixes and indexing optimizations for TR

Comment 78

[William Maddox]

Attachment: Clean up bugCompatibility -- follow on to previous patch

Bug 456852 addresses two independent issues, and accumulated quite a bit of history before versioning was raised as an issue.  This patch introduces a convention for dealing with this.  An alternative would be to introduce two new placeholder bugs to stand for the separate issues.  This would be an artifice for the sake of bugCompatibility, however, as the content has all been dumped here in this bug.  The title of this bug is not particularly descriptive of either issue.
Grumble.

Comment 79

[William Maddox]

Attachment: Vector exception fixes and indexing optimizations for TR

Updated patch restores accidentally-deleted EMACS mode line.

Comment 80

[William Maddox]

Attachment: Vector exception acceptance test

This is the same test committed to tr-spicy.

Comment 81

[William Maddox]

Attachment: Vector exception behavior as observed on recent releases for SWF10 and SWF11

These have been manually compared for expected behavior, and should be used as the basis of a test case.

Comment 82

[Rick Reitmaier]

Comment on attachment 524257 [details] [diff] [review]
Vector exception fixes and indexing optimizations for TR

r+ but could you add comments regarding why you do int followed by uint conversion in setpropertylate_d. 

CodegenLIR.optimizeIndexArgumentType 
  Also any reason why you promoted the uint32_t to a double (rather than the other way around) in the sequence.  If so, please comment.

Comment 83

[Tamarin Bot]

changeset: 6175:3c8d01c7b51a
user:      William Maddox <[e-mail]>
summary:   Bug 456852 - Vector exception fixes and indexing optimizations (r=rreitmai)

http://hg.mozilla.org/tamarin-redux/rev/3c8d01c7b51a

Comment 84

[Tamarin Bot]

changeset: 6301:82eb08710621
user:      Brent Baker <[e-mail]>
summary:   Bug 456852: skip vector testcase nonidexproperty.as when running diff testing. (r=brbaker)

http://hg.mozilla.org/tamarin-redux/rev/82eb08710621