ass_parse: simplify range check in dtoi32 #787
Conversation
| @@ -120,9 +120,10 @@ void ass_update_font(RenderContext *state) | |||
| */ | |||
| static inline int32_t dtoi32(double val) | |||
| { | |||
| if (isnan(val) || val <= INT32_MIN || val >= INT32_MAX + 1LL) | |||
| if (val > INT32_MIN && val < INT32_MAX + 1LL) | |||
There was a problem hiding this comment.
On the risk of missing something obvious again, is omission of the NaN check really safe?
Looking at ISO C99 7.12.14 §1:
The relational and equality operators support the usual mathematical relationships between numeric values. For any ordered pair of numeric values exactly one of the relationships — less, greater, and equal — is true. Relational operators may raise the ‘‘invalid’’ floating-point exception when argument values are NaNs. For a NaN and a numeric value, or for two NaNs, just the unordered relationship is true. 215) The following subclauses provide macros that are quiet (non floating-point exception raising) versions of the relational operators, and other comparison macros that facilitate writing efficient code that accounts for NaNs without suffering the ‘‘invalid’’ floating-point exception. In the synopses in this subclause, real-floating indicates that the argument shall be an expression of real floating type.
[followed by definitons of
isgreateretc macros]
- IEC 60559 requires that the built-in relational operators raise the ‘‘invalid’’ floating-point exception if the operands compare unordered, as an error indicator for programs written without consideration of NaNs; the result in these cases is false.
Comparisons with NaN will always raise a floating point exception. If we deal with IEEE floats the results (if/when normal control flow resumes) will be false and the check will work as intended regardless. But while IEEE only requires some flag be raised, it afaik allows for additional error handling like traps etc; are we comfortable with possibly invoking those?
If we don't deal with IEEE floats the program may just immediately terminate. So far when the question of whether or not we want to assume IEE floats came up, iiirc we always ended up avoiding relying on IEEE behaviour.
There was a problem hiding this comment.
Ah, these comparison operators should be replaced by isless and isgreater to avoid exceptions from NaNs.
There was a problem hiding this comment.
Will the result of comparsions with NaN always be false though when we don't have IEEE floats?
There was a problem hiding this comment.
Yes. This is given in your own quote:
For a NaN and a numeric value, or for two NaNs, just the unordered relationship is true.
No mention of IEEE float.
There was a problem hiding this comment.
Ah, these comparison operators should be replaced by
islessandisgreaterto avoid exceptions from NaNs.
Hmm, I pushed this change after testing and verifying it produces identical code on macOS and glibc, but now that I test further, I see it does produce substantially worse code on musl (which apparently doesn't use the gcc builtins for __isnan and friends), so I've reverted to the previous version. ICX also emits external calls to __isless and __isgreater. As far as I'm aware, it's not actually possible to get a NaN here, and this change came out of a discussion around adding -ffinite-math-only, so it seems probably-irrelevant.
Side-note: are we aware of any actual environment we'd plausibly run in where operations on NaNs actually trap? I'm pretty sure this isn't the only place that would be susceptible, but I'm also not aware of any real-world situation where it matters.
There was a problem hiding this comment.
Where does it define what result “unordered” relationships yield in comparisons? I missed it
§6.5.8 Relational operators:
Each of the operators
<(less than),>(greater than),<=(less than or equal to), and>=(greater than or equal to) shall yield 1 if the specified relation is true and 0 if it is false.
§7.12.14 Comparison macros (your earlier quote):
For any ordered pair of numeric values exactly one of the relationships — less, greater, and equal — is true. […] For a NaN and a numeric value, or for two NaNs, just the unordered relationship is true.
Quite simply, for a NaN, the less or greater relationship (as appropriate) isn’t true, so the relational operator doesn’t yield 1. (And the macros are defined to yield the same value as the operators.)
There was a problem hiding this comment.
As far as I'm aware, it's not actually possible to get a NaN here
I’ve just checked; it’s possible to get a NaN using a negative acceleration in \t:
Dialogue: 0,0:00:00.00,0:00:10.00,Default,,0,0,0,,{\t(0,1000,-1,\c&H808080&)}Black on the first frame
The k calculation in the complex_tag("t") block performs pow(time, accel) which is pow(0, -1) which is +infinity, and then calc_anim subtracts two infinities, which is NaN.
There was a problem hiding this comment.
Wait, do we actually care about floating-point exceptions at all? They're not a POSIX signal or a C++-style exception or anything, they're just a flag in a register we never read. For our purposes, comparisons with NaNs just return false, and we don't have to care about the side-effects.
There was a problem hiding this comment.
Wait, do we actually care about floating-point exceptions at all? They're not a POSIX signal
SIGFPE? And when it occurs, it defaults to aborting the process with a core dump, and to add insult to injury, “According to POSIX, the behavior of a process is undefined after it ignores a SIGFPE […] that was not generated by kill(2) or raise(3)”.
It’s just that (some?) modern architectures seem to disable triggering this signal by default. For example, on my Linux, fegetexcept() in a dummy test program returns 0. I can explicitly feenableexcept(FE_INVALID) and get a core dump on pow(0, -1). Ironically enough, I can’t seem to make it emit a signal on NaN < 0 (which was the crux of this discussion) even with a manually memcpy’d signaling NaN and -fsignaling-nans.
There was a problem hiding this comment.
SIGFPE is misleadingly-named; it's used for math exceptions in general (more commonly integer than floating-point ones), and some math floating-point traps raise a signal other than SIGFPE (eg on macOS they can raise SIGILL). IEC 60559 mandates that trapping behavior be disabled at process startup, and in practice this is true on all realistic target platforms I'm aware of. I don't think it'd be reasonable to try to support every possible non-default floating-point environment (including alternate rounding modes, etc), and if we did want to do so, I think the only sane solution would be to set the environment to all-defaults at entry to libass and then back to the previous values before return.
This is equivalent, but compilers seem to handle it better (and never emit a call to isnan, which occasionally happens otherwise).
This is equivalent, but compilers seem to handle it better (and never emit a call to isnan, which occasionally happens otherwise).