Repository: numpy/numpy
Pull request: BLD: check if std=c99 is really required
Fixes gh-15237. We were unconditionally setting `std=c99` even where not needed, which in some cases degraded the compiler.

@pv ping.

At some point we can just give up, rip all this out, and tell users to update their compiler.

Problem statement:
gcc -std=c99 flag and excess floating point precision handling
Numpy 1.18.0 adds `-std=c99` unconditionally to C compiler options. This has indirect consequences, dealing with which may be annoying for downstream projects.

One issue is that for gcc, `-std=c99` also turns on `-fexcess-precision=standard` which causes floating-point behavior changes with `-mfpmath=387` (i.e. typically on by default on i386 platform, but not on x86_64).

A particular point here is that the floating point literal macros in `npy_math.h` probably would need casts added, along the lines of `#define NPY_PI_2     ((double)1.570796326794896619231321691639751442)` in order to behave in a non-surprising way.

Granted, downstream projects maybe can fix this type of problems in their code, but I'm not sure if Numpy should force the compiler configuration change on them.

### Reproducing code example:

Note that `-std=c99` changes floating point excess precision handling:
```
cat <<__EOF__ > bad.c
#include <stdio.h>

int main() 
{
     double x = 1e300;
     if (x == 1e300) {
        printf("OK\n");
     } else {
        printf("FAIL\n");
    }
}
__EOF__
gcc --version
# -> gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1)
gcc -mfpmath=387 -I/usr/include/python3.7m -o bad bad.c
./bad
# -> OK
gcc -std=c99 -mfpmath=387 -I/usr/include/python3.7m -o bad bad.c
./bad
# -> FAIL
gcc -std=c99 -mfpmath=387 -fexcess-precision=fast -I/usr/include/python3.7m -o bad bad.c
./bad
# -> OK
```
Adding `(double)` cast before the literal makes the comparison behave as expected. This also hits e.g. numeric floating constants defined in Numpy headers.

Hints from issue discussion:
I'm perfectly happy to remove it. @mattip I don't recall the motivation for the change, what was it?
We have c99 code in pocketfft. Maybe we could add it only in that setup.py
> We have c99 code in pocketfft.

@mattip But that wasn't a problem for the tests or the builds, we just added the flag when needed. What was the specific problem it solved?
We added the flag in CI. Then people built from source as part of installing another package that depends on NumPy and got build errors. gh-14702, gh-12766, gh-14147
@mattip OK, thanks for the reminder. 
`-std=c99` flag has unintended consequences on modern Ubuntu development release. It forces the precision to reduce on s390x and results in misscompilation to the point of loosing more than 10^-6 floating point precission.

Ubuntu in groovy defaults to C18 standard. It would be nice for numpy configuration to test which standard the compiler defaults, and raise it to C99 if it is below C99. But never lower it down to C99.

```
$ gcc -dM -E - < /dev/null | grep STDC_VER
#define __STDC_VERSION__ 201710L

$ gcc -std=c99 -dM -E - < /dev/null | grep STDC_VER
#define __STDC_VERSION__ 199901L
```

`201710L` constant is for C18  (ISO/IEC 9899:2018).

At the moment we had to revert and remove the "std=c99" flag out of numpy. Would it be more appropriate to test that compiler is below c99 and only add that flag to raise the standard?
> It forces the precision to reduce on s390x

Out of curiosity, why does it do that? Is poor precision part of the c99 standard?
> > It forces the precision to reduce on s390x
> 
> Out of curiosity, why does it do that? Is poor precision part of the c99 standard?

I was pointed at a wikipage explaining how the exponent and floating point number of digits for precision is different on S/390 because it predates C language. Thus on s390x, higher standard version is required when all of that was aligned. Or like quad precision is adequate everywhere. Let me find that again.
> > > It forces the precision to reduce on s390x
> > 
> > 
> > Out of curiosity, why does it do that? Is poor precision part of the c99 standard?
> 
> I was pointed at a wikipage explaining how the exponent and floating point number of digits for precision is different on S/390 because it predates C language. Thus on s390x, higher standard version is required when all of that was aligned. Or like quad precision is adequate everywhere. Let me find that again.

https://en.wikipedia.org/wiki/IBM_hexadecimal_floating_point

