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

Polymake.jl doesn't work if user has too new C++ library #251

Closed
fingolfin opened this issue Apr 10, 2020 · 10 comments
Closed

Polymake.jl doesn't work if user has too new C++ library #251

fingolfin opened this issue Apr 10, 2020 · 10 comments

Comments

@fingolfin
Copy link
Member

The following was reported by Meinolf Geck on the oscar mailing list, but I've seen it elsewhere before, too: when using Julia 1.3.1 under Ubuntu 19.10, install Oscar seems to work, but loading it doesn't. This can be reduced to Polymake.jl not loading:

julia> using Oscar
[ Info: Precompiling Oscar [f1435218-dba5-11e9-1e4d-f1a5fab5fc13]
ERROR: LoadError: could not load library "/home/geck/.julia/packages/Polymake/kIMyn/src/../deps/src/libpolymake.so"
/usr/local/lib/julia-1.3.1/bin/../lib/julia/libstdc++.so.6: version `GLIBCXX_3.4.26' not found (required by /home/geck/.julia/packages/Polymake/kIMyn/src/../deps/src/libpolymake.so)
Stacktrace:
...

Here's my understanding of the issue: Julia includes a copy of a the standard C++ library (which is fine; and it has to, as the user might not have it, or might have a version that's too old). On your computer, there is a newer version of that library; now when Polymake.jl is installed (which involves CxxWrap.jl, and compiling C++ code), the compiler generates output which assumes this newer version of the C++ library will be loaded. But in reality, when run under Julia, it always loads the older version of the C++ library shipped with Julia. As a result, the freshly compiled code fails to load.

So, how can we fix this? One view would be that Polymake.jl's build system should explicitly use the same C++ lib as Julia does. Perhaps also CxxWrap needs a corresponding change (I am not sure whether other packages using CxxWrap are affected?).

Telling users to remove the Julia copy of libstdc++ IMHO is not a viable solution. But perhaps you have some other ideas or already a fix?

The issue also exists in Julia 1.4 and on other Linux distributions; it's really just a matter of how old the C++ library is.

@fingolfin
Copy link
Member Author

I should point out that @fieker and me also did run into this before; I should have filed an issue back then but apparently forgot, sorry about that :-(.

I also just checked whether this affects Singular.jl as well, but that doesn't seem to be the case.

@kalmarek
Copy link
Contributor

Thanks for reporting;
just for reference: @rbehrends opened this #250, but the issue is exactly the same.

there is some movement in julia itself: there is
https://github.com/JuliaBinaryWrappers/CompilerSupportLibraries_jll.jl
and
JuliaLang/julia#33973

obviously @benlorenz is the person to assess if linking against julia-shipped libstdc++ is reasonable workaround, until we can use upstream solution

@fingolfin
Copy link
Member Author

Yeah we need a workaround on our side, as Julia 1.3.1 and 1.4 are both broken in this regard, and I am rather reluctant to accept the idea that we ought to tell DFG referees to delete Julia's libstdc++.so

Thanks for the pointers to the Julia issue. I am unconvinced by their plan (and commented there accordingly), but I may very well simply have misunderstood it or not understood the full picture.

Based on data from https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html the libstdc++ bundled with Julia is GLIBCXX_3.4.24 which is from GCC 7.2, while the GLIBCXX_3.4.26 symbol is from GCC 9.0

@fingolfin
Copy link
Member Author

So let's see what is need from libc++ 3.4.26, if anything:

$ strings ~/.julia/packages/Polymake/kIMyn/src/../deps/src/libpolymake.so | fgrep GLIBCXX_3.4.26 | c++filt
GLIBCXX_3.4.26
std::__cxx11::basic_ostringstream<char, std::char_traits<char>, std::allocator<char> >::basic_ostringstream()@@GLIBCXX_3.4.26

Luckily there is just a single use of ostringstream in Polymake.jl:

template <typename T>
std::string show_small_object(const T& obj, bool print_typename = true)
{
    std::ostringstream buffer;
    auto               wrapped_buffer = wrap(buffer);
    if (print_typename) {
        wrapped_buffer << polymake::legible_typename(typeid(obj)) << pm::endl;
    }
    wrapped_buffer << obj;
    return buffer.str();
}

Replacing the body of this function with return "fixme"; solves the issue!

Of course that's not an acceptable fix, but I am sure we can find a better workaround.

Oh and background of this? My guess is that this is because C++17 added support for ostringstream << nullptr... yay. What a great reason to break ABI compatibility sigh

@kalmarek
Copy link
Contributor

Cool, thanks for digging into this; so the question is: can we obtain a string from Polymake object without using std::ostringstream?

Or maybe we could delegate this to julia show?
it's almost for free for small objects <:AbstractArrays, but we'd have to write something for other small types: Integers, Rationals, Sets, Polynomials, maybe something better for sparse stuff...
opinions? @saschatimme @alexej-jordan

@fingolfin
Copy link
Member Author

I note this in Polymake.jl:

CMakeLists.txt:12:SET( CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} -std=c++14 ${polymake_cflags}" )

But that -std=c++14 has no effect, as CxxWrap adds -std=gnu++17 after it.

And in C++17, the only addition to basic_ostream I can spot in libstdc++ is in this (however, I may very well have missed something; but in the end, I guess it doesn't matter anyway):
https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/src/c%2B%2B17/ostream-inst.cc

As far as I can tell, Polymake's "wrap" only takes std::basic_ostream, so we simply can't use it there, hrm. So next idea would be to move that code into a separate C++ file that you somehow compile as C++14 only (so it can't use CxxWrap headers). But show_small_object heavily uses templates, so I can't think of a way to pull that off :-(

@fingolfin
Copy link
Member Author

@kalmarek that sounds good -- I think I can't help from here on as I know next to nothing about the Polymake code, so I'd just be a pointless distraction to you guys. I am rooting for you, though :-).

@benlorenz
Copy link
Member

First, I don't think this has anything to do with C++17. GCC and libstdc++ have (almost) full C++17 since version 7, see https://en.cppreference.com/w/cpp/compiler_support.

Second, delegating to julia is not really possible, as one of the main feature of this is to be able to print types that are not interfaced yet, and for that julia would need to understand the memory layout of the polymake objects.

Oh and background of this? My guess is that this is because C++17 added support for ostringstream << nullptr... yay. What a great reason to break ABI compatibility sigh

The error is a friendly way to tell us that the libstdc++ we are using is too old, instead of obscure crashes. There is all this symbol versioning in place which is helping quite a lot.
And what we are looking at here would be reverse ABI compatibility which is really not something anyone claims or even tries. libstdc++ is compatible with all old versions which is really nice to have, otherwise just using a newer one would even work at all.

But we might be able to fight this horrible mess that julia brings along (with tons of sometimes outdated bundled libraries; this is not the first similar issue that I encountered....) by throwing in even more bundled libraries: I will try to pull in the CompilerSupportLibraries_jll in legacy mode as another dependency. This contains libstdc++.so.6.0.26 which will then be dlopened by the deps.jl file, I will give it a try.
Of course this might bring new errors to other systems we dont know about yet...

benlorenz added a commit that referenced this issue Apr 10, 2020
also add patchelf to adjust the DT_NEEDED, otherwise dlopen will
use the old libstdc++ that is already in memory even when the new
one is dlopen'ed manually.

(all of the above only for Sys.islinux())
@benlorenz
Copy link
Member

There is in fact a really surprisingly easy fix, see the updated #252, and the commit that caused this is probably the following
gcc-mirror/gcc@8cffd3e3 , previously the default constructor relied on a defaulted argument, now there is a non-explicit default constructor without arguments.
And this commit was triggered by P0935R0

kalmarek added a commit that referenced this issue Apr 11, 2020
fix #251: adjust ostringstream constructor for recent libstdc++
@kalmarek
Copy link
Contributor

this should be fixed by #252

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

3 participants