atomMaxIntValue and atomMinIntValue are apparently incorrect (and the latter unused)

RESOLVED FIXED in Q3 11 - Serrano

Status

P3
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: lhansen, Assigned: lhansen)

Tracking

unspecified
Q3 11 - Serrano
Bug Flags:
flashplayer-qrb +
flashplayer-bug +

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
atom.h:155: 

#ifdef AVMPLUS_64BIT
    // in 64-bit builds, integer atoms can hold a 53-bit signed value.
    // (this is so that integer values can be interchanged with doubles
    // with no loss of precision)
    const intptr_t atomMinIntValue = -(1LL<<53);
    const intptr_t atomMaxIntValue = (1LL<<53)-1;
#else
    // in 32-bit builds, integer atoms can hold a 29-bit signed value.
    const intptr_t atomMinIntValue = -(1L<<29);
    const intptr_t atomMaxIntValue = (1L<<29)-1;
#endif

The 32-bit values are incorrect.  It is correct that at atom can hold a 29-bit signed value, but the range is then -2^28..2^28-1, while 1L<<29 is 2^29.

The 64-bit values are apparently correct, but only because the double representation is sign+magnitude so we technically have a 54-bit signed number (as the values reveal).

atomMinIntValue is not used anywhere in the VM.

atomMaxIntValue is only used in ::atomIsValidIntptrValue_u(), where it is shifted in such a way that the erroneous value doesn't matter, but it's not quite clear to me why that's going on except possibly to work around the problem with the value of the constant being too large.

Are these constants used externally to the VM, ie, is this a public API?

Comment 1

9 years ago
Good catch. AFAIK these values are not used (or intended to be used) external to the VM itself. I'll do some grepping to verify. In the meantime, let's fix the values here (and add your comments expanding about how the values are derived).

Updated

9 years ago
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
(Assignee)

Updated

9 years ago
Assignee: nobody → lhansen
(Assignee)

Comment 2

9 years ago
Neither name is referenced in the player source code (frr-argo).
(Assignee)

Comment 3

9 years ago
Another bug here is the shift used for 64-bit values in atomIsValidIntptrValue, it should be 10, not 8.  (The function operates on untagged values, which have 54 bits of precision including the sign.)
(Assignee)

Comment 4

9 years ago
Created attachment 437966 [details] [diff] [review]
Patch
Attachment #437966 - Flags: review?(stejohns)

Updated

9 years ago
Attachment #437966 - Flags: review?(stejohns) → review+
(Assignee)

Comment 5

9 years ago
tamarin-redux changeset:   4362:87ad0ab7b931
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

9 years ago
There is a tiny bug here in the 64-bit case because a sign+magnitude representation has a range from -(2^(n-1)-1) to 2(n-1)-1.  The bug is not important because that value is not currently being used in the code, it is there for completeness.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

9 years ago
tamarin-redux changeset:   4367:07fe671debf0
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Updated

8 years ago
Flags: flashplayer-bug+
You need to log in before you can comment on or make changes to this bug.