Maybe in addition to c99 flag we should be specifying to use the IEEE precision? or maybe just that?

With or without, s390x precision issue, for Ubuntu, I don't wish any code to use c99 anymore, because there are too much undefined behavior and bugs in c99.
@xnox Sounds like c99 is abused to flag backwards compatibility with an outdated compiler :) Well, it is what it is.

Commit message: BUG: fixes failures (from review)

Patch to review:
diff --git a/setup.py b/setup.py
index 8178a6c51cb1..aa2ad39138ef 100755
--- a/setup.py
+++ b/setup.py
@@ -189,7 +189,6 @@ def check_submodules():
     for line in status.splitlines():
         if line.startswith('-') or line.startswith('+'):
             raise ValueError('Submodule not clean: {}'.format(line))
-            
 
 
 class concat_license_files():
@@ -235,19 +234,25 @@ def get_build_overrides():
     from numpy.distutils.command.build_clib import build_clib
     from numpy.distutils.command.build_ext import build_ext
 
-    def _is_using_gcc(obj):
-        is_gcc = False
+    def _is_using_old_gcc(obj):
+        is_old_gcc = False
         if obj.compiler.compiler_type == 'unix':
-            cc = sysconfig.get_config_var("CC")
-            if not cc:
-                cc = ""
-            compiler_name = os.path.basename(cc)
-            is_gcc = "gcc" in compiler_name
-        return is_gcc
+            cc = obj.compiler.compiler[0]
+            if "gcc" in cc:
+                out = subprocess.run([cc, '-dumpversion'],
+                                     stdout=subprocess.PIPE,
+                                     stderr=subprocess.PIPE,
+                                    )
+                # will print something like b'4.2.1\n'
+                ver_parts = out.stdout.split(b'.')
+                if int(ver_parts[0]) < 6:
+                    # perhaps 5 is OK?
+                    is_old_gcc = True
+        return is_old_gcc
 
     class new_build_clib(build_clib):
         def build_a_library(self, build_info, lib_name, libraries):
-            if _is_using_gcc(self):
+            if _is_using_old_gcc(self):
                 args = build_info.get('extra_compiler_args') or []
                 args.append('-std=c99')
                 build_info['extra_compiler_args'] = args
@@ -255,7 +260,7 @@ def build_a_library(self, build_info, lib_name, libraries):
 
     class new_build_ext(build_ext):
         def build_extension(self, ext):
-            if _is_using_gcc(self):
+            if _is_using_old_gcc(self):
                 if '-std=c99' not in ext.extra_compile_args:
                     ext.extra_compile_args.append('-std=c99')
             build_ext.build_extension(self, ext)


Code review comments:
diff_hunk: @@ -235,27 +234,33 @@ def get_build_overrides():
     from numpy.distutils.command.build_clib import build_clib
     from numpy.distutils.command.build_ext import build_ext
 
-    def _is_using_gcc(obj):
-        is_gcc = False
+    def _is_using_old_gcc(obj):
+        is_old_gcc = False
         if obj.compiler.compiler_type == 'unix':
-            cc = sysconfig.get_config_var("CC")
-            if not cc:
-                cc = ""
-            compiler_name = os.path.basename(cc)
-            is_gcc = "gcc" in compiler_name
-        return is_gcc
+            cc = obj.compiler.compiler[0]
+            if "gcc" in cc:
+                out = subprocess.run([cc, '-dumpversion'],
+                                     stdout=subprocess.PIPE,
+                                     stderr=subprocess.PIPE,
+                                    )
+                # will print something like b'4.2.1\n'
+                ver_parts = out.stdout.split(b'.')
+                if int(ver_parts[0]) < 6:
path: setup.py
text: @user1:
Or perhaps:
```suggestion
                # will print something like b'4.2.1\n'
                ver = LooseVersion(out.stdout.strip())
                if ver < "6.0.0":
```

@author:
I think `packaging.version.parse` is to be preferred over `distutils.version.LooseVersion`, see [this comment](https://github.com/pypa/setuptools/issues/2261#issuecomment-660343237) in the issue about setuptools taking over distutils. 

@author:
changed to use LooseVersion, we can revisit if the preference becomes more widespread