Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong return type for std::isinf/isnan(double) #108

Open
1 task done
ZzEeKkAa opened this issue Apr 10, 2024 · 23 comments
Open
1 task done

Wrong return type for std::isinf/isnan(double) #108

ZzEeKkAa opened this issue Apr 10, 2024 · 23 comments
Labels
bug Something isn't working

Comments

@ZzEeKkAa
Copy link

Solution to issue cannot be found in the documentation.

  • I checked the documentation.

Issue

Return type of std::isinf(double); must be bool, not int. Similar issue happens with pkgs/main.
Was able to reproduce both with clang++ and g++.

#include <cmath>

#include <type_traits>

// this one works as expected
static_assert(
  std::is_same_v < decltype(std::isinf(std::declval < float > ())), bool > );

// this works, but should not
static_assert(
  std::is_same_v < decltype(std::isinf(std::declval < double > ())), int > );

// this one does not work, but should
static_assert(
  std::is_same_v < decltype(std::isinf(std::declval < double > ())), bool > );

cpp reference (https://en.cppreference.com/w/cpp/numeric/math/isinf):

bool isinf( float num );
bool isinf( double num );
bool isinf( long double num );

To create environment:

conda create -n compiler -c conda-forge --override-channels gxx_linux-64 gcc_linux-64
conda activate compiler
x86_64-conda-linux-gnu-g++ --sysroot=${CONDA_BUILD_SYSROOT} -c test.hpp

Exact error:

test.hpp:15:8: error: static assertion failed
   15 |   std::is_same_v < decltype(std::isinf(std::declval < double > ())), bool > );
      |   ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Installed packages

# packages in environment at /home/yevhenii/.miniforge3/envs/compiler:
#
# Name                    Version                   Build  Channel
_libgcc_mutex             0.1                 conda_forge    conda-forge
_openmp_mutex             4.5                       2_gnu    conda-forge
binutils_impl_linux-64    2.40                 hf600244_0    conda-forge
binutils_linux-64         2.40                 hdade7a5_3    conda-forge
gcc_impl_linux-64         13.2.0               h338b0a0_5    conda-forge
gcc_linux-64              13.2.0               h1ed452b_3    conda-forge
gxx_impl_linux-64         13.2.0               h338b0a0_5    conda-forge
gxx_linux-64              13.2.0               he8deefe_3    conda-forge
kernel-headers_linux-64   2.6.32              he073ed8_17    conda-forge
ld_impl_linux-64          2.40                 h41732ed_0    conda-forge
libgcc-devel_linux-64     13.2.0             ha9c7c90_105    conda-forge
libgcc-ng                 13.2.0               h807b86a_5    conda-forge
libgomp                   13.2.0               h807b86a_5    conda-forge
libsanitizer              13.2.0               h7e041cc_5    conda-forge
libstdcxx-devel_linux-64  13.2.0             ha9c7c90_105    conda-forge
libstdcxx-ng              13.2.0               h7e041cc_5    conda-forge
sysroot_linux-64          2.12                he073ed8_17    conda-forge

Environment info

active environment : compiler
    active env location : /home/yevhenii/.miniforge3/envs/compiler
            shell level : 1
       user config file : /home/yevhenii/.condarc
 populated config files : /home/yevhenii/.miniforge3/.condarc
                          /home/yevhenii/.condarc
          conda version : 24.1.2
    conda-build version : not installed
         python version : 3.10.14.final.0
                 solver : libmamba (default)
       virtual packages : __archspec=1=skylake
                          __conda=24.1.2=0
                          __glibc=2.35=0
                          __linux=5.15.123.1=0
                          __unix=0=0
       base environment : /home/yevhenii/.miniforge3  (writable)
      conda av data dir : /home/yevhenii/.miniforge3/etc/conda
  conda av metadata url : None
           channel URLs : https://repo.anaconda.com/pkgs/main/linux-64
                          https://repo.anaconda.com/pkgs/main/noarch
                          https://repo.anaconda.com/pkgs/r/linux-64
                          https://repo.anaconda.com/pkgs/r/noarch
                          https://conda.anaconda.org/conda-forge/linux-64
                          https://conda.anaconda.org/conda-forge/noarch
          package cache : /home/yevhenii/.miniforge3/pkgs
                          /home/yevhenii/.conda/pkgs
       envs directories : /home/yevhenii/.miniforge3/envs
                          /home/yevhenii/.conda/envs
               platform : linux-64
             user-agent : conda/24.1.2 requests/2.31.0 CPython/3.10.14 Linux/5.15.123.1-microsoft-standard-WSL2 ubuntu/22.04.4 glibc/2.35 solver/libmamba conda-libmamba-solver/24.1.0 libmambapy/1.5.7
                UID:GID : 1000:1000
             netrc file : /home/yevhenii/.netrc
           offline mode : False
