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

Build script fails on OS X because it is unable to check if an IEEE floating point value is finite #18

Closed
mandeluna opened this issue Mar 7, 2022 · 18 comments

Comments

@mandeluna
Copy link

configure: WARNING: Cannot find a C function to check if an IEEE floating point
value is finite. There is no hope of building dylp on this
system.

Continuation of discussion with @LouHafer on coin-or-tools/homebrew-coinor #80

The root cause is that isfinite is a macro on Mac OS, so AC_CHECK_FUNC is unable to resolve the symbol in ac_dylp_find_fp_funcs.m4

However, replacing the AC_CHECK_FUNC for isfinite with the following check does not resolve the symbol either:
AC_CHECK_DECL([isfinite],[ac_name_of_isfinite=isfinite],,[#include <math.h>])

Some searching finds similar problems in the past:

https://list.coin-or.org/pipermail/coinutils/2011-November/000027.html
and
https://issues.fast-downward.org/issue295

According to the mailing list discussion, there seems to be (at least circa 2011) some odd behaviour related to importing <math.h> vs but the code builds with isfinite and using std::isfinite seems ridiculous, and, anyway, after trying various permutations of those, it is still unable to resolve anything.

Suggestions are welcome.

@LouHafer
Copy link
Contributor

LouHafer commented Mar 7, 2022

@mandeluna --- Looks like I just need to update dylp's configure to use the current coin macros for isfinite and isnan. One of the many rough edges to smooth out in this update. I'll do that over the course of the day and try to have something ready this afternoon or tomorrow.

If there are no issues with configuration of CoinUtils, this should work, as CoinUtils uses the same macros I'm going to plug into dylp. You can check by looking in the build directory at the file CoinUtils/src/config.h, the symbol COINUTILS_C_FINITE. If it's defined to something sensible, we're good.

@mandeluna
Copy link
Author

Thanks @LouHafer -- CoinUtils builds with #define COINUTILS_C_FINITE std::isfinite

BuildTools/coin_math.m4 has a macro that handles this correctly

@tkralphs
Copy link
Member

tkralphs commented Mar 7, 2022

Just keep in mind that there hasn't been a release of any projects in the Cbc stack using the "new" BuildTools (master branch) yet and the master version is incompatible with stable/0.8. So you can fix the issue in master, but any release would still have to use stable/0.8 for now. As the original issue was for a building within homebrew, it may not be easy to fix in a way that would allow packaging and release under homebrew unless you roll your own macro to go along with stable/0.8.

@LouHafer
Copy link
Contributor

LouHafer commented Mar 7, 2022

@mandeluna, @tkralphs --- Ouch, forgot about that. I did (sort of) have to roll my own macro. The ones in coin_math.m4 insist on trying in the C++ context first, a non-starter for dylp. Easy enough to replace the guts of my old macros with the guts of the new coin macros, so I could try a backport. Painful, though. I've been working with BuildTools master ever since we got started writing the new version. I'll have to recreate an environment suitable for Buildtools/stable/0.8. @mandeluna, how important is that (compatibility with stable/0.8) to you? Do you actually work with the autotools stuff? Or will the ability to pull a working Dylp/master do the trick?

@tkralphs
Copy link
Member

tkralphs commented Mar 7, 2022

I guess the main question is whether @mandeluna wants a homebrew install of DyLP, in which case there is no choice but for it to be from a release.

@mandeluna
Copy link
Author

I ran into this issue while trying to install Symphony. I still haven't got all the pieces to work together on this laptop as I have been working off the master branch, but I am curious about how well the solver performs on this architecture.

I wanted to record the issue here in case others run into the same problem. It is not urgent for me, as I had dusted off an old computer and fell back to a precompiled Linux package for the work I was doing.

@LouHafer
Copy link
Contributor

LouHafer commented Mar 8, 2022

@mandeluna, I've committed some revisions to dylp's configure macros (as per above, replaced the guts of the original macros in m4/ac_dylp_find_fp_funcs.m4 with the guts of the current macros from coin_math.m4). Give it a try when you have a chance and let me know if configure can find isfinite now. If it still fails, it'll help me if you attach the config.log file produced by when configure is run.

@svigerske
Copy link
Member

isfinite() is C99 (https://en.cppreference.com/w/c/numeric/math/isfinite) and seems to be supported by MSVC (https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/finite-finitef?view=msvc-170). So any reasonable current compiler should have this. Thus, I would just assume that isfinite() is present, especially if there are difficulties with the old BuildTools/autotools macros.

@mandeluna
Copy link
Author

@LouHafer thanks for the update. Unfortunately the macros are still unable to resolve the symbol.

My configure output is here config.log

It does not seem that AC_CHECK_DECL and friends are able to find this symbol in the C context (I have no idea why, I can see the definition in math.h). The following preprocessor check does work, but I don't know how or whether it makes sense to incorporate into a macro (__has_builtin is available only after GCC 10, and I'm not sure about MSVC):

#if __has_builtin(__builtin_isfinite)
  printf("found isfinite()\n");
#else
  printf("did not find isfinite()\n");
#endif

@LouHafer
Copy link
Contributor

LouHafer commented Mar 8, 2022

@mandeluna --- Good grief! And thanks for a working test! The `only available after GCC 10' isn't a problem, the test will simply fail :-). I'll see what I can do, various other distractions to deal with today.

@tkralphs
Copy link
Member

tkralphs commented Mar 8, 2022

I ran into this issue while trying to install Symphony.

Just curious how you ran into this while installing SYMPHONY, which doesn't depend on DyLP. I guess that it can use DyLP as the underlying LP solver in principle, but I've never actually tried this.

@mandeluna
Copy link
Author

mandeluna commented Mar 8, 2022

Hi @tkralphs -- I was unfamiliar with the project, and followed the basic instructions on the Github page:

brew tap coin-or-tools/coinor
brew install coin-or-tools/coinor/symphony

It is building DyLP as a dependency, and everything stops with the error we have been discussing, excerpted below:

➜  brew install symphony
[... irrelevant housekeeping entries omitted]
Already downloaded: /Users/steve/Library/Caches/Homebrew/downloads/5149ab50cfff79d0cd4f9c9925a76f59c39acf86ded07a89451977f7aa0d58af--SYMPHONY-releases-5.6.18.tar.gz
==> Installing symphony from coin-or-tools/coinor
==> Installing dependencies for coin-or-tools/coinor/symphony: coin-or-tools/coinor/dylp
==> Installing coin-or-tools/coinor/symphony dependency: coin-or-tools/coinor/dylp
==> ./configure --prefix=/opt/homebrew/Cellar/dylp/1.10.4_1 --datadir=/opt/homebrew/Cellar/dylp/1.10.4_1/share/dylp --includedir=/opt/homebrew/C
==> make
Last 15 lines from /Users/steve/Library/Logs/Homebrew/dylp/02.make:
mkdir .libs/libDylp.lax
rm -fr .libs/libDylp.lax/libDylpStdLib.a
mkdir .libs/libDylp.lax/libDylpStdLib.a
Extracting /private/tmp/dylp-20220308-85818-s2s7eb/DyLP-releases-1.10.4/DyLP/src/Dylp/../DylpStdLib/.libs/libDylpStdLib.a
(cd .libs/libDylp.lax/libDylpStdLib.a && ar x /private/tmp/dylp-20220308-85818-s2s7eb/DyLP-releases-1.10.4/DyLP/src/Dylp/../DylpStdLib/.libs/libDylpStdLib.a)
clang -dynamiclib -single_module  -o .libs/libDylp.1.9.4.dylib  .libs/dy_consys_io.o .libs/dy_consys_mathutils.o .libs/dy_consys_scaling.o .libs/dy_consys_utils.o .libs/dy_basis.o .libs/dy_bound.o .libs/dy_cmdint.o .libs/dy_coldstart.o .libs/dy_conmgmt.o .libs/dy_dual.o .libs/dy_dualmultipivot.o .libs/dy_dualpivot.o .libs/dy_duenna.o .libs/dy_force.o .libs/dy_hotstart.o .libs/dylp.o .libs/dylp_io.o .libs/dylp_utils.o .libs/dy_options.o .libs/dy_penalty.o .libs/dy_pivreject.o .libs/dy_primal.o .libs/dy_primalmultipivot.o .libs/dy_primalpivot.o .libs/dy_rays.o .libs/dy_scaling.o .libs/dy_setup.o .libs/dy_solutions.o .libs/dy_statistics.o .libs/dy_tableau.o .libs/dy_varmgmt.o .libs/dy_warmstart.o .libs/glpinv.o .libs/glplib1.o .libs/glplib2.o .libs/glplib3.o .libs/glplib4.o .libs/glpluf.o .libs/dy_vector_utils.o  .libs/libDylp.lax/libDylpStdLib.a/dylib_bnfrdr.o .libs/libDylp.lax/libDylpStdLib.a/dylib_strrtns.o .libs/libDylp.lax/libDylpStdLib.a/dylib_errs.o .libs/libDylp.lax/libDylpStdLib.a/dylib_bnfrdrio.o .libs/libDylp.lax/libDylpStdLib.a/dylib_littab.o .libs/libDylp.lax/libDylpStdLib.a/dylib_binsrch.o .libs/libDylp.lax/libDylpStdLib.a/dylib_hash.o .libs/libDylp.lax/libDylpStdLib.a/dylib_io.o   -lm  -install_name  /opt/homebrew/Cellar/dylp/1.10.4_1/lib/libDylp.1.dylib -compatibility_version 11 -current_version 11.4
Undefined symbols for architecture arm64:
  "_unavailable", referenced from:
      _consys_create in dy_consys_utils.o
      _dy_ctlopt in dy_options.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [libDylp.la] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all-recursive] Error 1

If reporting this issue please do so at (not Homebrew/brew or Homebrew/core):
  https://github.com/coin-or-tools/homebrew-coinor/issues

@LouHafer
Copy link
Contributor

@mandeluna, I went in a slightly different direction and added __builtin_isfinite and __builtin_isnan to the front of the list of names that are checked by AC_CHECK_DECL in m4/ac_dylp_find_fp_funcs.m4. Works just dandy in my Fedora / GCC / C environment, finds the builtins in preference to the regular names. Apparently this __builtin_ business originated in GCC and has since propagated to other compilers. Pushed to master. Let me know if this does the trick on your M1 system.

The most curious thing (to me) about all this is that the check for isfinite fails but the check for isnan succeeds. Unfortunately, your configure doesn't include the failed program in config.log (except for once, at line 14902), so there's not as much information as I was hoping to get from it.

@mandeluna
Copy link
Author

Thanks @LouHafer.

I have confirmed that AC_CHECK_DECL can detect the presence of __has_builtin but not __builtin_isfinite, so unfortunately this fix doesn't resolve the issue on Mac OS X.

I've excerpted some of Apple's math.h macro definitions and their commentary regarding the inlining of these calls. I think the declarations are inlined under normal circumstances, and resolve neither to a function nor a macro. It is puzzling that isnan is resolved, as the only place I can find it is alongside the definition for isfinite.

#if (defined(__GNUC__) && 0 == __FINITE_MATH_ONLY__)
/*  These inline functions may fail to return expected results if unsafe
    math optimizations like those enabled by -ffast-math are turned on.
    Thus, (somewhat surprisingly) you only get the fast inline
    implementations if such compiler options are NOT enabled.  This is
    because the inline functions require the compiler to be adhering to
    the standard in order to work properly; -ffast-math, among other
    things, implies that NaNs don't happen, which allows the compiler to
    optimize away checks like x != x, which might lead to things like
    isnan(NaN) returning false.                                               
 
    Thus, if you compile with -ffast-math, actual function calls are
    generated for these utilities.                                            */
    
#define isfinite(x)                                                      \
    ( sizeof(x) == sizeof(float)  ? __inline_isfinitef((float)(x))       \
    : sizeof(x) == sizeof(double) ? __inline_isfinited((double)(x))      \
                                  : __inline_isfinitel((long double)(x)))

#define isinf(x)                                                         \
    ( sizeof(x) == sizeof(float)  ? __inline_isinff((float)(x))          \
    : sizeof(x) == sizeof(double) ? __inline_isinfd((double)(x))         \
                                  : __inline_isinfl((long double)(x)))

#define isnan(x)                                                         \
    ( sizeof(x) == sizeof(float)  ? __inline_isnanf((float)(x))          \
    : sizeof(x) == sizeof(double) ? __inline_isnand((double)(x))         \
                                  : __inline_isnanl((long double)(x)))
...
#else /* defined(__GNUC__) && 0 == __FINITE_MATH_ONLY__ */

/*  Implementations making function calls to fall back on when -ffast-math
    or similar is specified.  These are not available in iOS versions prior
    to 6.0.  If you need them, you must target that version or later.         */
    
#define isfinite(x)                                               \
    ( sizeof(x) == sizeof(float)  ? __isfinitef((float)(x))       \
    : sizeof(x) == sizeof(double) ? __isfinited((double)(x))      \
                                  : __isfinitel((long double)(x)))
    
#define isinf(x)                                                  \
    ( sizeof(x) == sizeof(float)  ? __isinff((float)(x))          \
    : sizeof(x) == sizeof(double) ? __isinfd((double)(x))         \
                                  : __isinfl((long double)(x)))
    
#define isnan(x)                                                  \
    ( sizeof(x) == sizeof(float)  ? __isnanf((float)(x))          \
    : sizeof(x) == sizeof(double) ? __isnand((double)(x))         \
                                  : __isnanl((long double)(x)))

@LouHafer
Copy link
Contributor

@mandeluna, thanks for checking and for sending the excerpt from math.h. Looking at the excerpt, I'm thinking the macro resolves at compile time, when sizeof(x) should be a compile-time constant. But it won't work without an actual parameter (sizeof(1.0) maybe, sizeof() would be an error). What bothers me at this point is that the doc'n for AC_CHECK_DECL says "This macro actually tests whether symbol is defined as a macro or can be used as an r-value". The definitions for isfinite and isnan look to me to be identical, but isnan works and isfinite doesn't. It's a mystery. There's something we're missing.

I'll try and incorporate your working test from a few days back into an autoconf macro.

@LouHafer
Copy link
Contributor

@mandeluna, I've attached two files (zipped), a revised configure script for dylp, and a revised copy of ac_dylp_find_fp_funcs.m4 so you can see what's happening in FIND_ISFINITE. I've added two more tests. 'Just try it' simply constructs a program that invokes isfinite and tries to compile it. 'Mandeluna' is your test, modified slightly so it'll fail to compile if the test with __has_builtin is false and (hopefully) compile without error if it's true. As before, works for me. See if it works for you. Here's how the output looks on my system

checking whether isfinite is declared... yes
configure: CHECK_DECL found isfinite.
configure: Just try it found isfinite.
configure: Mandeluna found isfinite.
configure: Using isfinite as isfinite().

Just replace dylp's configure with the one included here and give it a spin. Let me know what you see, please. Should avoid a lot of 'trying again' commits.

@mandeluna
Copy link
Author

Hi @LouHafer -- Very interesting indeed; this is very close to what I am seeing. The project builds without any additional tweaks and all 90 tests completed successfully.

configure: Checking for proper name for isfinite().
checking whether isfinite is declared... yes
configure: CHECK_DECL found isfinite.
configure: Just try it found isfinite.
configure: Mandeluna found isfinite.
configure: Using isfinite as isfinite().

@LouHafer
Copy link
Contributor

@mandeluna, very interesting indeed. Because that first line (CHECK_DECL found isfinite) is produced by the test that's been failing from the start. It's a mystery for sure. I'll clean up the messaging but leave the new tests in place. No harm in that. Thanks for your help! I'll close this issue once I've pulled the changes into the master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants