Re: [Cbor] Implementing float->int numeric reduction

Hi Thiago,

Appreciate your comments here and the ref to the C/C++ standard.

It sounds to me that those of us that want completely bullet-proof code for every compiler and platform and that want to guarantee passing static analyzers and such should put the range check in. The check is probably unnecessary for the popular compilers and CPUs, but there might be one somewhere for which it is.

Would be interesting to see what the rust definition says. Maybe it’s not UB in rust (in which case the compiler is doing something).

LL

> On Aug 17, 2023, at 9:46 AM, Thiago Macieira <thiago.macieira@intel.com> wrote:
> 
> On Thursday, 17 August 2023 08:52:06 PDT you wrote:
>>>> If the value of the integral part cannot be represented by the integer
>>>> type, the behavior is undefined.
>> 
>> OK, officially it is undefined in the sense that it could burn down the
>> computer. In reality, a value will be returned by the conversion to
>> integer, however weird that value will be. Now that value is then checked
>> against the floating point version (is there a need for the cast, BTW?), so
>> if the value is weird, the reduction won’t happen.
> 
> There are two problems with UB here. The first is the reason why this is UB in 
> the first place: different architectures produce different values for the 
> conversion if it falls outside the range. Some may saturate to the maximum and 
> minimum, others may set to a specific value (SSE and x87 do this), others may 
> discard the value above the integral limit and produce essentially garbage. 
> This problem is mitigated by converting back and comparing to the original 
> value, but it can still lead to problems.
> 
> The second is that compilers often optimise based on the assumption you never 
> trigger UB, so if they see you performing a conversion from FP to integral, 
> they are allowed to assume that the value was in range in the first place. And 
> if it does eliminate the cases where the FP input was out of range, the 
> comparison failing can only be the result of there being a fractional 
> component. If you combine that with it somehow knowing the absolute value of 
> the input was above 2^53 (meaning a double-precision can't have a fraction), 
> the compiler could optimise the entire check out of existence.
> 
> In any case, this is an implementation detail for those implementations that 
> use languages with undefined behaviour (like mine). The wording is correct.
> 
>> (To avoid checking against both int64 and uint64, I would probably extract
>> the sign first.)
> 
> Do note the conversion to uint64_t is more expensive due to the lack of such 
> instruction for x86 until AVX512 came along.
> 
>>> I ran into this issue before, in both TinyCBOR and Qt's wrapper around it.
>> 
>> Can you help me understand this issue better by explaining how it manifested
>> there?
> 
> In our particular case, it was just the UBSan complaining. I don't remember it 
> manifesting in real life. I also noticed this because I was mostly trying to 
> suppress the -Wfloat-equal warning.
> 
> Here's the original implementation:
> https://codebrowser.dev/qt5/qtbase/src/corelib/global/qnumeric_p.h.html#205
> 
> I ended up writing an optimised version with intrinsics because then I am 
> allowed to assume certain things. You can see it in the Qt6 update to the 
> above:
> 
> https://codebrowser.dev/qt6/qtbase/src/corelib/global/qnumeric_p.h.html#154
> 
>>> PS: I haven't yet verified whether QCborValue already generates dCBOR
>>> without even trying. It's possible it does. But it doesn't support
>>> integers above 2^63-1.
>> 
>> If your application doesn’t use (generate and/or is expected to consume)
>> uint64\int64, i.e., [2**63..2**64-1], that may not actually matter.  (But
>> it is a restriction that is not that widely found in CBOR implementations.)
> 
> QCborValue is a library class, meaning lots of applications and other 
> libraries could use it to implement their decoding of existing protocols.
> 
> Strictly speaking, QCborValue supports those values by aliasing them to 
> negative ones in int64_t. If the application knows the payload was unsigned, 
> it can convert back to unsigned and retrieve the original losslessly (and 
> without UB).
> 
> But it's impossible to tell what the payload actually was. So one 
> recommendation I'd make is that protocols choose *either* [0..2^64-1] or 
> [-2^63..2^63-1], but never [-2^63..2^64-1]. The entire range is representable 
> in CBOR and dCBOR, but protocols should avoid making use of that.
> 
> -- 
> Thiago Macieira - thiago.macieira (AT) intel.com
>  Cloud Software Architect - Intel DCAI Cloud Engineering
> _______________________________________________
> CBOR mailing list
> CBOR@ietf.org
> https://www.ietf.org/mailman/listinfo/cbor