[flang] Fix failure of regression tests on macOS

[parabola94]

Some tests in flang fail on macOS because of SDKROOT. It appends-isysroot flag, but some tests did not handle it appropriately. This patch fixes #150765.

[llvmbot]

@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-flang-openmp

Author: None (parabola94)

Changes

Some tests in flang fail on macOS because of SDKROOT. It appends-isysroot flag, but some tests did not handle it appropriately. This patch fixes #150765.

---

Full diff: https://github.com/llvm/llvm-project/pull/151812.diff

2 Files Affected:

• (added) flang/test/Driver/lit.local.cfg (+10)
• (modified) flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90 (+2-2)

diff --git a/flang/test/Driver/lit.local.cfg b/flang/test/Driver/lit.local.cfg
new file mode 100644
index 0000000000000..aef6bd734300f
--- /dev/null
+++ b/flang/test/Driver/lit.local.cfg
@@ -0,0 +1,10 @@
+import re
+from lit.llvm import llvm_config
+
+# Drop -isysroot flag for darwin-version.f90.
+idx, driver = next(((i, subst)
+    for i, subst in enumerate(config.substitutions) if "flang" in subst[0]))
+subst_key, command = driver
+command = re.sub(' -isysroot [^ ]+', '', command)
+config.substitutions.insert(idx, (subst_key, command))
+config.substitutions.remove(driver)
diff --git a/flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90 b/flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90
index 0d4fd964b71ec..72b5fea2c171e 100644
--- a/flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90
+++ b/flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90
@@ -1,7 +1,7 @@
 ! This test checks the lowering and application of default map types for the target enter/exit data constructs and map clauses
 
-!RUN: %flang -fc1 -emit-fir -fopenmp -fopenmp-version=52 -o - %s | FileCheck %s --check-prefix=CHECK-52
-!RUN: not %flang -fc1 -emit-fir -fopenmp -fopenmp-version=51 -o - %s 2>&1| FileCheck %s --check-prefix=CHECK-51
+!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-version=52 -o - %s | FileCheck %s --check-prefix=CHECK-52
+!RUN: not %flang_fc1 -emit-fir -fopenmp -fopenmp-version=51 -o - %s 2>&1| FileCheck %s --check-prefix=CHECK-51
 
 module test
   real, allocatable :: A

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

Author: None (parabola94)

Changes

Some tests in flang fail on macOS because of SDKROOT. It appends-isysroot flag, but some tests did not handle it appropriately. This patch fixes #150765.

---

Full diff: https://github.com/llvm/llvm-project/pull/151812.diff

2 Files Affected:

• (added) flang/test/Driver/lit.local.cfg (+10)
• (modified) flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90 (+2-2)

diff --git a/flang/test/Driver/lit.local.cfg b/flang/test/Driver/lit.local.cfg
new file mode 100644
index 0000000000000..aef6bd734300f
--- /dev/null
+++ b/flang/test/Driver/lit.local.cfg
@@ -0,0 +1,10 @@
+import re
+from lit.llvm import llvm_config
+
+# Drop -isysroot flag for darwin-version.f90.
+idx, driver = next(((i, subst)
+    for i, subst in enumerate(config.substitutions) if "flang" in subst[0]))
+subst_key, command = driver
+command = re.sub(' -isysroot [^ ]+', '', command)
+config.substitutions.insert(idx, (subst_key, command))
+config.substitutions.remove(driver)
diff --git a/flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90 b/flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90
index 0d4fd964b71ec..72b5fea2c171e 100644
--- a/flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90
+++ b/flang/test/Lower/OpenMP/target-enter-data-default-openmp52.f90
@@ -1,7 +1,7 @@
 ! This test checks the lowering and application of default map types for the target enter/exit data constructs and map clauses
 
-!RUN: %flang -fc1 -emit-fir -fopenmp -fopenmp-version=52 -o - %s | FileCheck %s --check-prefix=CHECK-52
-!RUN: not %flang -fc1 -emit-fir -fopenmp -fopenmp-version=51 -o - %s 2>&1| FileCheck %s --check-prefix=CHECK-51
+!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-version=52 -o - %s | FileCheck %s --check-prefix=CHECK-52
+!RUN: not %flang_fc1 -emit-fir -fopenmp -fopenmp-version=51 -o - %s 2>&1| FileCheck %s --check-prefix=CHECK-51
 
 module test
   real, allocatable :: A

[tarunprabhu]

How do the clang tests handle this? Is it really necessary to remove -isysroot like this? I have very limited experience building on MacOS though, so I'm afraid I cannot offer something more constructive.

[Meinersbur]

Please avoid modifying substitutions after the fact. It is confusing if %flang_fc1 means something in one test, but something else in another. Change

llvm-project/flang-rt/test/lit.cfg.py

Line 68 in 7cd1ce3

isysroot_flag = ["-isysroot", config.osx_sysroot]

instead. If you need it to be different, add a new substitutions, such as %flang_fc1_noisysroot (or even better, only add -isysroot it to tests that need it).

Could you add details on when -isysroot is needed and when not? "some tests did not handle it appropriately" doesn't really explain anything. The change in target-enter-data-default-openmp52.f90 also seems to be different.

[Meinersbur]

Suggested change

	idx, driver = next(((i, subst)
	for i, subst in enumerate(config.substitutions) if "flang" in subst[0]))
	driver = next((subst
	for subst in config.substitutions if "flang" in subst[0]), None)

i is not used; don't fail on StopIteration if it is not in the list.

[Meinersbur]

So this is for this one test only? Could we limit the change for that test?

[parabola94]

Thank you for your feedback.
I was missing that clang tests handle -isysroot flag only if necessary. On the other hand, flang adds it to all tests if CMAKE_OSX_SYSROOT is set. In general, -isysroot flag is necessary for searching for headers and libraries. However, I was not sure which tests exactly need the flag, so I tried removing it in testing and found that all tests were passed.
Now I understand it would be better to handle the flag only if necessary.

[luporl]

However, I was not sure which tests exactly need the flag, so I tried removing it in testing and found that all tests were passed.

If I remove -isysroot from tests, I see the following failures:

check-flang:
  Flang :: Integration/iso-fortran-binding.cpp
check-flang-rt:
  flang-rt :: Driver/ctofortran.f90
  flang-rt :: Runtime/no-cpp-dep.c
  flang-rt-Unit :: Runtime/./RuntimeTests/NoArgv/FdateNotSupported (this test was already failing)

So, when not using DEFAULT_SYSROOT or SDKROOT, -isysroot is needed, so that the system libraries are found.

What is the issue with SDKROOT? Should the tests be using the SDK specified in it when it is set?
If so, this could be easily changed in llvm-project/flang/test/lit.cfg.py.

Since there are only 3 tests that need -isysroot, another option is to not include it by default and add a new substitution, such as %isysroot_flag, and then use it only in the tests above.

[luporl]

However, I was not sure which tests exactly need the flag, so I tried removing it in testing and found that all tests were passed.

If I remove -isysroot from tests, I see the following failures:

check-flang:
  Flang :: Integration/iso-fortran-binding.cpp
check-flang-rt:
  flang-rt :: Driver/ctofortran.f90
  flang-rt :: Runtime/no-cpp-dep.c
  flang-rt-Unit :: Runtime/./RuntimeTests/NoArgv/FdateNotSupported (this test was already failing)

So, when not using DEFAULT_SYSROOT or SDKROOT, -isysroot is needed, so that the system libraries are found.

Actually, these tests are all failing now. I hadn't noticed it because I always use DEFAULT_SYSROOT.
There are multiple issues:

• As @parabola94 mentioned in the linked issue, CMake doesn't set CMAKE_OSX_SYSROOT by default anymore, as it did when -isysroot was added to %flang, but only if SDKROOT is set. We will need to get its value from somewhere else. Compiler-rt build sets -isysroot, so we can probably reuse its logic. This can be done in a separate PR.
• In darwin-version.f90, -isysroot can't be used because it overrides the macosx version set by -target.
• iso-fortran-binding.cpp should be refactored, to build the test with %clang instead of trying to figure out where it is from %flang.

What is the issue with SDKROOT? Should the tests be using the SDK specified in it when it is set? If so, this could be easily changed in llvm-project/flang/test/lit.cfg.py.

From what I understood, setting SDKROOT when running CMake (4.0 or higher) sets CMAKE_OSX_SYSROOT, which adds -isysroot to %flang, overriding the macosx version set by -target, which makes darwin-version.f90 fail.

Since there are only 3 tests that need -isysroot, another option is to not include it by default and add a new substitution, such as %isysroot_flag, and then use it only in the tests above.

IMHO, considering the darwin-version.f90 issue, this seems the best option.
The idea of adding -isysroot to %flang was to make things work transparently on macOS.
But there are only a few tests that need it, and every now and then people forget about %flang_fc1 and use %flang -fc1 instead, which works on all other architectures.

[parabola94]

Thank you for your helpful comments.

Since there are only 3 tests that need -isysroot, another option is to not include it by default and add a new substitution, such as %isysroot_flag, and then use it only in the tests above.

IMHO, considering the darwin-version.f90 issue, this seems the best option. The idea of adding -isysroot to %flang was to make things work transparently on macOS. But there are only a few tests that need it, and every now and then people forget about %flang_fc1 and use %flang -fc1 instead, which works on all other architectures.

This approach seems good to me as well. I closed this PR because my approach has some problems.