[Vm-dev] there might be a bug in FFIPlugin ffiPushSignedLongLongOop: it might fail to fail

[Nicolas Cellier]

Ah OK, you mean it is already corrected in ThreadedFFIPlugin :)
I checked COG, and it is OK.
The issue might still hold for classical Interpreter VM...

Thanks

2013/1/17 Eliot Miranda <eliot.miranda at gmail.com>:
>
> Hi Nicolas,
>
>
> On Wed, Jan 16, 2013 at 3:00 PM, Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com> wrote:
>>
>>
>> I'm not really sure, but I don't see any guard against too high a
>> magnitude to fit in a signed long long...
>> Thus the primitive migth fail to fail when passed (1<<63), though it
>> is most probably > LLONG_MAX.
>>
>> If someone can check whether this is a real bug or not by simply using
>> FFI (I haven't anything installed handy),
>> then we'll can think about corrections (find some ideas below)
>>
>> Nicolas
>
>
> The issue would have to be to do with signed64BitValueOf: since that's the converter in ffiPushSignedLongLongOop:in:
>
> ffiPushSignedLongLongOop: oop in: calloutState
> <var: #calloutState type: #'CalloutState *'>
> "Push a longlong type (e.g., a 64bit integer).
> Note: Coercions from float are *not* supported."
> | value |
> <var: #value type: #sqLong>
> (oop = interpreterProxy nilObject
> or: [oop = interpreterProxy falseObject])
> ifTrue:[value := 0] ifFalse:
> [oop = interpreterProxy trueObject
> ifTrue:[value := 1] ifFalse:
> [value := interpreterProxy signed64BitValueOf: oop.
> interpreterProxy failed ifTrue:
> [^FFIErrorCoercionFailed]]].
> ^self ffiPushSignedLongLong: value in: calloutState
>
> So provided that signed64BitValueOf: is correct ffiPushSignedLongLongOop:in: will be correct.  That can be tested independently of the FFI no?  I think it's tested by AlienSUnit>>testLongLong.
>
>
>>
>>
>> ----------------------------------------------------------------
>> My favorite way to do it now that C standards have evolved enough,
>> would be something simple like:
>>
>>     <var: #value type: #'signed long long'>
>>     <var: #magnitude type: #'unsigned long long'>
>>
>>     magnitude := objectMemory fetch... some bytes in a loop...
>>    (or directly fetch 1 or 2 32 bit words whenever we decide to store
>> LargeInt with VM native endianness 32 bit digits, which I'm currently
>> exploring).
>>
>>         (negative
>>                 ifTrue: [magnitude > 16r8000000000000000] "Once, we could have
>> written 16r8e15..."
>>                 ifFalse: [magnitude >= 16r8000000000000000])
>>                         ifTrue: [self primitiveFail].
>>         negative
>>                 ifTrue: [value := 0 - magnitude]
>>                 ifFalse: [value := magnitude].
>>
>> I'm partially happy with above code...
>> hardcoded constant, are not that nice, but:
>> * 2-complement though not guaranteed is quite ubiquitous (and we rely
>> on it in many places)
>> * 64 bits length is implicitely coded thru LargeInteger byte length
>> (8) somewhere above (not accounting for CHAR_BIT != 8)
>>
>> Of course, I would be even more happy if we
>> - used an int64_t - and  rename the primitive ;) - if we really mean
>> it to be on 64 bits,
>> - or did some assertion about sizeof(unsigned long long)
>>  and replaced hardcoded constants with
>>     if((negative)
>>         ? magnitude >  - (unsigned)(LLONG_MIN)
>>         : magnitude > (unsigned)(LLONG_MAX))
>> See http://stackoverflow.com/questions/12231560/correct-way-to-take-absolute-value-of-int-min
>> for germane discussion about standardness.
>>
>> Alas, I don't know how to use some already existing constants from
>> some standard header in slang....
>> Didn't someone ever think of using a standard constant? Should I dig
>> cCode: '....'?
>>
>> Otherwise, the code currently found in #signed64BitValueOf: might be correct...
>> It's just that it's too complex for me to check if it adheres to standards...
>
>
>
>
> --
> best,
> Eliot
>