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)

x86_64
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
flash10.2.x-Spicy

People

(Reporter: pnkfelix, Assigned: pnkfelix)

References

Details

Attachments

(5 files, 2 obsolete files)

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: nobody → fklockii
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?
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.
(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.
(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?)
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.
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)
Based on what we know at this point, this can be targeted to 10.1.1... fix soon, but not argo.
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.
(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.
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.
(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."
AvmCore::doubleToAtom definitely behaves differently for 32- versus 64-bit builds, for n = 8388608

32bit: returns  0x4000006

64bit: returns  0x101026fc7

So that's something.
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.
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)
(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.
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().
Depends on: 559321
Flags: flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1.1
Status: NEW → ASSIGNED
(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?
Attached patch patch for CAN_BE_INT (obsolete) — Splinter Review
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)
Math.pow seems to be a possible source of problems.
(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]
Attachment #439344 - Attachment mime type: application/applefile → text/plain
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)
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?
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.
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)
Blocks: 559321
No longer depends on: 559321
(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 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.
Attachment #439359 - Flags: feedback?(edwsmith) → feedback+
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)
I verified that generated instructions (before and after patch) are identical for 64-bit Mac.
Attachment #439604 - Flags: review?(edwsmith)
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 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+
Attachment #439603 - Attachment is obsolete: true
Attachment #439605 - Flags: review?(edwsmith)
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+
Comment on attachment 439604 [details] [diff] [review]
remove unnecessary conditionals from atomFromIntptrValue[_u]

(push me please)
Attachment #439604 - Flags: superreview?(stejohns)
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+
Attachment #439605 - Flags: superreview?(stejohns) → superreview+
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?
long overdue mark as fixed
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: flash10.1.1 → flash10.2.x-Spicy
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: