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

Win64 std::string error #407

Closed
visr opened this issue Feb 19, 2019 · 10 comments
Closed

Win64 std::string error #407

visr opened this issue Feb 19, 2019 · 10 comments

Comments

@visr
Copy link
Contributor

visr commented Feb 19, 2019

I ran into this a while ago in JuliaGeo/LibGEOS.jl#56, but so far I assumed it was library specific. Until @piever mentioned running into the same issue in piever/Sass.jl#10 yesterday.

To trigger (Win64 only), check out the linked branches of LibGEOS.jl or Sass.jl (both try to switch to a newer BinaryBuilder build). Then build LibGEOS | build Sass and run this code to trigger the error:

using LibGEOS; readgeom("POLYGON((0 0,1 0,1 1,0 0))")
# or
using Sass
filename = joinpath(Sass.examplefolder, "test.sass")
Sass.compile_file(filename; output_style = Sass.nested)

Both examples involve sending julia strings to the library. For LibGEOS it is telling that all earlier tests that send numbers and pointers to the library seem to work fine, it only fails here.

As @staticfloat suggested I ran both under gdb and posted the backtrace here: https://gist.github.com/visr/6472f9e9921e83f45eb26f9669749f33

Possibly relevant and similar lines from the backtrace posted here:

LibGEOS.jl

#8  0x000000006fcb3586 in libstdc++-6!_ZNSs6assignEPKcy () from /cygdrive/d/repo/julia/usr/bin/libstdc++-6.dll
#9  0x000000002beb4b3d in std::string::assign (__s=0x2848224 "C", this=0x2be5ec60) at /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/include/c++/4.8.5/bits/basic_string.h:1131
#10 std::string::operator= (__s=0x2848224 "C", this=0x2be5ec60) at /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/include/c++/4.8.5/bits/basic_string.h:555
#11 geos::io::CLocalizer::CLocalizer (this=0x2be5ec60) at CLocalizer.cpp:38

Sass.jl

#5  0x000007fefdad10c8 in msvcrt!free () from /cygdrive/c/WINDOWS/system32/msvcrt.dll
#6  0x000000002c14a1f3 in std::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string (this=0x2c11ea90, __in_chrg=<optimized out>)
    at /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/include/c++/4.8.5/bits/basic_string.h:539
#7  Sass::Context::Context (this=0x2aa9a690, c_ctx=...) at context.cpp:90
@visr
Copy link
Contributor Author

visr commented Mar 16, 2019

After reading JuliaPackaging/BinaryProvider.jl#152 I thought I'd try building and forcing to use the GCC7 build on Windows. I did this in JuliaGeo/LibGEOS.jl#56 and the std::string issue is gone, all tests pass.

@piever
Copy link

piever commented Mar 17, 2019

So in practice just this change here should fix it? Will update as soon as the building is done and I can see if AppVeyor passes.

@visr
Copy link
Contributor Author

visr commented Mar 17, 2019

Yes, and I used Gnimuc/Cxx.jl@cc9dd95 to make BinaryProvider get the GCC7 version, that may be needed as well.

@piever
Copy link

piever commented Mar 17, 2019

