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

Remove dependency on openlibm #42299

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Remove dependency on openlibm #42299

wants to merge 1 commit into from

Conversation

ViralBShah
Copy link
Member

Fix #26434

@ViralBShah
Copy link
Member Author

ViralBShah commented Sep 18, 2021

How do I prevent the system image building step from trying to look for a libm?

    LINK usr/lib/libjulia-internal.1.8.dylib
    LINK usr/lib/libjulia-internal.1.dylib
    LINK usr/lib/libjulia-internal.dylib
    JULIA usr/lib/julia/corecompiler.ji
ERROR: Unable to load dependent library /Users/viral/julia/usr/lib/julia/libm.dylib
Message:dlopen(/Users/viral/julia/usr/lib/julia/libm.dylib, 10): image not found
make[1]: *** [/Users/viral/julia/usr/lib/julia/corecompiler.ji] Error 1
make: *** [julia-sysimg-ji] Error 2

@DilumAluthge DilumAluthge added needs nanosoldier run This PR should have benchmarks run on it needs pkgeval Tests for all registered packages should be run with this change labels Sep 18, 2021
@ViralBShah
Copy link
Member Author

SpecialFunctions.jl needs this - that's the main big package, without which everything that depends on it will fail. But we will have it depend directly on openlibm.

@ararslan
Copy link
Member

SpecialFunctions.jl needs this - that's the main big package, without which everything that depends on it will fail. But we will have it depend directly on openlibm.

JuliaMath/SpecialFunctions.jl#344

@ViralBShah ViralBShah changed the title Remove openlibm and use system libm everywhere Use system libm and prepare to remove openlibm Sep 23, 2021
@ViralBShah ViralBShah added the excision Removal of code from Base or the repository label Sep 23, 2021
base/Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Make.inc Outdated Show resolved Hide resolved
@ararslan
Copy link
Member

Looks like tests are failing because this is still pulling in OpenLibm_jll as a stdlib

@ViralBShah
Copy link
Member Author

ViralBShah commented Sep 24, 2021

