Closed
Bug 559082
Opened 14 years ago
Closed 14 years ago
ByteArray set[] property failure at 2^23
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
flash10.2.x-Spicy
People
(Reporter: pnkfelix, Assigned: pnkfelix)
References
Details
Attachments
(5 files, 2 obsolete files)
1008 bytes,
text/plain
|
Details | |
624 bytes,
text/plain
|
Details | |
2.94 KB,
patch
|
edwsmith
:
feedback+
|
Details | Diff | Splinter Review |
1.18 KB,
patch
|
edwsmith
:
review+
stejohns
:
superreview+
|
Details | Diff | Splinter Review |
2.11 KB,
patch
|
pnkfelix
:
review+
stejohns
:
superreview+
|
Details | Diff | Splinter Review |
An attempt to set a property at 2^23 can fail with ByteArrays on 64-bit builds. Oddly, this problem does not uniformly manifest itself; ie lifting the code so that it occurs within a function definition allows the property set to succeed. The attached file illustrates the problem; here is the output I get from a build configured with flags: % ../configure.py --enable-shell --enable-debug --enable-debugger --target=x86_64-darwin hello attempt 1: b[2^22] = 22 attempt 2: b[2^22] = 22 attempt 1: b[2^23-1] = 23 attempt 2: b[2^23-1] = 23 attempt 1: b[2^23] = 23 atempt 1b: b[2^23] = 23 attempt 2: b[2^23] = 23 ReferenceError: Error #1056: Cannot create property 8388608 on flash.utils.ByteArray. at global$init() On a 32-bit build, I instead see: hello attempt 1: b[2^22] = 22 attempt 2: b[2^22] = 22 attempt 1: b[2^23-1] = 23 attempt 2: b[2^23-1] = 23 attempt 1: b[2^23] = 23 atempt 1b: b[2^23] = 23 attempt 2: b[2^23] = 23 attempt 1: b[2^24] = 24 attempt 2: b[2^24] = 24 goodbye
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → fklockii
Assignee | ||
Comment 1•14 years ago
|
||
So far it seems like the 64-bit build is going down a control flow path where it is interpreting the 2^23 property as a dynamic instance variable that needs to be added to the ByteArray. ByteArrays are sealed so this is disallowed by the runtime. But why is 2^23 considered an instance variable for a 64-bit build but not for a 32-bit build?
Assignee | ||
Comment 2•14 years ago
|
||
The control flow at Interpreter.cpp:2066 differs for this input between 32-bit and 64-bit builds. It seems like the 32-bit build is creating an atom of kIntptrType while the 64-bit build is creating an atom of kDoubleType. I am surprised by this, but maybe I do not yet grok what a 64-bit Intptr means in AVM.
Assignee | ||
Comment 3•14 years ago
|
||
(In reply to comment #2) > The control flow at Interpreter.cpp:2066 differs for this input between 32-bit > and 64-bit builds. The real shame here is that if control actually reached the end of this series of if-tests, it seems like the default case of just invoking the setAtomProperty method of the script object would just work. But the next if-test, at Interpreter.cpp:2072 decides to use Toplevel::setproperty. It decides this because AvmCore::isDictionaryLookup returns false for the input (a ByteArray and a kDoubleType). So Toplevel::setproperty then throws the exception. I am going to need to acquaint myself better with the semantics of Actionscript binding before I can resolve this oddness.
Assignee | ||
Comment 4•14 years ago
|
||
(There's also still the question of why this code worked in response to such small changes such as the lifting illustrated in the test case. That may bear investigation; are we getting doubles in some cases and intptrs in others?)
Assignee | ||
Comment 5•14 years ago
|
||
FYI: Ed asked whether this is specific to 64-bit interpreter only. The notes above are with the default hybrid mode. With -Ojit, the bug goes away. With -Dinterp, it continues to arise. So, as one might infer from the notes above, the issue is a problem in the interpreter alone.
Comment 6•14 years ago
|
||
the visible effect of OP_setproperty should not change, whether the index is kIntegerType, or kDoubleType of the same value. 'course the kDoubleType logic can be a slow path, but it should not have different side effects. this could be a subtle interpreter vs jit difference, as well as 32-bit vs 64-bit. and, an open question is why would the 64bit code have a kDoubleType atom in this case. (could be a latent performance bug)
Comment 7•14 years ago
|
||
Based on what we know at this point, this can be targeted to 10.1.1... fix soon, but not argo.
Comment 8•14 years ago
|
||
I should state an implicit assumption: 1. A 32bit int or uint value, on a 32-bit machine, could end up being kIntegerType or kDoubleType, depending on its value. on a 64-bit machine, all int32/uint32 values fit in kIntegerType atoms. 2. most of our code handling atoms is not specialized for 32/64bit. 3. therefore, any optimization for kIntegerType values that doesn't have a specific value range limit, and isn't specialized for 32 vs 64bit, must have a slow path for kDoubleType that has identical behavior. sketch of a test methodology (out of scope for this bug, but writing it down anyway): 1. make intToAtom() and uintToAtom always generate kDoubleType atoms 2. see what happens. nothing should change, other than speed/memory behavior. its possible we'll find pieces of code that require small integers to become kIntegerType atoms. however, 2^29 is a somewhat arbitrary limit, and code like that could be fragile and buggy.
Assignee | ||
Comment 9•14 years ago
|
||
(In reply to comment #5) > FYI: Ed asked whether this is specific to 64-bit interpreter only. The notes > above are with the default hybrid mode. With -Ojit, the bug goes away. With > -Dinterp, it continues to arise. So, as one might infer from the notes above, > the issue is a problem in the interpreter alone. Ah, one subtlety: not only does the bug continue to arise with -Dinterp, but also it comes up more often. That is, if you turn off the JIT entirely, then the very first attempt to set b[2^23] in the test case fails. So that may provide a hint as to why small changes to the code caused the test to succeed.
Assignee | ||
Comment 10•14 years ago
|
||
And now I've come full circle. The whole reason I started looking at this was to try to find a case that would break the logic in AvmCore::getIndexFromAtom; well, my "most obvious fix" breaks it. But that might not be good enough.
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #8) > I should state an implicit assumption: Am I right in interpreting your statement as saying that we should /not/ be assuming that atoms that are kDoubleType can never be indexes? The code as it stands right now seems to attempt to maintain an invariant that if a double can be represented as an int atom, it will be. E.g. the variants of AvmCore::doubleToAtom all end with "if can be int atom, then make it so." I am trying to figure out if this is an invariant that we all rely on (legitimately), or an invariant that is merely "nice when it happens to work out."
Assignee | ||
Comment 12•14 years ago
|
||
AvmCore::doubleToAtom definitely behaves differently for 32- versus 64-bit builds, for n = 8388608 32bit: returns 0x4000006 64bit: returns 0x101026fc7 So that's something.
Comment 13•14 years ago
|
||
indeed, it looks like CAN_BE_INT_ATOM is way wrong: * <<8>>8 intends to check to see if the value is representable with (64-8 = 56) bits, but thats too much precision - should be 53. * the intval passed in is int32_t, so <<8>>8 wipes out bits 24:31, so the check fails for reasonably big values that are still representable. in fact, with a 32bit value, the test should be (intval == n) && (/* etc */). 1. we ought to fix it and test it, BUT 2. it will hide the other bug you stumbled across, that the behavior of OP_setproperty behave the same for kIntegerType and kDoubleType atoms representing the same value. there are legal property indices that dont fit kIntegerType on 32bit cpu's, but do fit on 64bit cpu's.
Comment 14•14 years ago
|
||
I think the <<8 >>8 was fixed recently in redux. (At least one of them was fixed.) changeset: 4367:07fe671debf0 user: Lars T Hansen <lhansen@adobe.com> date: Fri Apr 09 10:56:40 2010 -0700 summary: Further fix for 540580 - atomMaxIntValue and atomMinIntValue are apparently incorrect (and the latter unused): account for symmetric sign+magnitude change (r=lhansen, shrugging_acquiesence=stejohns) changeset: 4362:87ad0ab7b931 user: Lars T Hansen <lhansen@adobe.com> date: Thu Apr 08 15:25:41 2010 -0700 summary: Fix 540580 - atomMaxIntValue and atomMinIntValue are apparently incorrect (and the latter unused) (r=stejohns)
Comment 15•14 years ago
|
||
(In reply to comment #13) > indeed, it looks like CAN_BE_INT_ATOM is way wrong: wow what timing, i was just about to post this. I assume the (<< 8) >> 8 is assuming that typeof(n) is int64_t. but from what I can see, in all occurrences of CAN_BE_INT_ATOM its type is int32_t.
Comment 16•14 years ago
|
||
what we need is a fix similar to this one: http://hg.mozilla.org/tamarin-redux/rev/4362 bonus points for nuking the macro and using atomIsValidIntptrValue().
Flags: flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1.1
Comment 17•14 years ago
|
||
(In reply to comment #16) > what we need is a fix similar to this one: > > http://hg.mozilla.org/tamarin-redux/rev/4362 distressingly, that change doesn't appear to be in tr-argo. does it need to be?
Comment 18•14 years ago
|
||
Fixes the obvious flaw noted in comments above (and some other related cleanup), but sadly, doesn't fix the underlying bug: Felix's testcase still fails.
Attachment #439324 -
Flags: feedback?(edwsmith)
Assignee | ||
Comment 19•14 years ago
|
||
Math.pow seems to be a possible source of problems.
Assignee | ||
Comment 20•14 years ago
|
||
(In reply to comment #19) > Created an attachment (id=439344) [details] > test specific issues with Math.pow and bytearray indices > > Math.pow seems to be a possible source of problems. Forgot to mention: on a 64-bit build running with -Dinterp, you get the following result from the test: bash-3.2$ ./shell/avmshell -Dinterp ../../interp64_busted.abc b[8388608] : 1 idx1: 8388608 before doing b[idx1]=2 ReferenceError: Error #1056: Cannot create property 8388608 on flash.utils.ByteArray. at global$init()[../../interp64_busted.as:28]
Updated•14 years ago
|
Attachment #439344 -
Attachment mime type: application/applefile → text/plain
Assignee | ||
Comment 21•14 years ago
|
||
Comment on attachment 439324 [details] [diff] [review] patch for CAN_BE_INT One problem: this is missing the necessary changes to the type declarations. The Left and Right shift dances are throwing away information because you are applying them to 32 bit quantities (int32_t 's)
Assignee | ||
Comment 22•14 years ago
|
||
Adding a cast to (intptr_t) in front of the first IVAL in CAN_BE_INT_ATOM makes my test go through. But maybe it would be better to make a generalized version of atomIsValidIntPtrValue that takes the original double as an additional argument? Do these things really need to be macros here?
Comment 23•14 years ago
|
||
Good observation and fix. Don't need to be macros, but not sure if this should live at the same level as atomIsValidIntPtrValue -- comparing the value to a float is an unusual thing that normal code shouldn't ever want or need to do.
Assignee | ||
Comment 24•14 years ago
|
||
This is the missing cast operation that I noticed. Otherwise I've left Steven's work alone. I hope to make a macro-less refactoring (much like ed suggested), but I don't see a reason to hold off on this fix for that.
Attachment #439324 -
Attachment is obsolete: true
Attachment #439359 -
Flags: feedback?(edwsmith)
Attachment #439324 -
Flags: feedback?(edwsmith)
Assignee | ||
Updated•14 years ago
|
Comment 25•14 years ago
|
||
(In reply to comment #13) > 2. it will hide the other bug you stumbled across, that the behavior of > OP_setproperty behave the same for kIntegerType and kDoubleType atoms > representing the same value. there are legal property indices that dont fit > kIntegerType on 32bit cpu's, but do fit on 64bit cpu's. This note from Ed makes me suspect that the CAN_BE_INT patch still is not going to work in general. We've been having some private chats about the matter; I will try to come up with some inputs that expose this problem Friday morning.
Comment 26•14 years ago
|
||
Comment on attachment 439359 [details] [diff] [review] patch for CAN_BE_INT For AvmCore::uintToAtom() and intToAtom(), a reasonable C++ compiler should inline and specialize the 32bit case to the point that its equivalent to the 64-bit case, on a 64-bit cpu. i.e. we could eliminate the ifdef, and add a comment to preserve their value as documentation: return atomIsValidIntptrValue_u(n) ? // always true on 64-bit atomFromIntptrValue_u(n) : allocDouble(n); of course we should try it first. if either compiler isn't successful with the inlining, then the code should be left as in the patch. if you're investigating inlining, consider moving intToAtom and uintToAtom to AvmCore-inlines.h and marking them REALLY_INLINE, while leaving allocDouble out-of-line. the fast path doesn't require allocation or use of the "this" (AvmCore*) parameter, so it should specialize well at call sites.
Updated•14 years ago
|
Attachment #439359 -
Flags: feedback?(edwsmith) → feedback+
Assignee | ||
Comment 27•14 years ago
|
||
I inspected the generated machine code at the invocation site within 'avmplus::AvmCore::doubleToAtom_sse2'; its the same before and after this patch, modulo register allocation differences that led to the introduction of a memory-to-reg move instruction. I think the improved readability and usability (e.g. when stepping in a debugger) make this change worth it. One oddity is what to name this function; it acts similarly to atomIsValidIntptr so I gave it a self-documenting variant on that name, but its defined in the AvmCore namespace ... is that violating any team rules?
Attachment #439603 -
Flags: review?(edwsmith)
Assignee | ||
Comment 28•14 years ago
|
||
I verified that generated instructions (before and after patch) are identical for 64-bit Mac.
Attachment #439604 -
Flags: review?(edwsmith)
Assignee | ||
Comment 29•14 years ago
|
||
Comment on attachment 439603 [details] [diff] [review] removes CAN_BE_INT; puts in inline function instead. whoops I forgot to delete CAN_BE_INT as part of the patch. Will do that now.
Attachment #439603 -
Flags: review?(edwsmith)
Comment 30•14 years ago
|
||
Comment on attachment 439604 [details] [diff] [review] remove unnecessary conditionals from atomFromIntptrValue[_u] Looks fine, and the consistent naming is fine too.
Attachment #439604 -
Flags: review?(edwsmith) → review+
Assignee | ||
Comment 31•14 years ago
|
||
Attachment #439603 -
Attachment is obsolete: true
Attachment #439605 -
Flags: review?(edwsmith)
Assignee | ||
Comment 32•14 years ago
|
||
Comment on attachment 439605 [details] [diff] [review] removes CAN_BE_INT; puts in inline function instead. (push me please)
Attachment #439605 -
Flags: superreview?(stejohns)
Attachment #439605 -
Flags: review?(edwsmith)
Attachment #439605 -
Flags: review+
Assignee | ||
Comment 33•14 years ago
|
||
Comment on attachment 439604 [details] [diff] [review] remove unnecessary conditionals from atomFromIntptrValue[_u] (push me please)
Attachment #439604 -
Flags: superreview?(stejohns)
Comment 34•14 years ago
|
||
Comment on attachment 439604 [details] [diff] [review] remove unnecessary conditionals from atomFromIntptrValue[_u] Approved, though it's possible that some of the more anal-retentive C++ compilers will complain about "condition always true" -- guess we'll find out.
Attachment #439604 -
Flags: superreview?(stejohns) → superreview+
Updated•14 years ago
|
Attachment #439605 -
Flags: superreview?(stejohns) → superreview+
Comment 35•14 years ago
|
||
pushed patches as http://hg.mozilla.org/tamarin-redux/rev/7ca2f40c4c11 Tests now seem to run cleanly on 64-bit (interp and jit) -- shall we mark it fixed?
Assignee | ||
Comment 36•14 years ago
|
||
long overdue mark as fixed
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•