-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[flang][windows] Add option to link against specific MSVC CRT #70833
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -976,12 +976,46 @@ bool tools::addOpenMPRuntime(ArgStringList &CmdArgs, const ToolChain &TC, | |
return true; | ||
} | ||
|
||
void tools::addFortranRuntimeLibs(const ToolChain &TC, | ||
void tools::addFortranRuntimeLibs(const ToolChain &TC, const ArgList &Args, | ||
llvm::opt::ArgStringList &CmdArgs) { | ||
if (TC.getTriple().isKnownWindowsMSVCEnvironment()) { | ||
CmdArgs.push_back("Fortran_main.lib"); | ||
CmdArgs.push_back("FortranRuntime.lib"); | ||
CmdArgs.push_back("FortranDecimal.lib"); | ||
CmdArgs.push_back(Args.MakeArgString( | ||
"/DEFAULTLIB:" + TC.getCompilerRTBasename(Args, "builtins"))); | ||
unsigned RTOptionID = options::OPT__SLASH_MT; | ||
if (auto *rtl = Args.getLastArg(options::OPT_fms_runtime_lib_EQ)) { | ||
RTOptionID = llvm::StringSwitch<unsigned>(rtl->getValue()) | ||
.Case("static", options::OPT__SLASH_MT) | ||
.Case("static_dbg", options::OPT__SLASH_MTd) | ||
.Case("dll", options::OPT__SLASH_MD) | ||
.Case("dll_dbg", options::OPT__SLASH_MDd) | ||
.Default(options::OPT__SLASH_MT); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The switch accepts names There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. Now that I see those names on disk after building from your update, file names like |
||
} | ||
switch (RTOptionID) { | ||
case options::OPT__SLASH_MT: | ||
CmdArgs.push_back("/DEFAULTLIB:libcmt"); | ||
CmdArgs.push_back("Fortran_main.static.lib"); | ||
CmdArgs.push_back("FortranRuntime.static.lib"); | ||
CmdArgs.push_back("FortranDecimal.static.lib"); | ||
break; | ||
case options::OPT__SLASH_MTd: | ||
CmdArgs.push_back("/DEFAULTLIB:libcmtd"); | ||
CmdArgs.push_back("Fortran_main.static_dbg.lib"); | ||
CmdArgs.push_back("FortranRuntime.static_dbg.lib"); | ||
CmdArgs.push_back("FortranDecimal.static_dbg.lib"); | ||
break; | ||
case options::OPT__SLASH_MD: | ||
CmdArgs.push_back("/DEFAULTLIB:msvcrt"); | ||
CmdArgs.push_back("Fortran_main.dynamic.lib"); | ||
CmdArgs.push_back("FortranRuntime.dynamic.lib"); | ||
CmdArgs.push_back("FortranDecimal.dynamic.lib"); | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From #70833 (comment):
This temporary approach will actually make things harder for CMake to support Can you instead add these four CRT-specific libraries as defaultlib directives in all object files, and add the more detailed conditions to remove unnecessary libraries later? Since they are all static There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to add defaultlib directives using a CLI tool, or do they have to be added when the compiler creates the object file? The main issue is we have an MLIR step in between flang and LLVM IR and I don't believe MLIR supports the attributes for setting these defaultlib directives yet, so implimenting that might be quite a lot of work... It's not so much that we would have difficulty adding the specific defaultlib directive we need, as that we'd have an issue adding any of them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having had a look, I think we can replicate what clang -cc1's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not aware of any easy way to add defaultlib directives after-the-fact. I think it's okay to merge this PR's approach as a temporary solution. It does fix the empty- BTW, the |
||
case options::OPT__SLASH_MDd: | ||
CmdArgs.push_back("/DEFAULTLIB:msvcrtd"); | ||
CmdArgs.push_back("Fortran_main.dynamic_dbg.lib"); | ||
CmdArgs.push_back("FortranRuntime.dynamic_dbg.lib"); | ||
CmdArgs.push_back("FortranDecimal.dynamic_dbg.lib"); | ||
break; | ||
} | ||
} else { | ||
CmdArgs.push_back("-lFortran_main"); | ||
CmdArgs.push_back("-lFortranRuntime"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,3 +53,26 @@ add_flang_library(FortranDecimal INSTALL_WITH_TOOLCHAIN | |
binary-to-decimal.cpp | ||
decimal-to-binary.cpp | ||
) | ||
|
||
if (DEFINED MSVC) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of redefining There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I tried this but inside add_flang_library these are made into custom targets (meaning the target property doesn’t do anything) and I’m not entirely sure why so I didn’t want to mess with it too much. I’ll give it another try There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You should be able to do add_flang_library(FortranDecimal.static ...)
set_property(TARGET FortranDecimal.static PROPERTY MSVC_RUNTIME_LIBRARY MultiThreaded) |
||
add_flang_library(FortranDecimal.static INSTALL_WITH_TOOLCHAIN | ||
binary-to-decimal.cpp | ||
decimal-to-binary.cpp | ||
) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDLL) | ||
add_flang_library(FortranDecimal.dynamic INSTALL_WITH_TOOLCHAIN | ||
binary-to-decimal.cpp | ||
decimal-to-binary.cpp | ||
) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDebug) | ||
add_flang_library(FortranDecimal.static_dbg INSTALL_WITH_TOOLCHAIN | ||
binary-to-decimal.cpp | ||
decimal-to-binary.cpp | ||
) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDebugDLL) | ||
add_flang_library(FortranDecimal.dynamic_dbg INSTALL_WITH_TOOLCHAIN | ||
binary-to-decimal.cpp | ||
decimal-to-binary.cpp | ||
) | ||
endif() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -274,10 +274,38 @@ if (NOT FLANG_EXPERIMENTAL_OMP_OFFLOAD_BUILD STREQUAL "off") | |
endif() | ||
endif() | ||
|
||
add_flang_library(FortranRuntime | ||
${sources} | ||
LINK_LIBS | ||
FortranDecimal | ||
if (NOT DEFINED MSVC) | ||
add_flang_library(FortranRuntime | ||
${sources} | ||
LINK_LIBS | ||
FortranDecimal | ||
|
||
INSTALL_WITH_TOOLCHAIN | ||
) | ||
INSTALL_WITH_TOOLCHAIN | ||
) | ||
else() | ||
add_flang_library(FortranRuntime | ||
${sources} | ||
LINK_LIBS | ||
FortranDecimal | ||
) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded) | ||
add_flang_library(FortranRuntime.static ${sources} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When targeting the MSVC ABI, the plain There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The plain FortranRuntime library is linked by the Runtime tests, which need to be built against whatever the user built LLVM with, which we can't necessarily find out that easily I think. It should not have INSTALL_WITH_TOOLCHAIN set though, I'll remove that |
||
LINK_LIBS | ||
FortranDecimal.static | ||
INSTALL_WITH_TOOLCHAIN) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDLL) | ||
add_flang_library(FortranRuntime.dynamic ${sources} | ||
LINK_LIBS | ||
FortranDecimal.dynamic | ||
INSTALL_WITH_TOOLCHAIN) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDebug) | ||
add_flang_library(FortranRuntime.static_dbg ${sources} | ||
LINK_LIBS | ||
FortranDecimal.static_dbg | ||
INSTALL_WITH_TOOLCHAIN) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDebugDLL) | ||
add_flang_library(FortranRuntime.dynamic_dbg ${sources} | ||
LINK_LIBS | ||
FortranDecimal.dynamic_dbg | ||
INSTALL_WITH_TOOLCHAIN) | ||
endif() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,21 @@ | ||
add_flang_library(Fortran_main STATIC INSTALL_WITH_TOOLCHAIN | ||
Fortran_main.c | ||
) | ||
if (DEFINED MSVC) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded) | ||
add_flang_library(Fortran_main.static STATIC INSTALL_WITH_TOOLCHAIN | ||
Fortran_main.c | ||
) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDLL) | ||
add_flang_library(Fortran_main.dynamic STATIC INSTALL_WITH_TOOLCHAIN | ||
Fortran_main.c | ||
) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDebug) | ||
add_flang_library(Fortran_main.static_dbg STATIC INSTALL_WITH_TOOLCHAIN | ||
Fortran_main.c | ||
) | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDebugDLL) | ||
add_flang_library(Fortran_main.dynamic_dbg STATIC INSTALL_WITH_TOOLCHAIN | ||
Fortran_main.c | ||
) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of LLVM 17.0's distribution, the Fortran runtime libraries are built with
msvcrt
, so I think the current default is actually/MD
. Since this wasn't really modeled before, and CMake will be taught to pass one of the four flags explicitly anyway, changing the default may not matter, but it's something to be aware of.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should take the opportunity to match what clang's default behaviour is here, which is to pass
/MT
. I think having the two do something different is surprising at the moment and should be changed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, changing the default to match Clang makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In local testing,
flang-new -fms-runtime-lib=static foo.f90 -v
, wherefoo.f90
is an emptyprogram
statement, fails with a bunch of unresolved CRT symbols.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you share the missing symbols? If it's something like
__udivti3
then that's an issue with not finding compiler-rt (those symbols are for 128 bit types and not contained in the MSVC rt libs of any flavour), see #25679. If so, there's perhaps a chance that things work withflang-new -DAVOID_NATIVE_UINT128_T=1
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should be adding the missing compiler-rt libs in this patch too. I don’t see any missing symbols in any of the configurations when testing an empty program locally. Could you share the error you’re seeing?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The errors were due to #70833 (review) because the runtime library variants were not being built with the correct CRT themselves. After switching back to the
CMAKE_MSVC_RUNTIME_LIBRARY
the problem is resolved.