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

Use versioned libblastrampoline and GMP #47676

Merged
merged 6 commits into from
Jan 26, 2023

Conversation

gbaraldi
Copy link
Member

Basically the title.

@gbaraldi gbaraldi requested a review from staticfloat November 22, 2022 22:17
@brenhinkeller brenhinkeller added the external dependencies Involves LLVM, OpenBLAS, or other linked libraries label Nov 22, 2022
@giordano giordano added the JLLs label Nov 23, 2022
@staticfloat
Copy link
Member

Very interesting; both win32 and win64 died with approximately the same error. Any ideas as to why?

@staticfloat
Copy link
Member

staticfloat commented Nov 28, 2022

It's minimally reproducible with the following:

julia> using LinearAlgebra, SparseArrays
       using SparseArrays: fixed
       b = sprandn(10, 10, 0.99) + I
       a = fixed(b)
       lu(a)

Please submit a bug report with steps to reproduce this fault, and any error messages that follow (in their entirety). Thanks.
Exception: EXCEPTION_ACCESS_VIOLATION at 0x0 -- unknown function (ip: 0000000000000000)
in expression starting at REPL[6]:5
unknown function (ip: 0000000000000000)
Allocations: 1954934 (Pool: 1953138; Big: 1796); GC: 3

@gbaraldi
Copy link
Member Author

I imagine it's the blas trampoline change, because the GMP change works in the mimalloc PR.

@giordano
Copy link
Contributor

It's minimally reproducible with the following:

JuliaLinearAlgebra/libblastrampoline#89

@staticfloat
Copy link
Member

That looks quite probable:

julia> println.(filter(x -> occursin("blast", x), Libdl.dllist()))
C:\Users\Administrator\AppData\Local\Temp\julia-0f526b01f8\bin\libblastrampoline-5.dll
C:\Users\Administrator\AppData\Local\Temp\julia-0f526b01f8\bin\libblastrampoline.dll

@giordano
Copy link
Contributor

So there is still something pulling in the wrong library.

@staticfloat
Copy link
Member

It's SuiteSparse:

$ for f in *.dll; do if [[ -n $(objdump.exe -p $f | grep "libblastrampoline.dll") ]]; then echo $f; fi; done
libblastrampoline.dll
libcholmod.dll
libspqr.dll
libumfpack.dll

@vchuravy vchuravy added the backport 1.9 Change should be backported to release-1.9 label Nov 28, 2022
version() = VersionNumber(unsafe_string(unsafe_load(cglobal((:__gmp_version, :libgmp), Ptr{Cchar}))))
bits_per_limb() = Int(unsafe_load(cglobal((:__gmp_bits_per_limb, :libgmp), Cint)))
if Sys.iswindows()
const libgmp = "libgmp-10.dll"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pull in the version from the buildsystem? See base/Makefile the build_h.jl generation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gbaraldi bump

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, maybe we can pull the version from the jll from inside the makefile? It's kind of annoying not being able to depend on the jll proper here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, is there a reason for BigNums being part of base instead of being stdlibs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're used in Base, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They’re needed quite early because we can parse literal bignums

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get version numbers from the makefile system, then embed those into build_h.jl. I suggest we try some awk trickery to figure out the so major version number.

@vchuravy
Copy link
Member

One minor nit we should move the so version into a place where packagers can override them, and it would be great to just have a single place to change them

@giordano
Copy link
Contributor

SuiteSparse is still failing on Windows despite the update to the new build 😕

@giordano
Copy link
Contributor

giordano commented Dec 29, 2022

sandbox:${WORKSPACE} # for f in bin/*.dll; do if [[ -n $(${target}-objdump -p $f | grep "libblastrampoline.dll") ]]; then echo $f; fi; done
bin/libblastrampoline.dll
sandbox:${WORKSPACE} # for f in bin/*.dll; do if [[ -n $(${target}-objdump -p $f | grep "libblastrampoline-5.dll") ]]; then echo $f; fi; done
bin/libblastrampoline-5.dll
sandbox:${WORKSPACE} # for f in bin/*.dll; do if [[ -n $(${target}-objdump -p $f | grep "libblastrampoline-5-2-0.dll") ]]; then echo $f; fi; done
bin/libblastrampoline-5-2-0.dll
bin/libblastrampoline-5.dll
bin/libblastrampoline.dll
bin/libcholmod.dll
bin/libspqr.dll
bin/libumfpack.dll

So now we're linking to libblastrampoline-5-2-0.dll, which is even worse than libblastrampoline.dll

@giordano
Copy link
Contributor

All Windows tests are green 🥳

@vchuravy vchuravy requested a review from staticfloat January 26, 2023 22:01
@staticfloat staticfloat merged commit 335cd5e into JuliaLang:master Jan 26, 2023
KristofferC pushed a commit that referenced this pull request Feb 1, 2023
Use versioned libblastrampoline and GMP

(cherry picked from commit 335cd5e)
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external dependencies Involves LLVM, OpenBLAS, or other linked libraries JLLs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants