Closed sebastic closed 1 year ago
@sebastic Those areas of the code haven't changed since PROJ 9.2.1, and I see in https://buildd.debian.org/status/fetch.php?pkg=proj&arch=s390x&ver=9.2.1-1&stamp=1686496069&raw=0 they were successful, but there was a bump of gcc version from 12 to 13.
The grid->isNodata(-88.8888f, 1.0) failure is particularly concerning on the correctness of floating-point operations on that architecture, since GTXVerticalShiftGrid::isNodata(float val, double multiplier) is
return val * multiplier > 1000 || val * multiplier < -1000 ||
val == -88.88880f;
I can't imagine a good reason for that to fail...
We have a s390x CI target and it runs master successfully: https://app.travis-ci.com/github/OSGeo/PROJ/jobs/608699310 , but with an older gcc version. Hence my strong suspicion it is an issue with gcc 13.
We'll ignore the test failure on s390x for the time being.
I've contacted the s390 porters about this issue, they might be able to look into the gcc-13 regression.
We (Gayathri and myself) have started looking into this issue and will have an update sooner on this.. Thanks..
Hi @sebastic / @rouault , We have extracted the logic as individual test case where it is failing and causing the issue. When we debug in multiple ways we found that, there is flag CMAKE_CXX_EXTENSIONS OFF in CMakelists.txt file is causing the issue. by default this flag is enabled and perform optimization. when this flag is enabled the -std is set to c++11 and Due to this the test suites which has the float variable comparison with the negative float constant and any other float value comparison are failing. when CMAKE_CXX_EXTENSIONS is on, we noticed that “-std” flag will be set to “gnu++11” i.e. “-std==gnu++11” and hence the test suites are passing without any errors…
Do I understand correctly that the c++11 implementation in GCC is buggy?
Yes we have checked with our compiler team on this issue. They communicated with the gcc maintainers as well and confirmed they will enable the configure switch in next debian and ubuntu releases. when we enable CMAKE_CXX_EXTENSIONS flag ON (-std=gnu++11), haven't seen any errors. Please confirm us from your side, and if needs a work around we are ready a raise PR for that change and fix.
We already ignore the test failure on s390x. That could be replaced with a patch to set CMAKE_CXX_EXTENSIONS
, but that doesn't seem worth the effort.
Once the issue is fixed in GCC we can stop ignoring the test failure.
The gcc-13 changelog mentions:
- Configure with --disable-s390-excess-float-precision for sid/trixie and Ubuntu noble (24.04 LTS).
Is this the change in question?
Yes, with that change the problem will go away.
Actually --disable-s390-excess-float-precision is the default when configuring GCC on IBM Z. The Debian/Ubuntu compiler accidentally has been configured with --enable-s390-excess-float-precision, which was supposed to be only a temporary workaround.
For C++ compilers with excess precision enabled, there is a problem with literals due to contradicting statements in the current version of the C++ standard. This is currently being debated in the C++ standard committee: https://cplusplus.github.io/CWG/issues/2752.html https://github.com/cplusplus/papers/issues/1584
This problem makes the testcase fail when using excess precision compilers. So while it might take a bit to get an actual fix for the problem. Disabling excess precision in GCC for IBM Z is the right way to go and should be used everywhere.
The OSGeo testcase problem surfaced with GCC 13 since older GCCs did not use the excess precision settings for C++ (only for C).
Confirmed fixed with the latest gcc-13 from Debian unstable on the s390x porterbox.
Example of problem
Full buildlog: s390x
Environment Information
proj
): 9.3.0-rc1Installation method