Unsafe comparison of floats in SvTRUE #14840
Comments
From julien@trigofacile.comHi, Using SvTRUE() triggers a gcc warning (when building INN code source). In file included from /usr/lib/x86_64-linux-gnu/perl/5.20/CORE/perl.h:3335:0, ^ Could the check in sv.h be changed or, if it can't be improved, could a Thanks beforehand, -- Flags: Site configuration information for perl 5.20.2: Configured by Debian Project at Sun May 3 16:16:25 UTC 2015. Summary of my perl5 (revision 5 version 20 subversion 2) configuration: Platform: Locally applied patches: @INC for perl 5.20.2: Environment for perl 5.20.2: |
From @jkeenanOn Sat Aug 08 11:02:37 2015, julien@trigofacile.com wrote:
I am unfamiliar with 'INN code source'. Could you clarify? Thank you very much. -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @jkeenanOn Sat Aug 08 11:02:37 2015, julien@trigofacile.com wrote:
I suspect that people who try to reproduce this warning may have trouble doing so given the large number of configuration arguments you are using. With that large number of config args, it becomes difficult to say which one might be implicated in the warning. For example, using: ##### to configure and build Perl 5 blead on linux/x86_64, I was unable to reproduce that warning. (See attached for the warnings I did get.) Would it be possible for you to re-configure and re-build, starting with as few config args as possible, adding one at a time until you get the warning? Thank you very much. -- |
From @jkeenan647:../../embed.h:656:37: warning: ‘iv’ may be used uninitialized in this function [-Wmaybe-uninitialized] |
From @ikegamiOn Sun Aug 09 08:50:08 2015, jkeenan wrote:
Seems to me -Werror=float-equal should be enough to replicate the problem. Floating point numbers are often slightly different than expected due to rounding of periodic numbers (such as 1/10, 2/10, 3/10, 4/10, 6/10, 7/10, 8/10 and 9/10). As such, one should usually check if two floating pointer numbers are equal by checking if their difference falls within a tolerance. This isn't done here, so a warning is issued. |
From julien@trigofacile.comHi Eric,
Yes, exactly.
SvTRUE currently has "SvNVX(sv) != 0.0" whereas something like More information here: http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/ -- « La sécurité en informatique, c'est comme le sexe : c'est à |
From julien@trigofacile.comHi James,
I should have been more precise, you're right. The related code is at line 117 of: -- « Veux-tu rendre à César ce qui m'appartient ? » (César) |
From @bulk88On Sun Aug 09 14:06:16 2015, julien@trigofacile.com wrote:
That is bad Perl XS code. ERRSV macro has a getter C function internally, SvTRUE macro evaluates its arguments multiple times, leading to exponential calls of the getter function. Assign ERRSV to a C auto SV* before passing that SV * to SvTRUE macro. -- |
From @jkeenanOn Sun Aug 09 11:29:24 2015, ikegami@adaelis.com wrote:
Thank you for that suggestion. Here is the problem reproduced in Perl 5 blead with: ##### See attached file (slight editing for clarity), where 'make' died very quickly. -- |
From @jkeenanecho @`sh cflags "optimize='-O2'" perlmini.o` -DPERL_IS_MINIPERL -DPERL_EXTERNAL_GLOB perlmini.c In file included from perl.h:3726:0, from perl.c:33: perl.c: In function ‘Perl_eval_pv’: |
From @jkeenanOn Sun Aug 09 16:58:08 2015, jkeenan wrote:
Additional attachment: ack the core source code for potentially problematic code. -- |
From @jkeenanpp.c:1252: } else if (mod2 == 0.0) { /* even integer */ |
From @tonycozOn Sat Aug 08 11:02:37 2015, julien@trigofacile.com wrote:
The floating point check done by the macro needs to remain as it is - the macro implements perl's definition of "truthiness" for values, changing it to fabs(SvNVx(sv)) < 0.00001 would change what perl considers true, and be wrong in other ways.[1] Unfortunately using the diagnostic pragma on the macro is impractical. Putting the diagnostic pragmas around the macro definition (which might have worked if GCC was being smart about dealing with where the code comes from) doesn't work. So to suppress the warning we'd need to add the pragma around every single use of the SvTRUE() and SvTRUE_NN() macro, which isn't practical. The only alternative I can see is to make SvTRUE_common() into an inline function, and add the pragmas around that instead, but we'd need to add probes to Configure to check that the pragma is available and guard the pragma with whatever macro we define to indicate the pragma is available. I'm not sure it's worthwhile when you could just build without that option. Tony [1] using an epsilon of 0.00001 assumes we're working with values around 1.0, which isn't necessarily the case. |
From julien@trigofacile.comHi Tony,
Thanks for your answer.
Then FLT_MIN (defined in float.h) could be used.
Well, the problem is that the current code is not totally right because -- « Pas question de faire voler un tapis volé ! » (Astérix) |
From zefram@fysh.orgJulien ELIE wrote:
Where a fuzzy comparison is desired, that's not the way to do it.
The code is totally right in this respect. It is implementing the The float-equal warning is counterproductive when applied to the -zefram |
From @bulk88On Sun Aug 09 18:01:05 2015, tonyc wrote:
I'll add on, Perl assumes 0.0 is a double with all 0 bits on most (all?) platforms http://perl5.git.perl.org/perl.git/blob/6147b0088faa521d6b4bc8bb97279ad46f5365a0:/config_h.SH#l4981 . X87 instruction set also clearly points out what 0.0 is with its FLDZ "load zero" instruction http://docs.oracle.com/cd/E19455-01/806-3773/instructionset-183/index.html . +0.0 is all 0 bits. There is a certain movie about working in an office space and retiring rich off of micropennies. Perl isn't going to change to allow that to happen. Perl's FP math results are always iffy -float, double or long double or quad (not x86/not intel) All those permutations can cause the same PP code with same disk file inputs, to produce different bitpatterns/different precision results on different perl builds.
I agree. Not every warning is good, not every option of GCC is sane. How about PDP-11 TSK executable with x86 machine code? Perl is a VM, not a C program. Diagnostics usually need to happen on VM level at runtime/interp time, not C compiler level. GCC has an option called -fstrict-aliasing on by default at -O2. This makes casting pointers illegal/impossible/crashing. -fstrict-aliasing has been explictly disabled for 15 years in Perl, since the minute it came out. That disabling isnt going to change, since Perl isn't going to agree to a change in the definition of the C language. Same way, I dont think perl is going to change the meaning of FP 0 just because the C compiler devs decided to change it for right or wrong. Also how would you pick 0.00001 vs 0.000001 as the cut off rule for all Perl code in existence today? -- |
From julien@trigofacile.comHi bulk88,
Thanks for the suggestion, I will fix it. Could perlcall documentation also be fixed because SvTRUE(ERRSV) is an /* Check the eval first */ Would the following example be the right one to follow? SV *errsv = NULL; /* Check the eval first */ -- « Pas question de faire voler un tapis volé ! » (Astérix) |
From @bulk88On Mon Aug 10 02:05:32 2015, zefram@fysh.org wrote:
I am writing to inform you of http://www.nntp.perl.org/group/perl.perl5.porters/2015/07/msg229345.html since you have stopped responding to issue of Time::HiRes maintenance but you have posted in many other threads on P5P in the mean time. Perhaps you have an email problem. -- |
From @bulk88On Mon Aug 10 02:22:00 2015, julien@trigofacile.com wrote:
perlcall needs to be fixed. Upto 10-15 years ago ERRSV had no function calls inside, since 10-15 years ago when a change was made ERRSV now has a getter style function call inside.
SvPV_nolen(ERRSV) this is also bad, SvPV_nolen macro evaluates its argument multiple times.
^^^^^^^^^^^NULL initializer is redundant, ERRSV will always return something/evaluate to something
^^^^^^^^^^this is fine/correct
^^^^^^^^^^^^this needs to be fixed and split like SvTRUE and ERRSV was above
-- |
From julien@trigofacile.comHi bulk88,
Same thing for INN. We even still have around a ppport.h version 1.0003 We defined only what we were actually using. I should have a look to
OK, thanks.
Regarding the update of perlcall, what is the process so that it is not -- « Pas question de faire voler un tapis volé ! » (Astérix) |
From julien@trigofacile.comHi bulk88,
Well, that's a choice. -- « – Hé ! toi, qu'est-ce que tu dessines ? |
From @bulk88On Mon Aug 10 04:55:57 2015, julien@trigofacile.com wrote:
In my personal opinion, don't upgrade ppport.h if you dont know why, it keeps the repo smaller. If you know why you want to upgrade, such as you want a backport of a newer API, then upgrade ppport.h. Newer and future perls come with all the compat macros/layers to run older XS source code. There was a time during 5.5 and 5.6 era where some macros wound up only existing in ppport.h and were removed from core headers, but that hasn't happened since 5.6 era.
There is a ticket for it already https://rt-archive.perl.org/perl5/Ticket/Display.html?id=120903 -- |
From julien@trigofacile.comHi bulk88,
Oh, that's great. And I am also glad to see that the next Perl release I've thoroughly read the upcoming changed. They are pretty clear, thanks.
Thanks for sharing your thoughts about that. Thanks again for all your work on Perl. -- « Vous savez, les idées, elles sont dans l'air. Il suffit que |
|
The original issue here isn't a bug, and the side issue about access to ERRSV (and the typo) has been fixed. |
Migrated from rt.perl.org#125770 (status was 'open')
Searchable as RT125770$
The text was updated successfully, but these errors were encountered: