Closed
Bug 456852
Opened 16 years ago
Closed 14 years ago
Different runtime errors when -Ojit set in acceptance test run
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Q3 11 - Serrano
People
(Reporter: dschaffe, Assigned: wmaddox)
References
Details
(Whiteboard: fixed-in-spicier)
Attachments
(13 files, 10 obsolete files)
(deleted),
patch
|
edwsmith
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rreitmai
:
review+
edwsmith
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rreitmai
:
review+
edwsmith
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
application/x-tar
|
Details | |
(deleted),
application/x-tar
|
Details | |
(deleted),
application/octet-stream
|
Details | |
(deleted),
patch
|
lhansen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rreitmai
:
review+
edwsmith
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rreitmai
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rreitmai
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rreitmai
:
review+
|
Details | Diff | Splinter Review |
(deleted),
application/x-gzip
|
Details |
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
Flags: wanted-flashplayer10+
Flags: in-testsuite+
Flags: flashplayer-triage+
Comment 1•16 years ago
|
||
still there after redux merge (using -Ojit)
Comment 2•16 years ago
|
||
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.
Reporter | ||
Updated•16 years ago
|
Flags: flashplayer-qrb?
Rick, any reason to not make the generated error consistent rather than changing the tests?
Comment 4•16 years ago
|
||
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.
Reporter | ||
Comment 5•16 years ago
|
||
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?
Updated•16 years ago
|
Assignee: nobody → rreitmai
Status: NEW → ASSIGNED
Comment 6•16 years ago
|
||
change to allow redux builds to support 'mixed' (interp+jit) modes once again.
Attachment #343584 -
Flags: review?(edwsmith)
Comment 7•16 years ago
|
||
prior patch was incorrect file
Attachment #343584 -
Attachment is obsolete: true
Attachment #343584 -
Flags: review?(edwsmith)
Updated•16 years ago
|
Attachment #343585 -
Flags: review?(edwsmith)
Comment 8•16 years ago
|
||
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()?
Updated•16 years ago
|
Attachment #343585 -
Flags: review?(edwsmith) → review+
Comment 9•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
pushed as changeset #1015.
Comment 12•16 years ago
|
||
last comment should have stated into the tamarin-redux build.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 13•16 years ago
|
||
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.
Status: RESOLVED → REOPENED
Flags: wanted-flashplayer10+ → wanted-flashplayer10-
Resolution: FIXED → ---
Comment 14•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
on x86, the ExceptionFrame struct is only 96 bytes.
Comment 17•16 years ago
|
||
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•16 years ago
|
||
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
Reporter | ||
Comment 19•16 years ago
|
||
the two tests as3/RuntimeErrors/Errors1115NotAConstructor.as
es4/vector/nonindexproperty.as started to pass in -Ojit in tamarin-redux 1724.
Comment 20•16 years ago
|
||
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.
Reporter | ||
Comment 21•16 years ago
|
||
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•15 years ago
|
||
status check?
Updated•15 years ago
|
Flags: flashplayer-triage+ → flashplayer-triage?
Comment 23•15 years ago
|
||
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
Updated•15 years ago
|
Flags: flashplayer-triage? → flashplayer-triage+
Updated•15 years ago
|
Priority: -- → P2
Target Milestone: --- → flash10.1
Comment 24•15 years ago
|
||
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•15 years ago
|
||
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.
Attachment #413225 -
Flags: superreview?(lhansen)
Attachment #413225 -
Flags: review?(stejohns)
Comment 26•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
Comment on attachment 413225 [details] [diff] [review]
align JIT and interp error handling
Okay then.
Attachment #413225 -
Flags: review?(stejohns) → review+
Comment 29•15 years ago
|
||
Comment on attachment 413225 [details] [diff] [review]
align JIT and interp error handling
Rubber stamp.
Attachment #413225 -
Flags: superreview?(lhansen) → superreview+
Comment 30•15 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
Comment 31•15 years ago
|
||
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•15 years ago
|
||
(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/
Updated•15 years ago
|
Status: RESOLVED → VERIFIED
Comment 33•15 years ago
|
||
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.
Updated•14 years ago
|
Blocks: interp_jit_semantics
Comment 34•14 years ago
|
||
jit and interpreter still aren't the same.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 35•14 years ago
|
||
Status?
Comment 36•14 years ago
|
||
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.
Assignee: rreitmai → nobody
Updated•14 years ago
|
Flags: flashplayer-qrb+ → flashplayer-qrb?
Comment 37•14 years ago
|
||
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).
Updated•14 years ago
|
Target Milestone: flash10.x - Serrano → flash10.2.x-Spicy
Assignee | ||
Updated•14 years ago
|
Summary: different runtime errors when -Dforcemir set in acceptance test run → Different runtime errors when -Ojit set in acceptance test run
Assignee | ||
Comment 38•14 years ago
|
||
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.
Attachment #494938 -
Flags: feedback?(rreitmai)
Assignee | ||
Updated•14 years ago
|
Attachment #494938 -
Flags: feedback?(edwsmith)
Comment 39•14 years ago
|
||
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.
Attachment #494938 -
Flags: feedback?(edwsmith) → feedback+
Assignee | ||
Comment 40•14 years ago
|
||
(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.
Updated•14 years ago
|
Attachment #494938 -
Flags: feedback?(rreitmai) → feedback+
Assignee | ||
Updated•14 years ago
|
Attachment #494938 -
Flags: feedback?(lhansen)
Assignee | ||
Comment 41•14 years ago
|
||
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.
Attachment #413225 -
Attachment is obsolete: true
Attachment #496441 -
Flags: superreview?(edwsmith)
Attachment #496441 -
Flags: review?(rreitmai)
Comment 42•14 years ago
|
||
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.
Attachment #496441 -
Flags: review?(rreitmai) → review+
Assignee | ||
Comment 43•14 years ago
|
||
(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().
Assignee | ||
Comment 44•14 years ago
|
||
Pushed to tr-spicy (in advance of SR):
http://hg.mozilla.org/users/stejohns_adobe.com/tr-spicy/rev/d5a4637294f5
Assignee | ||
Comment 45•14 years ago
|
||
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.
Attachment #496741 -
Flags: superreview?(edwsmith)
Attachment #496741 -
Flags: review?(rreitmai)
Assignee | ||
Comment 46•14 years ago
|
||
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•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #496741 -
Flags: review?(rreitmai) → review+
Assignee | ||
Comment 48•14 years ago
|
||
Pushed to tr-spicy (in advance of SR):
http://hg.mozilla.org/users/stejohns_adobe.com/tr-spicy/rev/482f99db2c72
Comment 49•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
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.
Attachment #496441 -
Flags: superreview?(edwsmith) → superreview+
Comment 52•14 years ago
|
||
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
Attachment #496741 -
Flags: superreview?(edwsmith) → superreview+
Assignee | ||
Comment 53•14 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Attachment #498016 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 54•14 years ago
|
||
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
Assignee | ||
Comment 55•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #498167 -
Attachment mime type: text/plain → application/x-tar
Assignee | ||
Comment 56•14 years ago
|
||
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.
Assignee | ||
Comment 57•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #498944 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 58•14 years ago
|
||
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.
Assignee | ||
Comment 59•14 years ago
|
||
Comment 60•14 years ago
|
||
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.
Attachment #494938 -
Flags: feedback?(lhansen)
Assignee | ||
Comment 61•14 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Attachment #494938 -
Attachment is obsolete: true
Assignee | ||
Comment 62•14 years ago
|
||
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.
Assignee | ||
Comment 63•14 years ago
|
||
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].
Attachment #498943 -
Attachment is obsolete: true
Attachment #498944 -
Attachment is obsolete: true
Attachment #498945 -
Attachment is obsolete: true
Attachment #498946 -
Attachment is obsolete: true
Attachment #501522 -
Flags: superreview?(edwsmith)
Attachment #501522 -
Flags: review?(lhansen)
Comment 64•14 years ago
|
||
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.)
Attachment #501522 -
Flags: review?(lhansen) → review+
Assignee | ||
Comment 65•14 years ago
|
||
(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•14 years ago
|
||
(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.
Assignee | ||
Comment 67•14 years ago
|
||
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.
Attachment #502986 -
Flags: review?(edwsmith)
Assignee | ||
Comment 68•14 years ago
|
||
Post correct version of patch.
Attachment #502986 -
Attachment is obsolete: true
Attachment #502988 -
Flags: review?(edwsmith)
Attachment #502986 -
Flags: review?(edwsmith)
Assignee | ||
Updated•14 years ago
|
Attachment #502988 -
Attachment is patch: true
Updated•14 years ago
|
Attachment #501522 -
Flags: superreview?(edwsmith)
Assignee | ||
Updated•14 years ago
|
Attachment #502988 -
Flags: superreview?(edwsmith)
Attachment #502988 -
Flags: review?(rreitmai)
Attachment #502988 -
Flags: review?(edwsmith)
Attachment #502988 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #502988 -
Flags: review?
Assignee | ||
Comment 69•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Whiteboard: fixed-in-spicier
Comment 70•14 years ago
|
||
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.
Assignee | ||
Comment 71•14 years ago
|
||
(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•14 years ago
|
||
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.
Attachment #502988 -
Flags: review?(rreitmai) → review+
Updated•14 years ago
|
Target Milestone: flash10.2.x-Spicy → flash10.x-Wasabi
Comment 73•14 years ago
|
||
changeset: 5826:c72c9b1c20a9
user: William Maddox <wmaddox@adobe.com>
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
Assignee | ||
Comment 74•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Target Milestone: flash10.x-Wasabi → flash10.x-Serrano
Updated•14 years ago
|
Whiteboard: fixed-in-spicier → fixed-in-spicier must –fix-candidate
Updated•14 years ago
|
Whiteboard: fixed-in-spicier must –fix-candidate → fixed-in-spicier must–fix-candidate
Updated•14 years ago
|
Whiteboard: fixed-in-spicier must–fix-candidate → fixed-in-spicier must-fix-candidate
Comment 75•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #502988 -
Flags: superreview?(edwsmith) → superreview+
Depends on: Andre
Flags: wanted-flashplayer10-
Flags: flashplayer-qrb?
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Assignee | ||
Comment 76•14 years ago
|
||
Assignee | ||
Comment 77•14 years ago
|
||
Attachment #524234 -
Flags: review?(rreitmai)
Assignee | ||
Updated•14 years ago
|
Attachment #521110 -
Attachment is obsolete: true
Assignee | ||
Comment 78•14 years ago
|
||
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.
Attachment #524235 -
Flags: review?(rreitmai)
Assignee | ||
Comment 79•14 years ago
|
||
Updated patch restores accidentally-deleted EMACS mode line.
Attachment #524234 -
Attachment is obsolete: true
Attachment #524257 -
Flags: review?(rreitmai)
Attachment #524234 -
Flags: review?(rreitmai)
Assignee | ||
Comment 80•14 years ago
|
||
This is the same test committed to tr-spicy.
Attachment #524437 -
Flags: review?(rreitmai)
Updated•14 years ago
|
Attachment #524235 -
Flags: review?(rreitmai) → review+
Updated•14 years ago
|
Attachment #524437 -
Flags: review?(rreitmai) → review+
Assignee | ||
Comment 81•14 years ago
|
||
These have been manually compared for expected behavior, and should be used as the basis of a test case.
Comment 82•14 years ago
|
||
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.
Attachment #524257 -
Flags: review?(rreitmai) → review+
Comment 83•14 years ago
|
||
changeset: 6175:3c8d01c7b51a
user: William Maddox <wmaddox@adobe.com>
summary: Bug 456852 - Vector exception fixes and indexing optimizations (r=rreitmai)
http://hg.mozilla.org/tamarin-redux/rev/3c8d01c7b51a
Assignee | ||
Updated•14 years ago
|
Status: REOPENED → RESOLVED
Closed: 15 years ago → 14 years ago
Resolution: --- → FIXED
Comment 84•13 years ago
|
||
changeset: 6301:82eb08710621
user: Brent Baker <brbaker@adobe.com>
summary: Bug 456852: skip vector testcase nonidexproperty.as when running diff testing. (r=brbaker)
http://hg.mozilla.org/tamarin-redux/rev/82eb08710621
You need to log in
before you can comment on or make changes to this bug.
Description
•