The Wayback Machine - https://web.archive.org/web/20230106014802/https://github.com/JuliaLang/julia/pull/42299
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

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Remove dependency on openlibm #42299

wants to merge 21 commits into from

Conversation

ViralBShah
Copy link
Sponsor Member

@ViralBShah ViralBShah commented Sep 18, 2021

Fix #26434

@ViralBShah
Copy link
Sponsor 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
Sponsor Member Author

ViralBShah commented Sep 18, 2021

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

ararslan commented Sep 20, 2021

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 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

ararslan commented Sep 23, 2021

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

@ViralBShah
Copy link
Sponsor 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 Sep 24, 2021
@ViralBShah ViralBShah mentioned this pull request Sep 24, 2021
17 tasks
@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Sep 26, 2021

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

@staticfloat
Copy link
Sponsor Member

staticfloat commented Sep 27, 2021

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

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Sep 27, 2021

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

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Oct 10, 2021

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
Sponsor 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
Sponsor Member Author

ViralBShah commented Oct 12, 2021

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

@staticfloat
Copy link
Sponsor Member

staticfloat commented Oct 16, 2021

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

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Oct 16, 2021

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

@ViralBShah ViralBShah marked this pull request as ready for review Oct 16, 2021
@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Oct 16, 2021

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 Oct 16, 2021
@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Oct 16, 2021

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

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

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Jul 13, 2022 •

Where are the functions like tgamma used? Here in Base?

No - elsewhere in packages that use Base.libm_name. See JuliaMath/NaNMath.jl#55 and the list of packages I mentioned above.

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Jul 14, 2022

@nanosoldier runtests(ALL, vs = ":master")

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Jul 14, 2022

The fmodf problem is still happening on win32. I felt that the CI passed on an earlier version of this PR. The issue is most likely related to what @staticfloat said above in #42299 (comment).

@nanosoldier
Copy link
Collaborator

nanosoldier commented Jul 15, 2022

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@inkydragon
Copy link
Sponsor Member

inkydragon commented Jul 15, 2022

should we find a way to import symbols from ucrtbase.dll on Windows to satisfy things like tgamma

We are using msvcrt for julia, if you want to link/load both msvcrt and ucrtbase to julia. There are maybe some conflict.
We need to do this carefully.

And MS recommends linking against to api-ms-win-*.dll, for tgamma that is api-ms-win-crt-math-l1-1-0.dll.

ref: tgamma, tgammaf, tgammal | Microsoft Docs

julia> ccall((:tgamma, "api-ms-win-crt-math-l1-1-0.dll"), Float64, (Float64,), 10.0)
362880.00000000006
@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Jul 17, 2022

And MS recommends linking against to api-ms-win-*.dll, for tgamma that is api-ms-win-crt-math-l1-1-0.dll.

Would this also be why we have the fmodf not being found on windows issue above (#42299 (comment))?

@inkydragon
Copy link
Sponsor Member

inkydragon commented Jul 17, 2022 •

Would this also be why we have the fmodf not being found on windows issue above

I'm using Win10 x64. So, I checked two dlls.

  • C:\Windows\System32\msvcrt.dll, contained and exported fmodf
    x64 dll
  • C:\Windows\SysWOW64\msvcrt.dll, missing fmodf, only exported fmod
    On x64 system, this is an x86 dll.

But C:\WINDOWS\SysWOW64\ucrtbase.dll (x86 dll) only exported fmod, missing fmodf too.

Then I found out that MSVC just calls fmod in fmodf.

// <math.h>  =>  <corecrt_math.h>
// C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h

// line 687
        _Check_return_ __inline float __CRTDECL fmodf(_In_ float _X, _In_ float _Y)
        {
            return (float)fmod(_X, _Y);
        }

So maybe we just need to wrap the fmod function on Win32.


Bonus: If you do compile an x86 program that calls fmodf and then check the import table, the program actually calls _CIfmod

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Jul 18, 2022

So, what is the right list of files to dlopen on Windows so that we get all the things openlibm has? Seems the list is slightly different for win64 and win32. Where in the codebase should we be doing this?

On win32, the fmodf is being called from llvm generated code - so this is something that needs to be fixed in a way that llvm can call fmodf (can't just be a julia side fix - the issue is only in the codegen).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 18, 2022

somethings just didn't exist in win32, though minor ones, we had been implicitly forwarding those to openlibm

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Jul 18, 2022

somethings just didn't exist in win32, though minor ones, we had been implicitly forwarding those to openlibm

Do we have a list? Can you point to where in the codebase?

@inkydragon
Copy link
Sponsor Member

inkydragon commented Jul 19, 2022

what is the right list of files to dlopen on Windows so that we get all the things openlibm has?

Further examination is needed.
We need to compare the list of exported functions of openlibm and msvcrt.


Do we have a list?

I checked the repo for julia and openlibm. Maybe we don't have one.
But mingw has an exhaustive list of dll exports:

msvcrt

F_NON_I386(fmodf F_X86_ANY(DATA))
https://github.com/mirror/mingw-w64/blob/master/mingw-w64-crt/lib-common/msvcrt.def.in#L1364

The name of the macro F_NON_I386 is self explained.
"function available on everything but i386"
https://github.com/mirror/mingw-w64/blob/c6e13e0c105eab7797c2373819b49fff6b05566c/mingw-w64-crt/def-include/func.def.in#L9

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Oct 18, 2022 •

So, stuff we forward implicitly to openlibm, can we not forward to the Julia versions (can we mark them @ccallable)?

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 29, 2022

Bump. The issue has all ticked (and SpecialFunctions works?). If this is about win32, then it's a tier2 platform. Can this be merged?

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Oct 29, 2022 •

Worth considering, but this will mean that we can never regain win32 without more work. There is not much to gain, but only to lose. @DilumAluthge Thoughts?

I would be ok to merge this if we go all the way and drop win32 altogether from CI.

@gbaraldi
Copy link
Member

gbaraldi commented Nov 8, 2022

Not sure what the issue was here exactly but I implemented a native fmod, which seemed to be a holdout? #47501

@DilumAluthge
Copy link
Member

DilumAluthge commented Nov 8, 2022

IIRC @vtjnash would like to promote Windows i686 to Tier 1, so that would block us from merging this PR.

@DilumAluthge
Copy link
Member

DilumAluthge commented Nov 8, 2022

Oh wait, does Gabriel's PR mean that this PR won't break Windows i686?

@gbaraldi
Copy link
Member

gbaraldi commented Nov 8, 2022

I'm not sure. It would mean that we don't need that specific intrinsic anymore. Which might make this work

@ViralBShah
Copy link
Sponsor Member Author

ViralBShah commented Nov 9, 2022

@gbaraldi You probably want to add your fmod to this PR, and see how it works out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build 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