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

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

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

[Steven Johnson]

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).

Comment 2

[Lars T Hansen]

Neither name is referenced in the player source code (frr-argo).

Comment 3

[Lars T Hansen]

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.)

Comment 4

[Lars T Hansen]

Attachment: Patch

Comment 5

[Lars T Hansen]

tamarin-redux changeset:   4362:87ad0ab7b931

Comment 6

[Lars T Hansen]

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.

Comment 7

[Lars T Hansen]

tamarin-redux changeset:   4367:07fe671debf0