[flang/flang-rt] Add -isysroot flag only to tests really requiring #152914
Conversation
-isysroot flag was added to all tests, but it makes Driver/darwin-version.f90 failed. In fact, only a few tests need -isysroot flag. So, -isysroot flag is now eliminated from the substitution `%flang`, and a new substitution `%isysroot` has been introduced. Moreover, Integration/iso-fortran-binding.cpp invokes clang++ via a shell script, which makes it hard to add -isysroot flag. So, it is refactored.
flang/test/Integration/lit.local.cfg
Outdated
|
|
||
| # Include path for C headers that define Flang's Fortran ABI. | ||
| config.substitutions.append( | ||
| ("%include", os.path.join(config.flang_source_dir, "include")) |
There was a problem hiding this comment.
What is this for? I comment it out and it still works.
There was a problem hiding this comment.
It would be necessary when building flang standalone.
/path/to/build/bin/clang --driver-mode=g++ -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX15.5.sdk -I %include/flang /path/to/llvm-project/flang/test/Integration/iso-fortran-binding.cpp -o /path/to/build2/flang/test/Integration/Output/iso-fortran-binding.cpp.tmp/a.out # RUN: at line 4
+ /path/to/build/bin/clang --driver-mode=g++ -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX15.5.sdk -I %include/flang /path/to/llvm-project/flang/test/Integration/iso-fortran-binding.cpp -o /path/to/build2/flang/test/Integration/Output/iso-fortran-binding.cpp.tmp/a.out
/path/to/llvm-project/flang/test/Integration/iso-fortran-binding.cpp:8:10: fatal error: 'ISO_Fortran_binding.h' file not found
8 | #include "ISO_Fortran_binding.h"
| ^~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
--
/path/to/build/bin/clang++ does not search /path/to/build2/lib/clang/22/include.
flang/test/lit.cfg.py
Outdated
| # Include path for C headers that define Flang's Fortran ABI. | ||
| config.substitutions.append( | ||
| ("%include", os.path.join(config.flang_source_dir, "include")) | ||
| ) |
There was a problem hiding this comment.
I assume that this is for ISO_fortran_binding.h. Does this get copied into the build directory during compilation? If so, it may be better to include the file from the build directory instead of the source directory.
There was a problem hiding this comment.
Thank you for pointing it out. Using flang_intrinsic_modules_dir seems more suitable for this usage.
| @@ -6,10 +6,12 @@ import lit.util | |||
| config.llvm_tools_dir = lit_config.substitute("@LLVM_TOOLS_DIR@") | |||
| config.llvm_shlib_dir = lit_config.substitute(path(r"@SHLIBDIR@")) | |||
| config.llvm_plugin_ext = "@LLVM_PLUGIN_EXT@" | |||
| config.host_triple = "@LLVM_HOST_TRIPLE@" | |||
There was a problem hiding this comment.
Why has this been added? I don't see it being used anywhere in this PR - at least not directly.
There was a problem hiding this comment.
It is used by llvm_config.use_clang(required=False). But it also has a fallback when not defined. Without e.g. the feature native is not defined. Not sure we strictly need it.
There was a problem hiding this comment.
If I remove this line, I'll get the following error message. We could fix clang_setup by adding a check for hasattr(self.config, "host_triple"), but I'm not sure it is better.
llvm-lit: /path/to/llvm-project/llvm/utils/lit/lit/TestingConfig.py:157: fatal: unable to parse config file '/path/to/llvm-project/flang/test/lit.cfg.py', traceback: Traceback (most recent call last):
File "/path/to/llvm-project/llvm/utils/lit/lit/TestingConfig.py", line 145, in load_from_path
exec(compile(data, path, "exec"), cfg_globals, None)
~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/llvm-project/flang/test/lit.cfg.py", line 175, in <module>
llvm_config.use_clang(required=False)
~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
File "/path/to/llvm-project/llvm/utils/lit/lit/llvm/config.py", line 689, in use_clang
self.clang_setup(additional_tool_dirs)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/llvm-project/llvm/utils/lit/lit/llvm/config.py", line 638, in clang_setup
if self.config.host_triple and self.config.host_triple != "@LLVM_HOST_TRIPLE@":
^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'TestingConfig' object has no attribute 'host_triple'
There was a problem hiding this comment.
I've seen hasattr(self.config, "host_triple") at other places, seems that some change did not consider it may not be set.
Since we would want clang to know whether it is cross-compiling, I would keep config.host_triple = .. as-is.
|
✅ With the latest revision this PR passed the Python code formatter. |
parabola94
force-pushed
the
flang-isysroot-test
branch
from
3ceb84e to
0cef883
Compare
flang/test/lit.cfg.py
Outdated
| ("%include", os.path.join(config.flang_source_dir, "include")) | ||
| ) | ||
| # Clang may need the include path for ISO_fortran_binding.h. | ||
| config.substitutions.append(("%flang_include", config.flang_intrinsic_modules_dir)) |
There was a problem hiding this comment.
flang_intrinsic_modules_dir is for .mod files. ISO_Fortran_binding.h is written into ${HEADER_BINARY_DIR}.
llvm-project/flang/CMakeLists.txt
Lines 580 to 582 in 4242589
Locations are supposed to diverge in #137828
There was a problem hiding this comment.
Thank you for the information. I have fixed it.
(${HEADER_BINARY_DIR} should be defined before lit.site.cfg.py is generated.)
-isysroot flag was added to all tests, but it makes Driver/darwin-version.f90 failed.
In fact, only a few tests regarding interoperability with C need -isysroot flag to search for headers and libraries. So, -isysroot flag is now eliminated from the substitution
%flang, and a new substitution%isysroothas been introduced.Moreover, Integration/iso-fortran-binding.cpp invokes clang++ via a shell script, which makes it hard to add -isysroot flag. So, it is refactored.
Fixes #150765