[flang-rt] Remove hard-coded dependency on compiler-rt path on Windows #150244
Conversation
This fixes an issue where if the build folder is no longer present flang cannot link anything on Windows because the path to compiler-rt in the binary is hard-coded. Flang already links compiler-rt on Windows so it isn't necessary for flang-rt to specify that it depends on compiler-rt at all, other than for the unit tests, so instead we can move that logic into the unit test compile lines.
There was a problem hiding this comment.
Flang already links compiler-rt on Windows
But Clang does not, and Flang-RT consists of mostly C/C++ files which Clang emits calls to __udivti3 into.
"Mostly", because flang_rt.runtime has iso_fortran_env_impl.f90 which is compiled using Fortran which should already add --dependent-lib=.. into flang_rt.runtime.
While theoretically CMAKE_Fortran_COMPILER could be another compiler, and I would prefer to have the dependency in a central location (i.e. removing flang implicitly adding a dependency to compile_rt.builtins but make it link transitively through flang_rt), that's probably too idealistic. I would have suggested to at least add --dependent-libs=flang_rt.runtime.<triple>.lib without path, but it should not matter in practice.
LGTM
|
I meant more specifically that flang-rt exists to be linked into binaries compiled by flang, which already adds the dependency to compiler-rt as well. So flang-rt doesn't really need the dependency itself. |
Non-Flang-compiled artifacts may also want to link to flang_rt.runtime, e.g. flang-rt as a dll (although not yet possible), C/C++ programs that #include <ISO_Fortran_binding.h> like those unittests.
And yet we duplicate them for the unittests?!? If it wasn't for |
|
Don't forget, I LGTM'ed this PR. Practically it may still be the best approach. May have to-rethink when adding support for compiling flang-rt as a DLL. |
Yes sorry I wasn't trying to argue but rather explain why I think this approach works, at least for now as a simple fix that can be backported to the release branch too. I hadn't considered DLL support, that might need more thinking once we get to it.
We don't link the output of compiling iso_fortran_env_impl.f90 to all the unit tests so we do need to duplicate it here unfortunately. The linker already has the path to compiler-rt added but still doesn't link it without us saying to do it forcefully unfortunately. I don't think this is an issue for the unit tests though as I think it's safe to assume they will always be run from a build directory, in which case linking with the whole path to the build directory is fine.
Presumably these will still link to a flang-compiled artifact eventually though? That header only exists to write Fortran bindings so either you'll have to link to some Fortran code, or you'll get an error for not linking a function you've declared anyway. But yeah, not saying this is necessarily the best approach, but we need a quick-ish fix to backport before the release and I think this works well enough for that without introducing too much churn. I'll merge it on that basis for now and if we need a better fix in future we can revisit. |
|
/cherry-pick c20a95a |
|
/pull-request #150764 |
llvm#150244) This fixes an issue where if the build folder is no longer present flang cannot link anything on Windows because the path to compiler-rt in the binary is hard-coded. Flang already links compiler-rt on Windows so it isn't necessary for flang-rt to specify that it depends on compiler-rt at all, other than for the unit tests, so instead we can move that logic into the unit test compile lines.
llvm#150244) This fixes an issue where if the build folder is no longer present flang cannot link anything on Windows because the path to compiler-rt in the binary is hard-coded. Flang already links compiler-rt on Windows so it isn't necessary for flang-rt to specify that it depends on compiler-rt at all, other than for the unit tests, so instead we can move that logic into the unit test compile lines. (cherry picked from commit c20a95a)
llvm#150244) This fixes an issue where if the build folder is no longer present flang cannot link anything on Windows because the path to compiler-rt in the binary is hard-coded. Flang already links compiler-rt on Windows so it isn't necessary for flang-rt to specify that it depends on compiler-rt at all, other than for the unit tests, so instead we can move that logic into the unit test compile lines. (cherry picked from commit c20a95a)
This fixes an issue where if the build folder is no longer present flang cannot link anything on Windows because the path to compiler-rt in the binary is hard-coded. Flang already links compiler-rt on Windows so it isn't necessary for flang-rt to specify that it depends on compiler-rt at all, other than for the unit tests, so instead we can move that logic into the unit test compile lines.