GNOME Bugzilla – Bug 779399
gjs 1.47.91 test failures on non-amd64
Last modified: 2017-03-15 18:32:42 UTC
gjs 1.47.91 Ubuntu 17.04 Beta Ubuntu's autopkgtests are failing for gjs 1.47.91 on all tested non-amd64 architectures. These tests passed with 1.47.90 on all architectures except s390x. http://autopkgtest.ubuntu.com/packages/gjs I'm guessing this is because of https://git.gnome.org/browse/gjs/commit/?id=048fceed7 And I think this is the relevant part of the test output: JS G NS: Found info type FUNCTION for 'test_assert_expected_messages_internal' in namespace 'GLib' ** GLib:ERROR:testEverythingBasic.js:0:Limits warns when conversion is lossy: Did not see expected message Gjs-WARNING **: *cannot be safely stored* FAIL: gjs/testEverythingBasic.test (Child process killed by signal 6)
I googled "excess precision" and learned something new today. So probably this check is platform-dependent. It is what Mozilla uses in SpiderMonkey for the assert that I was trying to avoid. However, I wonder if it's possible to use something like std::numeric_limits to see if a given 64-bit integer is possible to represent losslessly in a double.
Created attachment 347020 [details] [review] RFC - Platform-independent is_safe_to_store_in_double() This ought to tell whether a given int64 is safe to store in a double, without danger of being misled by excess precision. It seems like the cure is worse than the disease, though - this code is quite obscure. Maybe it would be better just to be simple and warn if number > 2^53, even though a particular value might be possible to represent losslessly.
Created attachment 347291 [details] [review] RFC - Platform-independent is_safe_to_store_in_double() This ought to tell whether a given int64 is safe to store in a double, without danger of being misled by excess precision. The idea is that the volatile keyword will cause the compiler to treat the double as an actual double memory location, and not make use of any excess precision.
Jeremy, do either of these patches cause the tests to pass on the affected architectures?
Which do you want me to try first?
With the second patch (347291), the tests now pass on i386 but still fail on armhf and ppc64el. Pass ---- https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-zesty-jbicha-arch/zesty/amd64/g/gjs/20170306_024959_e1c23@/log.gz https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-zesty-jbicha-arch/zesty/i386/g/gjs/20170306_024946_e1c23@/log.gz Fail ---- https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-zesty-jbicha-arch/zesty/armhf/g/gjs/20170306_025451_e1c23@/log.gz https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-zesty-jbicha-arch/zesty/ppc64el/g/gjs/20170306_025135_e1c23@/log.gz
The first patch (347020) passes on all architectures that passed on 1.47.90: amd64 i386 armhf ppc64el
Created attachment 347294 [details] [review] RFC - Platform-independent is_safe_to_store_in_double() This ought to tell whether a given int64 is safe to store in a double, without danger of being misled by excess precision.
Too bad. I liked the second patch much better since it's agnostic as to how doubles are stored on the platform, the first patch assumes IEEE 754 representation. But maybe "volatile" doesn't really mean anything for those architectures on which it still fails. Here's yet another patch based on one of the answers from that Stack Overflow question. Sorry to keep firing off patches into the dark, but would you mind trying this one as well?
Review of attachment 347291 [details] [review]: Determined not to be platform-independent.
The 347291 patch also fails on armfh and ppc64el Pass ---- https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-zesty-jbicha-arch/zesty/amd64/g/gjs/20170306_050322_60667@/log.gz https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-zesty-jbicha-arch/zesty/i386/g/gjs/20170306_050203_60667@/log.gz Fail ---- https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-zesty-jbicha-arch/zesty/armhf/g/gjs/20170306_050715_60667@/log.gz https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-zesty-jbicha-arch/zesty/ppc64el/g/gjs/20170306_050440_60667@/log.gz
Review of attachment 347294 [details] [review]: Also determined not to be platform-independent.
Cosimo, what do you think about this? My preference would be to simply check if abs(number) > 2^53 and ignore the cases where the number can be safely stored because its lower bits are 0.
Review of attachment 347020 [details] [review]: After reading the linked resources, I think this is fairly reasonable; I would not mind carrying this code, especially since the patch is already written and tested but I'd be OK with the alternative proposal if you feel strongly against it.
(In reply to Cosimo Cecchi from comment #14) > Review of attachment 347020 [details] [review] [review]: > > After reading the linked resources, I think this is fairly reasonable; I > would not mind carrying this code, especially since the patch is already > written and tested but I'd be OK with the alternative proposal if you feel > strongly against it. I'm fairly confident it works, but the more I think about it the less I like it. What if you marshal an integer > 2^53 that does store losslessly without a warning, then you add 1 to it in JS? Sounds like more confusion for little gain.
Fair point, but wouldn't that be handled on the JS side?
Well, no,... gjs> Number.MAX_SAFE_INTEGER 9007199254740991 gjs> Number.MAX_SAFE_INTEGER + 1 9007199254740992 gjs> Number.MAX_SAFE_INTEGER + 2 9007199254740992 gjs> Number.MAX_SAFE_INTEGER + 3 9007199254740994 But what I mean is that if you marshal 9007199254740993 (cannot be stored losslessly) through this code, and get a warning, then I actually find it misleading to _not_ get a warning if you marshal 9007199254740994.
Created attachment 347363 [details] [review] RFC - arg: Simpler is_safe_to_store_in_double() It could be considered misleading not to simply warn if the value being marshalled is > 2^53. In addition, warning only when the value itself is truncated is difficult, though not impossible, to do in a cross-platform way.
Review of attachment 347363 [details] [review]: I am OK with this.
(In reply to Philip Chimento from comment #17) > Well, no,... > > gjs> Number.MAX_SAFE_INTEGER > 9007199254740991 > gjs> Number.MAX_SAFE_INTEGER + 1 > 9007199254740992 > gjs> Number.MAX_SAFE_INTEGER + 2 > 9007199254740992 > gjs> Number.MAX_SAFE_INTEGER + 3 > 9007199254740994 > > But what I mean is that if you marshal 9007199254740993 (cannot be stored > losslessly) through this code, and get a warning, then I actually find it > misleading to _not_ get a warning if you marshal 9007199254740994. Understood; my point was that the binding would still have done everything in its power to precisely convey the value to the JS, but I agree that it does not buy us much to do that. Feel free to commit the simpler patch if you prefer that approach.
Thanks. The commit fixed this bug for me.
This currently breaks the build for me on Fedora 26. After reverting 44aa965 gjs compiles again. gjs/gi/arg.cpp: In instantiation of 'bool is_safe_to_store_in_double(T) [with T = long unsigned int]': gjs/gi/arg.cpp:2626:54: required from here gjs/gi/arg.cpp:2581:21: error: call of overloaded 'abs(long unsigned int&)' is ambiguous /usr/include/c++/7/bits/std_abs.h:102:3: note: candidate: constexpr __float128 std::abs(__float128) /usr/include/c++/7/bits/std_abs.h:84:3: note: candidate: constexpr __int128 std::abs(__int128) /usr/include/c++/7/bits/std_abs.h:78:3: note: candidate: constexpr long double std::abs(long double) /usr/include/c++/7/bits/std_abs.h:74:3: note: candidate: constexpr float std::abs(float) /usr/include/c++/7/bits/std_abs.h:70:3: note: candidate: constexpr double std::abs(double) /usr/include/c++/7/bits/std_abs.h:61:3: note: candidate: long long int std::abs(long long int) /usr/include/c++/7/bits/std_abs.h:56:3: note: candidate: long int std::abs(long int) /usr/include/stdlib.h:751:12: note: candidate: int abs(int)
Created attachment 347723 [details] [review] arg: Simplify max safe integer check Newer, stricter, compilers, such as VS2013 and GCC on Fedora 26, will warn about calling std::abs() on an unsigned integer, since it's ambiguous which signed integer it should convert to. Now that we've settled on a cross-platform form of the check, it doesn't actually have to be a template, and can just be done inline. Therefore we can skip calling std::abs() on the unsigned number.
OK, how about this? It gets rid of the templates altogether. This should also solve Fan Chun-Wei's problem from bug 775868.
Review of attachment 347723 [details] [review]: This makes sense to me, but at this point I'd rather have a confirmation from Sebastian that it fixes the issue.
I'm going to go ahead and commit it; regardless of whether it fixes the problem, I consider it an improvement. And if it does fix the problem, then we won't have to request a freeze break.
Comment on attachment 347723 [details] [review] arg: Simplify max safe integer check Attachment 347723 [details] pushed as 8de0518 - arg: Simplify max safe integer check
I can confirm that the patch fixes the build on Fedora 26.
Cool, thanks!
Hi Philip, This does indeed fix the build on Windows as well. Sorry for the belated feedback. With blessings, thank you!
No problem, thanks for letting me know.