marzer / tomlplusplus

Header-only TOML config file parser and serializer for C++17.
https://marzer.github.io/tomlplusplus/
MIT License
1.58k stars 152 forks source link

Cannot build with Intel MKL include dirs. #202

Closed iago-lito closed 1 year ago

iago-lito commented 1 year ago

Environment

toml++ version and/or commit hash:

Submodule : TOML++ v3.3.0 (c635f21)

Compiler:

$ c++ --version
c++ (GCC) 13.2.1 20230801

C++ standard mode:

set(CMAKE_CXX_STANDARD 17)

Target arch:

$ uname -a
Linux copak 6.4.12-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 24 Aug 2023 00:38:14 +0000 x86_64 GNU/Linux

Library configuration overrides:
none

Relevant compilation flags: After having installed intel-oneapi-mkl:

-I/opt/intel/oneapi/compiler/latest/linux/compiler/include

Describe the bug

The library compiles fine, until I add the above flag (because the project otherwise uses MKL), then I get:

In file included from ./extern/toml++/include/toml++/toml.h:36,
                 from ./main.cpp:1:
./extern/toml++/include/toml++/impl/forward_declarations.h:38:15: error: ‘FLT_RADIX’ was not declared in this scope
   38 | static_assert(FLT_RADIX == 2, TOML_ENV_MESSAGE);
      |               ^~~~~~~~~

Steps to reproduce (or a small repro code sample)

main.cpp

#include <toml++/toml.h>
int main(){};

CMakeLists.txt

cmake_minimum_required(VERSION 3.20..3.22)

project(TomlAndMkl LANGUAGES CXX)

set(CMAKE_CXX_STANDARD 17)

add_library(toml++ INTERFACE)
target_include_directories(toml++ INTERFACE "./extern/toml++/include")

add_executable(main main.cpp)

#-------------------------------------------------------------------------------
# Works if I remove this snippet.
find_package(PkgConfig REQUIRED)
pkg_search_module(MKL mkl-dynamic-lp64-iomp)
target_include_directories(main PRIVATE ${MKL_INCLUDE_DIRS})
#-------------------------------------------------------------------------------

target_link_libraries(main PRIVATE toml++) 

I'm not exactly sure what the error deeply means here. FWIU the FLT_RADIX macro is (re-)defined within intel/oneapi/compiler/latest/linux/compiler/include/float.h:75 to:

#undef  FLT_RADIX
#define FLT_RADIX   2

but I'm not confident what it means or why this would result in "undeclared FLT_RADIX". I'm happy to learn more ^ ^"

marzer commented 1 year ago

Hmnmn, that is weird! If float.h is undefining and redefining FLT_RADIX immediately, nothing should be amiss from toml++'s perspective. I suppose the MKL package does something weird with FLT_RADIX elsewhere and there's some include-order shenanigans going on with the CMake config. I have no idea what to do about it, though. This seems like a bug in the MKL headers.

marzer commented 1 year ago

Can you try replacing toml++'s use of FLT_RADIX with std::numeric_limits<double>::radix? i.e. change this line in toml++/impl/forward_declarations.hpp:

static_assert(FLT_RADIX == 2, TOML_ENV_MESSAGE);

to this:

static_assert(std::numeric_limits<double>::radix == 2, TOML_ENV_MESSAGE);
marzer commented 1 year ago

If that doesn't work, you can #define TOML_DISABLE_ENVIRONMENT_CHECKS somewhere before you include toml++ and it should work OK. The float radix is only used to check that the local environment is compatible with the TOML spec, and isn't actually used anywhere else.

iago-lito commented 1 year ago

Replacing the macro invocation by std::*::radix does fix the problem. And indeed I should maybe report this to intel-mkl. It seems that they broke something ^ ^" Thank you for swift support, and feel free to close then. I'll just need to figure out where to find intel now ^ ^"

iago-lito commented 1 year ago

you can #define TOML_DISABLE_ENVIRONMENT_CHECKS

I think I'll go for some more targetted hack instead:

#ifndef FLT_RADIX
#define FLT_RADIX std::numeric_limits<float>::radix
#endif

.. but I just want to get rid of one confusion first: do you expect FLT_RADIX to be std::numeric_limits<float>::radix or std::numeric_limits<double>::radix ? (because it seems that DBL_RADIX also exists)

marzer commented 1 year ago

I think I'll go for some more targetted hack instead [...]

I'd caution against that solution; the standard specifies std::numeric_limits<(float|double|long double)>::radix in terms of FLT_RADIX (ref), so you might end up causing compiler errors elsewhere. I'll add a workaround to the toml++ code to detect FLT_RADIX shenanigans to avoid this in future.

but I just want to get rid of one confusion first [...]

Either would be fine since they should be the same, but floats in TOML++ are (at least) 64-bit so it uses double everywhere internally. (double isn't necessarily going to be 64-bit on all platforms, of course, but the other static asserts check for that.)

iago-lito commented 1 year ago

Wop, thank you for spotting the snag! ^ ^"

iago-lito commented 1 year ago

FWIW it seems that the error does not occur.. provided you opt into using Intel's proprietary compiler icpx :\

marzer commented 1 year ago

Ah, right, so I guess MKL assumes you're using that compiler and does #undef FLT_RADIX because it knows that the compiler has some special behaviour in that area.

That's... unfortunate 😅