Linux32 seems to have a ton of issues when using the system libm. So we do need to remove all calls to libm (#26434) if we want to get rid of this dependency. Luckily it's just a couple of functions, and only a couple of hundred lines of code. Of course, there's no urgency - and this PR shows what needs to happen when we are ready to excise.

@ViralBShah ViralBShah marked this pull request as draft September 24, 2021 12:36
@ViralBShah ViralBShah mentioned this pull request Sep 24, 2021
17 tasks
@ViralBShah
Copy link
Member Author

Is the failure of the package_win32/64 builds an issue because of this PR? Or are they otherwise failing elsewhere?

@staticfloat
Copy link
Member

Yeah, I think it is. My guess is that LLVM.dll is expecting to find libm or something like that?

@ViralBShah
Copy link
Member Author

I thought LLVM will find the system libm. Do we link LLVM to openlibm?

@ViralBShah
Copy link
Member Author

It builds fine on linux for me, but not on my mac

➜  julia git:(vs/rm-openlibm) make
    JULIA usr/lib/julia/sys-o.a
/bin/sh: line 1: 13171 Segmentation fault: 11  JULIA_BINDIR=/Users/viral/julia/usr/bin WINEPATH="/Users/viral/julia/usr/bin;$WINEPATH" /Users/viral/julia/usr/bin/julia -O3 -C "native" --output-o /Users/viral/julia/usr/lib/julia/sys-o.a.tmp --startup-file=no --warn-overwrite=yes --sysimage /Users/viral/julia/usr/lib/julia/sys.ji /Users/viral/julia/contrib/generate_precompile.jl 1
*** This error is usually fixed by running `make clean`. If the error persists, try `make cleanall`. ***
make[1]: *** [/Users/viral/julia/usr/lib/julia/sys-o.a] Error 1
make: *** [julia-sysimg-release] Error 2

My suspicion is that this has something to do with llvm not finding libm and perhaps a -lSystem needs to be thrown in somewhere. Pinging @vchuravy for any suggestions.

@vtjnash
Copy link
Member

vtjnash commented Oct 12, 2021

There is no libm on many systems (including macOS)

$ ls -lh libm.tbd 
lrwxr-xr-x  1 root  wheel    13B Sep 21 16:12 libm.tbd -> libSystem.tbd

@ViralBShah
Copy link
Member Author

Right - but I don't know where the linking to libm is happening and I need to intervene for mac.

@staticfloat
Copy link
Member

I think if you link with g++ it automatically introduces a dependency on libm, is that right?

@ViralBShah
Copy link
Member Author

But wouldn't that do the right thing on Mac and find libSystem?

@ViralBShah ViralBShah marked this pull request as ready for review October 16, 2021 15:13
@ViralBShah
Copy link
Member Author

One of the last steps here is to remove Openlibm_jll from stdlib. I see mentions in Pkg and TOML. It seems that those packages need to be bumped first and we can then remove Openlibm_jll completely.

@ViralBShah ViralBShah changed the title Use system libm and prepare to remove openlibm Remove dependency on openlibm Oct 16, 2021
@ViralBShah
Copy link
Member Author

fma remains as the last issue now. How best to resolve this?

https://build.julialang.org/#/builders/90/builds/4890/steps/8/logs/stdio

@gbaraldi
Copy link
Member

gbaraldi commented Feb 26, 2023

It seems there's still a couple libm functions we call on windows, and I imagine other places as well.
rintf and trunc
Not sure why that's not a problem on other OSs however.

I would expect that maybe 32bit windows would need those libm functions, but x86 has native instructions for that since the first Core 2 CPUs.

@ViralBShah
Copy link
Member Author

The windows builds timed out.

@gbaraldi
Copy link
Member

gbaraldi commented Mar 3, 2023

Yeah, I took a deeper look, the missing functions we were seeing are supposed to be in msvcrt, but I think we are linking to some version of it that does not have the symbols. I think it's libmsvcrt.a

@ViralBShah
Copy link
Member Author

I don't think you can link in msvcrt.a statically, since that is a Microsoft library and we don't have license to redistribute.

@ViralBShah
Copy link
Member Author

@gbaraldi See some of @inkydragon 's notes in this thread starting at #42299 (comment)

@gbaraldi
Copy link
Member

gbaraldi commented Mar 4, 2023

We already link it for pkgimgs, but we link the one from mingw. The lib I was talking there is also from mingw. Not sure if being from mingw means it's legal or not, but that's what we do.

@ViralBShah
Copy link
Member Author

Ok, that sounds fine then. I had done some very cursory scanning on stack overflow.

@ViralBShah
Copy link
Member Author

-lucrt also doesn't help.

@gbaraldi
Copy link
Member

gbaraldi commented Mar 4, 2023

It needs to be added to the CSL like the normal libmsvcrt, I will try and do that tomorrow.

@gbaraldi
Copy link
Member

gbaraldi commented Mar 4, 2023

Also we should probably look more strongly into making julia with ucrt a thing. It's a much more reasonable way of doing things. By reasonable I mean, you can rely on the system to have a decent libc.

@gbaraldi
Copy link
Member

gbaraldi commented Mar 5, 2023

@vchuravy conviced me that shipping a versione mscvrt is a bad idea, which leaves the question. Why are those symbols needed at all. On linux and on macos I don't see a reference to them on any image at all.
I see the reason now, the instructions needed for those functions are in SSE4_1 which is a bit later than the baseline generic

@ViralBShah
Copy link
Member Author

Do we have a way to install openlibm only on win32, and nowhere else?

@ViralBShah
Copy link
Member Author

@PallHaraldsson
Copy link
Contributor

UCRT is available on Windows 10 and later (what we support), can be download for older, I believe here:

https://support.microsoft.com/en-us/topic/update-for-universal-c-runtime-in-windows-c0514201-7fe6-95a3-b0a5-287930f3560c

I'm not on Windows, and don't know much about this, but I think the problem may be that you're [cross-]compiling with MinGW32, i.e. not on Windows and not thus with UCRT available?

https://learn.microsoft.com/en-us/cpp/c-runtime-library/crt-library-features?view=msvc-170

The ISO C standard library is part of the C++ standard library. [..] the CRT was refactored into new binaries. The Universal CRT (UCRT) contains the functions and globals exported by the standard C99 CRT library. The UCRT is now a Windows component, and ships as part of Windows 10 and later versions. The static library, DLL import library, and header files for the UCRT are now found in the Windows SDK. When you install Visual C++, Visual Studio setup installs the subset of the Windows SDK required to use the UCRT. [..]
You can redistribute it using vcredist for supported versions of Windows other than Windows 10 or later.

I added the bold. Are you trying to statically link UCRT with Julia? It's possible using libucrt.lib though unclear if allowed (to redistribute), but that seems ok, since you should use "ucrt.lib ucrtbase.dll DLL import library for the UCRT."?

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 8, 2023

Is the problem only 32-bit and even just win32? Can we then just NOT support win32, in 1.12/master (at least for now with binaries), or as suggested to "install openlibm only on win32, and nowhere else"?

I didn't look more into "Cannot find -lucrt" but that's another workaround? I do not want to waste more time on that if the above is ok.

I understand we have replaced ALL of openlibm already with pure Julia, but another question on that, the replacement is currently in Base? Can all of it be moved to (upgradeable) stdlib Julialibm (eventually), and it excised from Base (might need 2.0, or not with juliax)? For now in 1.11 not do that, just get rid of openlibm. It could still be kept in 1.10 that will become LTS, in case people want to support win32 for a long time that way.

There just doesn't seem any point to support win32 otherwise for the future, as opposed to 32-bit for Arm, and maybe some other microcontrollers.

@PallHaraldsson
Copy link
Contributor

Any progress on this, should we just remove it and not promise it for 1.10 (not release binaries for win32 until we figure it out)?

@ViralBShah
Copy link
Member Author

It is not promised for 1.10. And no, we should not stop win32 binaries for this reason.

@ViralBShah ViralBShah force-pushed the vs/rm-openlibm branch 3 times, most recently from 18799f8 to 489c700 Compare February 18, 2024 17:38
@ViralBShah
Copy link
Member Author

Windows failure:

2024-02-18 12:54:48 EST | lld: error: undefined symbol: rint
  | 2024-02-18 12:54:48 EST | >>> referenced by float.jl:465
  | 2024-02-18 12:54:48 EST | >>>               jl_C440.tmp(text#2.o):(julia_tree_1380)
  | 2024-02-18 12:54:48 EST | >>> referenced by float.jl:465
  | 2024-02-18 12:54:48 EST | >>>               jl_C440.tmp(text#2.o):(julia_tree_1380)
  | 2024-02-18 12:54:48 EST | >>> referenced by float.jl:465
  | 2024-02-18 12:54:48 EST | >>>               jl_C440.tmp(text#2.o):(julia_flat_1100)
  | 2024-02-18 12:54:48 EST | >>> referenced 1 more times
  | 2024-02-18 12:54:48 EST | ERROR: failed process: Process(setenv(`'C:\workdir\usr\tools\lld.exe' -flavor gnu -m i386pep -Bdynamic --enable-auto-image-base --allow-multiple-definition '' -shared -o 'C:/workdir/usr/share/julia\compiled\v1.12\Profile\jl_C441.tmp' --whole-archive 'C:/workdir/usr/share/julia\compiled\v1.12\Profile\jl_C440.tmp' --no-whole-archive '-LC:\workdir\usr\lib\julia' '-LC:\workdir\usr\bin' -ljulia -ljulia-internal -lssp -lgcc_s -lgcc -lmsvcrt`,["MFLAGS=-w -j8 -Otarget --jobserver-auth=3,4", "PATH=C:\\workdir\\usr\\lib\\julia;C:\\workdir\\usr\\bin;C:\\buildkite-agent\\bin;C:\\msys64\\mingw64\\bin;C:\\msys64\\usr\\bin;C:\\Windows\\system32;C:\\Windows;C:\\Windows\\System32\\Wbem;C:\\Windows\\System32\\WindowsPowerShell\\v1.0;C:\\Windows\\System32\\OpenSSH;C:\\Users\\ContainerAdministrator\\AppData\\Local\\Microsoft\\WindowsApps;C:\\Program Files (x86)\\Windows Kits\\10\\bin\\10.0.22621.0\\x64;C:\\Program Files\\Amazon\\AWSCLIV2", "BUILDKITE_BUILD_CREATOR_TEAMS=committers-at-julialang-slash-committers", "BUILDROOT=C:/workdir", "MAKEFLAGS=w -j8 -Otarget --jobserver-auth=3,4 -- VERBOSE=1 TAGGED_RELEASE_BANNER=Official\\ https://julialang.org/\\ release JULIA_CPU_TARGET=generic;sandybridge,-xsaveopt,clone_all;haswell,-rdrnd,base(1)", "BUILDKITE_PROJECT_PROVIDER=github", "MAKEOVERRIDES=\${-*-command-variables-*-}", "UPLOAD_FILENAME=julia-028992f3ad-windows-x86_64", "ARCH=x86_64", "LONG_COMMIT=028992f3adaeaafedeb715272c1216e9dfc79ab3"  …  "PKG_CONFIG_PATH=C:/workdir/usr/lib/pkgconfig", "JULIA_LOAD_PATH=@stdlib", "BUILDKITE_PULL_REQUEST=42299", "EXPECTED_WORD_SIZE=64", "SYSTEMDRIVE=C:", "JULIA_CPU_TARGET=generic;sandybridge,-xsaveopt,clone_all;haswell,-rdrnd,base(1)", "TAR_VERSION=028992f3ad", "PROCESSOR_ARCHITECTURE=AMD64", "OPENBLAS_MAIN_FREE=1", "BUILDKITE_AGENT_META_DATA_CPUSET_LIMITED=true"]), ProcessExited(1)) [1]
  | 2024-02-18 12:54:48 EST

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Feb 26, 2024

Languages, at least Go, have dropped support for 32-bit Windows, and it's very unclear we shouldn't too, for at least 1.12.

Thus we can drop openlibm already, only 32-bit windows is stopping that, we will still support 32-bit Arm, and 32-bit Windows on LTS. We would revisit this and add openlibm and/or 32-bit Windows back, or even go further, and drop it from 1.11, but for now just on master.

@ViralBShah
Copy link
Member Author

The windows issues are on both 32 and 64-bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies excision Removal of code from Base or the repository needs nanosoldier run This PR should have benchmarks run on it needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove openlibm