Yes, can confirm that this fixes my issue as well (in piever/Sass.jl#10) and the manual editing of the build.jl was needed for it to find the windows GCC7 download.

@staticfloat
Copy link
Member

Mmmm, you should note that while what you're doing works right now, it's really sub-optimal. By doing this:

dl_info = download_info[Windows(Sys.WORD_SIZE == 64 ? :x86_64 : :i686, compiler_abi=CompilerABI(:gcc7))]

You are making the guarantee that whatever version of Julia you're running has been compiled with gcc 7.X. I see that your download_info above only has :gcc7 binaries, so I suppose there's no getting around it, but I think it would be preferable to instead delete the compiler_abi=CompilerABI(:gcc7) kwarg within the download_info Dict declaration. In more complex setups (where, for instance, you have binaries that are compiled with :gcc7 and :gcc8, but you want to default to :gcc7 if the user is running :gcc4) your override would not work properly. I suggest removing the CompilerABI kwarg from the elements of your download_info that you want to be treated as the "default" elements.

@Gnimuc
Copy link
Contributor

Gnimuc commented Mar 18, 2019

I guess the optimal solution would be to fix this JuliaPackaging/BinaryProvider.jl#152 ? The released Julia for Windows x86_64 is compiled with gcc 7(6?), but the default compiler_abi that BP is looking for is gcc4, which causes ABI mismatch.

julia> using BinaryProvider

julia> platform_key_abi()
Windows(:x86_64, compiler_abi=CompilerABI(:gcc4, :cxx11))

julia> versioninfo()
Julia Version 1.1.0
Commit 80516ca202 (2019-01-21 21:24 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.1 (ORCJIT, skylake)

visr added a commit to JuliaGeo/LibGEOS.jl that referenced this issue Mar 29, 2019
Which has expanded GCC versions, and uses
the workaround for the failing GCC4 builds as
described in
JuliaPackaging/BinaryBuilder.jl#407 (comment)
visr added a commit to JuliaGeo/LibGEOS.jl that referenced this issue Mar 29, 2019
See comments in Travis and build.jl about the workarounds for GCC4 issues, these are based on
JuliaPackaging/BinaryBuilder.jl#407 (comment)
@visr
Copy link
Contributor Author

visr commented Mar 29, 2019

Ok I implemented the alternative workaround suggested in #407 (comment) in LibGEOS.jl, and it is working. So I did not build GCC4 for the problematic two platforms (this issue and x86_64-linux-musl), and removed the compiler_abi kwarg for the corresponding GCC7 builds, and then added ignore_platform=true to install() to make BinaryProvider accept that.

Does this mean that the downstream builders (GDALBuilder) that may not have GCC4 issues should also stick to GCC7 for those two platforms (so implement the exact same workaround?

Happy to have a working build now, thanks for all the help. Agree with @Gnimuc that it's worth considering JuliaPackaging/BinaryProvider.jl#152 instead.

@staticfloat
Copy link
Member

I'm glad you got it working with the GCC 7 build; there are other issues that cause us to choose GCC 4 as the default, so I don't think it's as simple as just choosing GCC7 as the default for Windows. It sounds like the problem you're running into is that your Julia uses cxx11 ABI strings, and because GCC 4 doesn't support that, the binaries are wrong. However, your Julia is probably actually a GCC7-built Julia, it's probably a GCC6-built Julia, so if you were to have a package that contained both fortran code and C++ code, neither a GCC 7 nor a GCC 4 build would work.

As it stands, our GCC "versioning" is essentially locked to libgfortran version; GCC 4 is the oldest release we support with libgfortran3, GCC 7 is the oldest with libgfortran4, and GCC8 is the oldest with libgfortran5. We need these "oldest" versions so that users who compiled Julia against libgfortranX can get a binary that links against the correct version, but the "oldest" release will be the most compatible so as to not use features that may or may not exist on the target user's machine.

Unfortunately, as you can see in this issue, libgfortran compatibility is not the only thing we need to worry about. We also need to match cxx11 string ABI. This is usually not a problem, as users rarely mess with the defaults (and this only strikes when calling a C++ object that passes actual std::string values to/from Julia), but the default changed between GCC 4 and GCC 5.

I think the "proper" fix here is that we need to start being stricter on tracking GCC and CXX ABI compatibility. We need to introduce a GCC 5 shard that is still as old as possible (to maximize compatibility) while supporting the cxx11 ABI. We would then add an audit pass that looks for affected datatypes within binaries (similarly to how we detect linkage against libgfortran and alert the user that they should call exapnd_gcc_versions() on their platforms to build GCC version-specific tarballs) and alert the user that they need to worry about this. We can then automatically choose the proper tarball by tagging it as either cxx03 or cxx11. This support already exists, it's just not used at the moment.

@Gnimuc
Copy link
Contributor

Gnimuc commented Mar 30, 2019

@staticfloat thanks for the thorough explanation. IIUC, the underlying reason is that the released Julia binary for Windows is shipped with libgfortran3(gcc4+ compatible), but libgfortran4(gcc7+ compatible) is used for both Linux and macOS. Is there any particular reason for doing this?

Gnimuc added a commit to JuliaInterop/Clang.jl that referenced this issue Mar 30, 2019
Gnimuc added a commit to JuliaInterop/Clang.jl that referenced this issue Mar 30, 2019
@staticfloat
Copy link
Member

We now have the ability to build C++ objects with multiple C++ string ABI backends, and should be choosing the appropriate binary automatically. Please open new issues if you have new issues, and if you have a builder that hasn't been updated to use the new BinaryBuilder infrastructure, head on over to Yggdrasil and open an issue/pull request!

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