@ZzEeKkAa ZzEeKkAa added the bug Something isn't working label Apr 10, 2024
@h-vetinari
Copy link
Member

It's easy to confirm in godbolt that this should work as claimed in the OP.

The first suspicion I have is that this is somehow due to the underlying glibc, because std::isinf is in <cmath>, which wraps the C function isinf, and it's possible that the implementation in glib 2.12 (current baseline in conda-forge, though we're planning to move to 2.17 this summer) is deficient in that regard.

@ZzEeKkAa
Copy link
Author

ZzEeKkAa commented Apr 10, 2024

@h-vetinari thank you! Is there any way now to check if glib is the problem for the current issue?

@h-vetinari
Copy link
Member

You can add a compilation test to the recipe here, plus - {{ stdlib("c") }} under the build requirements, as well as

c_stdlib_version:    # [linux]
  - 2.17             # [linux]

under recipe/conda_build_config.yaml and then let the bot rerender the PR (or do it locally).

@ZzEeKkAa
Copy link
Author

I did some investigation with current build and that's what I've found:

from /home/yevhenii/.miniforge3/envs/compiler/x86_64-conda-linux-gnu/include/c++/13.2.0/cmath:

#ifndef __CORRECT_ISO_CPP11_MATH_H_PROTO_FP
  constexpr bool
  isnan(float __x)
  { return __builtin_isnan(__x); }

#if _GLIBCXX_HAVE_OBSOLETE_ISNAN \
  && !_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
  using ::isnan;
#else
  constexpr bool
  isnan(double __x)
  { return __builtin_isnan(__x); }
#endif

  constexpr bool
  isnan(long double __x)
  { return __builtin_isnan(__x); }
#endif

Then I've created test file:

#include <cmath>
#include <cstdio>


int main(){
#ifndef __CORRECT_ISO_CPP11_MATH_H_PROTO_FP
  printf("define float\n");

#if _GLIBCXX_HAVE_OBSOLETE_ISNAN \
  && !_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
  printf("not define double\n");
#else
  printf("define double\n");
#endif
  printf("define long double\n");
#endif


#if _GLIBCXX_HAVE_OBSOLETE_ISNAN 
  printf("_GLIBCXX_HAVE_OBSOLETE_ISNAN\n");
#endif

#if _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
  printf("_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC\n");
#endif

  return 0;
}

compile + run:

define float
not define double
define long double
_GLIBCXX_HAVE_OBSOLETE_ISNAN

So, for some reason cmath jumps into branch using ::isnan instead of defining isnan(double __x).

@h-vetinari do you know why is it happening?

@ZzEeKkAa
Copy link
Author

P.S.: output for the system g++:

define float
define double
define long double
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC

@h-vetinari
Copy link
Member

So it's not super-obvious how _GLIBCXX_HAVE_OBSOLETE_ISNAN ends up being set, but AFAICT it comes through:
libstdc++-v3/acinclude.m4 --> libstdc++-v3/config.h.in --> ... --> bits/c++config.h,
which, in our packaging (configured against very old glibc), ends up producing

/* Define if <math.h> defines obsolete isinf function. */
#define _GLIBCXX_HAVE_OBSOLETE_ISINF 1

