-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[flang][driver] Make -stdlib= option visible to flang and silently ignored by it #110598
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
Conversation
|
@llvm/pr-subscribers-clang Author: Paul Osmialowski (pawosm-arm) ChangesThis patch is to address a sudden problem I have encountered while trying to build mixed-language Fortran/C++ project. As I wanted the C++ part to utilize libc++ (instead of system-provided libstdc++), I've specified the This has created a nowhere to go situation, the only remaining option is to make the -stdlib flag visible to flang and silently ignored. Full diff: https://github.com/llvm/llvm-project/pull/110598.diff 2 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 607ff47a857b8f..3f8f980c20a279 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -5812,7 +5812,7 @@ def std_EQ : Joined<["-", "--"], "std=">,
;
}]>;
def stdlib_EQ : Joined<["-", "--"], "stdlib=">,
- Visibility<[ClangOption, CC1Option]>,
+ Visibility<[ClangOption, CC1Option, FlangOption]>,
HelpText<"C++ standard library to use">, Values<"libc++,libstdc++,platform">;
def stdlibxx_isystem : JoinedOrSeparate<["-"], "stdlib++-isystem">,
Group<clang_i_Group>,
diff --git a/flang/test/Integration/mixed-lang-stdlib.cpp b/flang/test/Integration/mixed-lang-stdlib.cpp
new file mode 100644
index 00000000000000..420733b6075749
--- /dev/null
+++ b/flang/test/Integration/mixed-lang-stdlib.cpp
@@ -0,0 +1,50 @@
+// REQUIRES: system-linux
+// RUN: split-file %s %t
+// RUN: chmod +x %t/mixed-runtest.sh
+// RUN: %t/mixed-runtest.sh -stdlib=platform %t %t/mixed-cppfile.cpp %t/mixed-fortranfile.f90 %flang | FileCheck %s
+
+//--- mixed-cppfile.cpp
+#include <iostream>
+
+extern "C" void hello(void)
+{
+ std::cout<<"Hello"<<std::endl;
+}
+
+// clang-format off
+// CHECK: PASS
+//--- mixed-fortranfile.f90
+program mixed
+ implicit none
+ interface
+ subroutine hello() bind(c)
+ implicit none
+ end subroutine
+ end interface
+
+ call hello()
+end program
+
+//--- mixed-runtest.sh
+#!/bin/bash
+LDFLAGS=$1
+TMPDIR=$2
+CPPFILE=$3
+F90FILE=$4
+FLANG=$5
+BINDIR=`dirname $FLANG`
+CPPCOMP=$BINDIR/clang++
+if [ -x $CPPCOMP ]
+then
+ $CPPCOMP -shared $LDFLAGS $CPPFILE -o $TMPDIR/libmixed.so
+ $FLANG $LDFLAGS -o $TMPDIR/mixed $F90FILE -L$TMPDIR -lmixed -Wl,-rpath=$TMPDIR
+ if [ -x $TMPDIR/mixed ]
+ then
+ echo "PASS"
+ else
+ echo "FAIL"
+ fi
+else
+ # No clang compiler, just pass by default
+ echo "PASS"
+fi
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
91d899b to
faa0df3
Compare
…nored by it This patch is to address a sudden problem I have encountered while trying to build mixed-language Fortran/C++ project. As I wanted the C++ part to utilize libc++ (instead of system-provided libstdc++), I've specified the `-stdlib=libc++` option globally, in the CXXFLAGS and LDFLAGS variables. This has saved me from making expensive changes in the Makefiles. Unfortunately, since it was the flang complier that was invoked for linking, suddenly it ended up with an error: ``` flang-new: error: unknown argument '-stdlib=libc++'; did you mean '-Xclang -stdlib=libc++'? ``` This has created a nowhere to go situation, the only remaining option is to make the -stdlib flag visible to flang and silently ignored.
faa0df3 to
beac073
Compare
|
I don't think it is correct to say that the option was ignored. It may still be read by the compiler driver (which generates the link line). In fact it probably should be processed by the compiler driver in order to set the C++ standard library correctly - otherwise you could just not set this in your |
But this (ignoring) is exactly what is happening. Contrary to classic flang, flang-new cannot be used to mix C++ and Fortran code in one compiler invocation, and working on this patch I've even found a test case guarding that controversial decision ( Using this flag for compilation of a Fortran code will display a warining: ...and this is expected. Using this flag for linking will not do anything, looking at |
|
If this is completely ignored and does not effect the link line, are you sure the resulting binary actually uses the right c++ library? Edit: flang will not link a C++ library. It is not a C++ compiler. |
|
We discussed this further offline and I am now convinced there could be a valid usecase for this. Some C++ code might use header-only parts of the C++ standard library. Setting When flang links the program it will not link to a C++ library by default so it is correct that this does nothing. But if cmake wants to include CXXFLAGS inside of LDFLAGS, it is more user friendly to allow this flag. If the user uses something that would require linking to the C++ library they will get a linker error not silent linking to the wrong c++ library. |
tblah
left a comment
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.
LGTM but if possible please wait for another reviewer
As we discussed it offline, in case of this project it does not matter, no calls to the library are actually being made from the C++ code, and it was more important what CMake did with CXXFLAGS (so the headers from the right C++ standard library were used). Also the integration test I've added depicts that flang-new doesn't really need to know anything about the language and standard libraries they use. The trouble here is that linker flags were blindly propagated by CMake (when asked to specify C++ standard library) no matter if it has any use or not. In smaller project, where we know that the C++ part isn't actually calling anything from library, the -stdlib flang should not be used during linking at all, but here we're talking about large scale project, where the Flang/C++ part is just a small part so the project maintainers didn't really care about making an exception in the flags being used when linking just that part. |
|
While the FortranRuntime is written in C++, it is ensured that it does not depend on libc++.so/libstdc++.so (regression test). Hence what C++ runtime the user links into their executable shouldn't concern the Fortran part. When mixing Fortran and C++ code with Fortran code in the same add_library/add_executable, will use the C++ linker to link all the object files together (just as with C and C++). That might be the expected thing to do. That means it will care for linking the correct C++ library, but leave out the Fortran runtime library, and the user has to add $ bin/clang hello.o -### -stdlib=platform
"/usr/bin/ld" "-z" "relro" "--hash-style=gnu" "--eh-frame-hdr" "-m" "elf_x86_64" "-pie" "-dynamic-linker" "/lib64/ld-linux-x86-64.so.2" "-o" "a.out" "/lib/x86_64-linux-gnu/Scrt1.o" "/lib/x86_64-linux-gnu/crti.o" "/usr/lib/gcc/x86_64-linux-gnu/13/crtbeginS.o" "-L/home/meinersbur/build/llvm-project/release/lib/clang/20/lib/x86_64-unknown-linux-gnu" "-L/usr/lib/gcc/x86_64-linux-gnu/13" "-L/usr/lib/gcc/x86_64-linux-gnu/13/../../../../lib64" "-L/lib/x86_64-linux-gnu" "-L/lib/../lib64" "-L/usr/lib/x86_64-linux-gnu" "-L/usr/lib/../lib64" "-L/lib" "-L/usr/lib" "hello.o" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "-lc" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "/usr/lib/gcc/x86_64-linux-gnu/13/crtendS.o" "/lib/x86_64-linux-gnu/crtn.o" [ 0.082s → 0 ]
$ bin/clang++ hello.o -### -stdlib=platform
"/usr/bin/ld" "-z" "relro" "--hash-style=gnu" "--eh-frame-hdr" "-m" "elf_x86_64" "-pie" "-dynamic-linker" "/lib64/ld-linux-x86-64.so.2" "-o" "a.out" "/lib/x86_64-linux-gnu/Scrt1.o" "/lib/x86_64-linux-gnu/crti.o" "/usr/lib/gcc/x86_64-linux-gnu/13/crtbeginS.o" "-L/home/meinersbur/build/llvm-project/release/lib/clang/20/lib/x86_64-unknown-linux-gnu" "-L/usr/lib/gcc/x86_64-linux-gnu/13" "-L/usr/lib/gcc/x86_64-linux-gnu/13/../../../../lib64" "-L/lib/x86_64-linux-gnu" "-L/lib/../lib64" "-L/usr/lib/x86_64-linux-gnu" "-L/usr/lib/../lib64" "-L/lib" "-L/usr/lib" "hello.o" "-lstdc++" "-lm" "-lgcc_s" "-lgcc" "-lc" "-lgcc_s" "-lgcc" "/usr/lib/gcc/x86_64-linux-gnu/13/crtendS.o" "/lib/x86_64-linux-gnu/crtn.o" (and there isn't even a warning that the option is unused) @pawosm-arm Wasn't this ever an issue? Should those to be consistent? Note that the test will fail when The flag That is, I think the patch is fine, modulo inherited issues. |
I wonder how the other C++ integration test in this directory ( I also wanted to bring your attention to the subject in context of your work on extracting Fortran Runtime, what I've noticed is that using |
Interesting, clang would do the same as flang-new patched with my patch. Maybe addition of the warning would be a good idea, if it isn't an overkill. Not sure if clang and clang++ should be consistent (i.e. expose the same behavior), this is not in the scope of this patch and not for me to judge |
|
We should avoid dummy flags like this - they are usually an indication of a larger issue. I don't want to block this and, as @Meinersbur points out, there are some "inherited issues" to consider too. However, I'd like to identify the right long-term solution here. If possible.
Why are CXXFLAGS used when invoking a Fortran compiler? That's not correct, is it?
To me it sounds like:
If |
|
From the summary?
Can you clarify? I think it is not ignored, otherwise the test would fail since it requires linking to the
AFAICS your patch only makes the
What effects? |
|
I missed replying on this review somehow, sorry to come back 2 weeks later...
Assuming this is all using cmake: I don't think CXXFLAGS will be being added to the flang invocation in this case. What I believe is happening is that cmake prefers to use the compiler driver for the relevant language to link the final binaries, so in this case is invoking The CXXFLAGS are only passed to CXX and the FCFLAGS only passed to FC, and the LDFLAGS will be passed to whichever cmake chooses to use as the linker at the end as well as CXXFLAGS or FCFLAGS depending which was picked. So probably you shouldn't be adding A quick test shows me that CMake picks the CXX compiler in mixed-language CXX and Fortran programs though, so I'm not sure why @pawosm-arm is seeing an error. It looks like CMake even does the right thing and manually adds -lFortranRuntime etc to the tl;dr this looks like a cmake misconfiguration issue to me? |
|
An example for my previous point. If I have a CMakeLists.txt: and the relevant files (doesn't really matter what they contain), then if I build with: then I get the output: and a working binary linked to libc++ with no extra special anything required. As you can see on the last line, all the correct libraries are added by CMake. |
|
@DavidTruby So CMake adds I found how CMake does it: https://github.com/Kitware/CMake/blob/ee2eadb252708b2401da99b8ccc69d6501c7143f/Modules/CMakeDetermineFortranCompiler.cmake#L246 I don't think we can rely on every build system doing that, so I would still advocate that |
|
I don’t personally think that the flang driver should ever attempt to link a C++ library of any kind. FWIW while there’s no stdlib option to consider in their case, g++ will never auto link the gfortran runtimes and gfortran will never auto link libstdc++ Mixed C++/Fortran programs are always going to be complicated for exactly this reason: as a user I wouldn’t expect a C++ compiler driver to automatically link a Fortran runtime or vice versa, and would find it quite surprising if that happened. In my opinion the current behaviour (which matches g++/gfortran) to require build systems to handle the complexity of adding the right linkage flags is the correct one. This would leave us rejecting stdlib= as an unrecognised option on flang as is the case in current main.
If we were to do things this way, what should flang do if no -stdlib flag is provided? Should it link the default C++ stdlib as clang++ does, or break with clang++ and not link any C++ stdlib if nothing is provided? Both would be surprising behaviour in my opinion, and both would also risk breaking the existing cmake support. |
gfortran doesn't have a flag that means "use this C++ standard library". Without this PR, Flang doesn't either which would be fine with me. OP would need to fix their build system. With this PR,
No |
|
Sorry I wasn’t very clear in my previous comments; I think we shouldn’t go ahead with this PR and should continue to reject the flag. So I think we’re in agreement. |
Meinersbur
left a comment
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.
Marking this PR as no consensus reached
|
@pawosm-arm do you still need this or do the CMake configuration changes I suggested work? Can we close this if it's not needed anymore? |
|
There was a change of plans, so I don't need it now. But I expect the subject will return sooner or later raised by someone else who stumble upon this just like me. Closing it for now. |
This patch is to address a sudden problem I have encountered while trying to build mixed-language Fortran/C++ project. As I wanted the C++ part to utilize libc++ (instead of system-provided libstdc++), I've specified the
-stdlib=libc++option globally, in the CXXFLAGS and LDFLAGS variables. This has saved me from making expensive changes in the Makefiles. Unfortunately, since it was the flang complier that was invoked for linking, suddenly it ended up with an error:This has created a nowhere to go situation, the only remaining option is to make the -stdlib flag visible to flang and silently ignored.