This is an archive of the discontinued LLVM Phabricator instance.

[lld][flang] Add exceptions for Flang runtime libraries on MinGW.
ClosedPublic

Authored by mmuetzel on Mar 6 2023, 8:14 AM.

Details

Summary

When linking a shared library with Flang on MinGW, the functions from the
Flang runtime are exported from the shared library. When trying to link an
executable to that library using Flang, the linker errors out because the
functions from the runtime conflict with the functions exported from the
shared library.

Add the Flang runtime libraries to the list of libraries for which no
symbols are exported.

Diff Detail

Event Timeline

mmuetzel created this revision.Mar 6 2023, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 8:14 AM
mmuetzel requested review of this revision.Mar 6 2023, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 8:14 AM

See also the downstream report for MSYS2 starting around this comment: https://github.com/msys2/MINGW-packages/pull/16032#issuecomment-1455828447

mstorsjo accepted this revision.Mar 6 2023, 8:57 AM

LGTM, thanks. (We usually don’t add testcases for these cases where we just as more entries to a list.)

This revision is now accepted and ready to land.Mar 6 2023, 8:57 AM

Hold on. I'm not sure if this actually helped...

The command from the downstream example expands to the following for me:

$ flang -### test-cdotu.f -otest-cdotu -lblas
flang-new version 16.0.0
Target: x86_64-w64-windows-gnu
Thread model: posix
InstalledDir: C:/msys64/clang64/bin
 "C:/msys64/clang64/bin/flang-new" "-fc1" "-triple" "x86_64-w64-windows-gnu" "-emit-obj" "-fcolor-diagnostics" "-mrelocation-model" "pic" "-pic-level" "2" "-target-cpu" "x86-64" "-o" "C:/msys64/tmp/test-cdotu-ec65a8.o" "-x" "f95-cpp-input" "test-cdotu.f"
 "C:/msys64/clang64/bin/ld.lld" "-m" "i386pep" "-Bdynamic" "-o" "test-cdotu.exe" "C:/msys64/clang64/lib/crt2.o" "C:/msys64/clang64/lib/crtbegin.o" "-LC:/msys64/clang64/x86_64-w64-mingw32/lib" "-LC:/msys64/clang64/x86_64-w64-mingw32/mingw/lib" "-LC:/msys64/clang64/lib" "-LC:/msys64/clang64/lib/clang/16/lib/windows" "C:/msys64/tmp/test-cdotu-ec65a8.o" "-lblas" "-lFortran_main" "-lFortranRuntime" "-lFortranDecimal" "-lmingw32" "C:/msys64/clang64/lib/clang/16/lib/windows/libclang_rt.builtins-x86_64.a" "-lunwind" "-lmoldname" "-lmingwex" "-lmsvcrt" "-ladvapi32" "-lshell32" "-luser32" "-lkernel32" "-lmingw32" "C:/msys64/clang64/lib/clang/16/lib/windows/libclang_rt.builtins-x86_64.a" "-lunwind" "-lmoldname" "-lmingwex" "-lmsvcrt" "-lkernel32" "C:/msys64/clang64/lib/crtend.o"

Different to the libclang_rt.builtins-x86_64 archive, the Fortran library is linked via -l flags (not via the full path to the library).

Does it need to be treated differently?

Sorry for the noise.
It *is* working as expected (if I don't mess up my tests...).

I'm not sure what the next steps are. Do I need to do something?

I'm not sure what the next steps are. Do I need to do something?

Ah, right; if the dust has settled around whether this solves the issue or not, etc, we should go ahead and push this. If you don't have push access (I saw that you had been involved in some patches before though), I can push it for you. Can you provide your preferred git author line, Real Name <email@address>, for the patch?

Sorry again for causing that noise. I tested by compiling libblas.dll from the reference LAPACK implementation from netlib.
During that test I didn't realize that I had accidentally locked that file (by keeping it loaded with "Dependencies"). After releasing the file, I could verify that the change indeed fixes the issue. The functions from the Flang runtime are no longer exported by libblas.dll.

I don't have push access. (And I guess that wouldn't be a good idea most importantly because I'm still not very familiar with the workflow. But I'm trying to learn.)
I'd be glad if you could push this change with Markus Mützel <markus.muetzel@gmx.de> as the author line.

I don't have push access. (And I guess that wouldn't be a good idea most importantly because I'm still not very familiar with the workflow. But I'm trying to learn.)
I'd be glad if you could push this change with Markus Mützel <markus.muetzel@gmx.de> as the author line.

I pushed the patch now, but I totally forgot to amend in your git user name - sorry for the mess. I'll revert and re-push with the right author name.

mmuetzel added a comment.EditedMar 9 2023, 3:20 AM

I pushed the patch now, but I totally forgot to amend in your git user name - sorry for the mess. I'll revert and re-push with the right author name.

Don't worry about that. It's not that important to me.

And thank you for taking care of this.

Is it worth cherry-picking this to LLVM 16?
It seems low-risk. But iiuc, Flang isn't advertised as being production-ready anyway.

Is it worth cherry-picking this to LLVM 16?
It seems low-risk. But iiuc, Flang isn't advertised as being production-ready anyway.

Yeah this is very low risk, so it should be a good fit for backporting to the release. But AFAIK they're trying hard to squeeze out the last bugs to get 16.0.0 out now, and this is kinda low priority, so perhaps it's better to not make more noise about this right now - perhaps it's best to wait until after 16.0.0 to file a backport request?

Is it worth cherry-picking this to LLVM 16?
It seems low-risk. But iiuc, Flang isn't advertised as being production-ready anyway.

Yeah this is very low risk, so it should be a good fit for backporting to the release. But AFAIK they're trying hard to squeeze out the last bugs to get 16.0.0 out now, and this is kinda low priority, so perhaps it's better to not make more noise about this right now - perhaps it's best to wait until after 16.0.0 to file a backport request?

I filed a backport request for this now in https://github.com/llvm/llvm-project/issues/61618.