I also found the following commit: gcc-mirror/gcc@350fe28, which means that _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC should be defined as true on systems with glibc >=2.23 through bits/os_defines.h (which I checked is #included in a local bits/c++config.h as packaged in our libstdcxx-devel_linux-64).

Given that your conda info shows __glibc=2.35=0, you should definitely satisfy the conditions for that, so I'm confused why _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC isn't true. Could you distinguish in your little test script between #ifdef <var> and #if <var>?

@isuruf
Copy link
Member

isuruf commented Apr 10, 2024

I can reproduce the same issue on Ubuntu 22.04 with gcc 11.4.0. So this is not unique to conda. Probably a gcc bug.

@h-vetinari
Copy link
Member

How did you reproduce it exactly? According to godbolt, it works back all the way to GCC + libstdcxx v7

@isuruf
Copy link
Member

isuruf commented Apr 10, 2024

Ubuntu 22.04 with stock gcc 11.4.0

@h-vetinari
Copy link
Member

h-vetinari commented Apr 10, 2024

Probably a gcc bug.

Or both ubuntu and conda-forge have the same kind of packaging bug...

@h-vetinari
Copy link
Member

Ubuntu 22.04 with stock gcc 11.4.0

What's the output of the test script above? What are the values of _GLIBCXX_HAVE_OBSOLETE_ISNAN / _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC?

@ZzEeKkAa
Copy link
Author

ZzEeKkAa commented Apr 11, 2024

Could you distinguish in your little test script between #ifdef <var> and #if <var>?

test.cpp:

#include <cmath>
#include <cstdio>
#include <gnu/libc-version.h>


int main(){
  printf("GNU libc version: %s\n", gnu_get_libc_version());
  printf("GNU libc release: %s\n", gnu_get_libc_release());

#ifndef __CORRECT_ISO_CPP11_MATH_H_PROTO_FP
  printf("define float\n");

#if _GLIBCXX_HAVE_OBSOLETE_ISNAN \
  && !_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
  printf("not define double\n");
#else
  printf("define double\n");
#endif
  printf("define long double\n");
#endif


#if _GLIBCXX_HAVE_OBSOLETE_ISNAN 
  printf("_GLIBCXX_HAVE_OBSOLETE_ISNAN\n");
#endif

#if _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
  printf("_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC\n");
#endif

#ifdef _GLIBCXX_HAVE_OBSOLETE_ISNAN 
  printf("def _GLIBCXX_HAVE_OBSOLETE_ISNAN\n");
#endif

#ifdef _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
  printf("def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC\n");
#endif

  return 0;
}

Ubuntu 22.04 output:

GNU libc version: 2.35
GNU libc release: stable
define float
define double
define long double
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC

Conda output:

GNU libc version: 2.35
GNU libc release: stable
define float
not define double
define long double
_GLIBCXX_HAVE_OBSOLETE_ISNAN
def _GLIBCXX_HAVE_OBSOLETE_ISNAN
def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC

@h-vetinari
Copy link
Member

Thanks! So the macro is defined but false.

I'm absolutely mystified how

#define _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC __GLIBC_PREREQ(2,23)

can evaluate to false on your system.

Ubuntu 22.04 output:

Can you provide a few more details on how you ran this? @isuruf said he can reproduce the problem on 22.04 with stock GCC, yet your 22.04 looks fine?

@ZzEeKkAa
Copy link
Author

@h-vetinari it is actually strange, because I've just tried stock ubuntu in docker container:

$ docker run -it --rm --platform linux/x86_64 ubuntu:22.04 bash
root@58295fbcf5e2:/# ldd --version
ldd (Ubuntu GLIBC 2.35-0ubuntu3.6) 2.35
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Written by Roland McGrath and Ulrich Drepper.
root@58295fbcf5e2:/# apt update && apt install g++ vim
root@58295fbcf5e2:/# vim test.cpp
root@58295fbcf5e2:/# g++ test.cpp
root@58295fbcf5e2:/# ./a.out
GNU libc version: 2.35
GNU libc release: stable
define float
define double
define long double
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC

@ZzEeKkAa
Copy link
Author

And 20.04 works as well (g++ (Ubuntu 9.4.0-1ubuntu1~20.04.2) 9.4.0):

GNU libc version: 2.31
GNU libc release: stable
define float
define double
define long double
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC

Ubuntu 18.04 (g++ (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0):

GNU libc version: 2.27
GNU libc release: stable
define float
define double
define long double
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC

Ubuntu 16.04 (g++ (Ubuntu 5.4.0-6ubuntu1~16.04.12) 5.4.0):


GNU libc version: 2.23
GNU libc release: stable
define float
define double
define long double

Ubuntu 14.04 (g++ (Ubuntu 4.8.4-2ubuntu1~14.04.4) 4.8.4):

GNU libc version: 2.19
GNU libc release: stable
define float
define double
define long double

@h-vetinari
Copy link
Member

Can you add __GLIBC_PREREQ(2,23) to the printed outputs? Something like

#include <features.h>
#define STR(x)   #x
#define SHOW_DEFINE(x) printf("%s=%s\n", #x, STR(x))

int main() {
  ...
  SHOW_DEFINE(__GLIBC_PREREQ(2,23));
  SHOW_DEFINE(_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC);
}

In godbolt this gets me

__GLIBC_PREREQ(2,23)=((2 << 16) + 35 >= ((2) << 16) + (23))
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC=((2 << 16) + 35 >= ((2) << 16) + (23))

@ZzEeKkAa
Copy link
Author

@h-vetinari sorry for long response, here is the output of conda:

GNU libc version: 2.35
GNU libc release: stable
define float
not define double
define long double
_GLIBCXX_HAVE_OBSOLETE_ISNAN
def _GLIBCXX_HAVE_OBSOLETE_ISNAN
def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
__GLIBC_PREREQ(2,23)=((2 << 16) + 12 >= ((2) << 16) + (23))
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC=((2 << 16) + 12 >= ((2) << 16) + (23))

And ubuntu output:

GNU libc version: 2.35
GNU libc release: stable
define float
define double
define long double
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
__GLIBC_PREREQ(2,23)=((2 << 16) + 35 >= ((2) << 16) + (23))
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC=((2 << 16) + 35 >= ((2) << 16) + (23))

See the difference + 12 and + 35

@ZzEeKkAa
Copy link
Author

ZzEeKkAa commented Apr 22, 2024

From sources:

#define __GLIBC_PREREQ(maj, min) \
	((__GLIBC__ << 16) + __GLIBC_MINOR__ >= ((maj) << 16) + (min))

That means, that __GLIBC_MINOR__ is defined as 12

So, having that, even upgrade to glibc 2.17 won't solve the issue.

@ZzEeKkAa
Copy link
Author

ZzEeKkAa commented Apr 22, 2024

Having that __GLIBC_MINOR__ and __GLIBC_PREREQ can't be changed, we need to focus on how _GLIBCXX_HAVE_OBSOLETE_ISNAN and _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC are defined:

/home/yevhenii/.miniforge3/envs/compiler/x86_64-conda-linux-gnu/include/c++/13.2.0/x86_64-conda-linux-gnu/bits/os_defines.h:
#define _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC __GLIBC_PREREQ(2,23)
/home/yevhenii/.miniforge3/envs/compiler/x86_64-conda-linux-gnu/include/c++/13.2.0/x86_64-conda-linux-gnu/bits/c++config.h:
#define _GLIBCXX_HAVE_OBSOLETE_ISNAN 1

Both those files are maintained by libstdcxx-devel.

My bet, is if conda is restricted to 2.12 glibc, compiler must not introduce environment variables set from later releases. But it's just a blind guess

@h-vetinari
Copy link
Member

Having that __GLIBC_MINOR__ and __GLIBC_PREREQ can't be changed,

__GLIBC_MINOR__ should not be 12 if you're on a glibc 2.35 system! Something somewhere seems to be overriding that macro.

@h-vetinari
Copy link
Member

__GLIBC_MINOR__ should not be 12 if you're on a glibc 2.35 system! Something somewhere seems to be overriding that macro.

Wait, this is probably coming in due to the presence of

sysroot_linux-64          2.12                he073ed8_17    conda-forge

Can you try installing sysroot_linux-64 =2.28?

If I do that, I get the following output with our compilers:

GNU libc version: 2.35
GNU libc release: stable
define float
define double
define long double
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC

@ZzEeKkAa
Copy link
Author

@h-vetinari yes! Sorry, I've looked at sysroot_linux-64 and for some reason thought it is glibc version. Indeed Setting sysroot_linux-64==2.28 gives the expected output:

GNU libc version: 2.35
GNU libc release: stable
define float
define double
define long double
_GLIBCXX_HAVE_OBSOLETE_ISNAN
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
def _GLIBCXX_HAVE_OBSOLETE_ISNAN
def _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC
__GLIBC_PREREQ(2,23)=((2 << 16) + 28 >= ((2) << 16) + (23))
_GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC=((2 << 16) + 28 >= ((2) << 16) + (23))

And indeed:

/home/yevhenii/.miniforge3/envs/compiler/x86_64-conda-linux-gnu/sysroot/usr/include/features.h:#define  __GLIBC_MINOR__ 28

sysroot is overriding it. Do you know how to use real glibc version?

@h-vetinari
Copy link
Member

Do you know how to use real glibc version?

Pointing to our own sysroot is kind of built-into our infrastructure, in the sense that we want to control the glibc baseline that packages build against, so that packages are widely installable, and don't end up depending on glibc features not available on older systems.

Since this is a runtime dependence here in the recipe

- sysroot_{{ cross_target_platform }}

you also cannot override this with the currently-published artefacts (and we can't remove it here because it's required to keep our infrastructure working).

Possible answers (we'll definitely need input from @conda-forge/linux-sysroot here):

  • Build sysroot_linux-64 for all glibc versions, so that users can "install" their local version
    • this is unrealistic because that recipe is currently based on RHEL-rpms (not the glibc sources), and those don't exist for every glibc version.
  • Create a "passthrough" sysroot variant that points to the user's glibc, in a way that the run-dep here gets satisfied
    • If we do that, we probably want to put this in a special label, so that the package cannot accidentally be installed in regular conda-forge CI
  • Some other clever thing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants