[clang][test] Disallow using the default module cache path in lit tests

[benlangmuir]

Summary

Make the default module cache path invalid when running lit tests so
that tests are forced to provide a cache path. This avoids accidentally
escaping to the system default location, and would have caught the
failure recently found in ClangScanDeps/multiple-commands.c.

[bruno]

Awesome!

[benlangmuir]

Reverted due to failure on a bot https://lab.llvm.org/buildbot/#/builders/214/builds/3252

I'm not sure how to deal with missing env -u.

• We could do env CLANG_MODULE_CACHE_PATH= and change the compiler's interpretation of empty string for this variable. I'm not sure if the current behaviour (there will be no module cache in the cc1 at all) is intentional or useful. Hesitant to change this behaviour.
• We could try to enumerate all the environments that don't support env -u and disable these two tests on those platforms. So far it was just one AIX bot, but I wouldn't be surprised if other less commonly used unixes have the same issue.
• We could make the command inscrutable, like not env -u X true || env -u ... real command ... so that if env -u X true fails (presumably due to not supporting -u option) we won't run the rest of the RUN line.

If someone has a suggestion for a simple fix, I can try again. But otherwise I doubt it's worth putting much time into this.

[bruno]

I'm not sure how to deal with missing env -u.

• We could do env CLANG_MODULE_CACHE_PATH= and change the compiler's interpretation of empty string for this variable. I'm not sure if the current behaviour (there will be no module cache in the cc1 at all) is intentional or useful. Hesitant to change this behaviour.

How about using self.with_environment('CLANG_MODULE_CACHE_PATH', '') so we don't need to worry about using env -u to unset? My understand is that (1) getDefaultModuleCachePath is the only place using CLANG_MODULE_CACHE_PATH, and (2) std::getenv return nullptr if it's empty, which will fallback to system provided path instead.

• We could try to enumerate all the environments that don't support env -u and disable these two tests on those platforms. So far it was just one AIX bot, but I wouldn't be surprised if other less commonly used unixes have the same issue.

• We could make the command inscrutable, like not env -u X true || env -u ... real command ... so that if env -u X true fails (presumably due to not supporting -u option) we won't run the rest of the RUN line.

Not sure it helps much but according to option-X.test, system-aix support unset.

If someone has a suggestion for a simple fix, I can try again. But otherwise I doubt it's worth putting much time into this.

Thanks for trying to improve this :)

[benlangmuir]

In D133622#3788218, @bruno wrote:

I'm not sure how to deal with missing env -u.

• We could do env CLANG_MODULE_CACHE_PATH= and change the compiler's interpretation of empty string for this variable. I'm not sure if the current behaviour (there will be no module cache in the cc1 at all) is intentional or useful. Hesitant to change this behaviour.

How about using self.with_environment('CLANG_MODULE_CACHE_PATH', '') so we don't need to worry about using env -u to unset? My understand is that (1) getDefaultModuleCachePath is the only place using CLANG_MODULE_CACHE_PATH, and (2) std::getenv return nullptr if it's empty, which will fallback to system provided path instead.

Where are you thinking we would call self.with_environment in this case? We explicitly do not want the system-provided path in most tests. I think we would need to set it to None, since

(2) std::getenv return nullptr if it's empty, which will fallback to system provided path instead.

getenv returns empty string, not nullptr.

Not sure it helps much but according to option-X.test, system-aix support unset.

Heh, I'm worried we'll just hit an issue with a different platform (Windows?). If we can't find a better fix I guess I can at least attempt it and see what breaks.

clang/test/Driver/modules-cache-path.m

-// RUN: %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-DEFAULT
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-DEFAULT
 // CHECK-DEFAULT: -fmodules-cache-path={{.*}}clang{{[/\\]+}}ModuleCache
 
 // RUN: env CLANG_MODULE_CACHE_PATH=/dev/null \
 // RUN:   %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=OVERRIDE
 // OVERRIDE: -fmodules-cache-path=/dev/null
 
 // RUN: env CLANG_MODULE_CACHE_PATH= \
 // RUN:   %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=DISABLE
 // DISABLE-NOT: -fmodules-cache-path=

clang/test/Modules/driver.c

-// RUN: %clang -fmodules -fimplicit-module-maps %s -### 2>&1 | FileCheck -check-prefix CHECK-NO_MODULE_CACHE %s
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -fimplicit-module-maps %s -### 2>&1 | FileCheck -check-prefix CHECK-NO_MODULE_CACHE %s
 // RUN: %clang -fmodules -fimplicit-module-maps -fmodules-cache-path=blarg %s -### 2>&1 | FileCheck -check-prefix CHECK-WITH_MODULE_CACHE %s
 
 // CHECK-NO_MODULE_CACHE: {{clang.*"-fmodules-cache-path=.*ModuleCache"}}
 
 // CHECK-WITH_MODULE_CACHE: {{clang.*"-fmodules-cache-path=blarg"}}

llvm/utils/lit/lit/llvm/config.py

def use_clang(self, additional_tool_dirs=[], additional_flags=[],
             'LIBCLANG_CODE_COMPLETION_LOGGING',
             ]
         # Clang/Win32 may refer to %INCLUDE%. vsvarsall.bat sets it.
         if platform.system() != 'Windows':
             possibly_dangerous_env_vars.append('INCLUDE')
 
         self.clear_environment(possibly_dangerous_env_vars)
 
+        # Make the default module cache path invalid so that tests are forced to
+        # provide a cache path if they use implicit modules.
+        self.with_environment('CLANG_MODULE_CACHE_PATH', '/dev/null')
+
         # Tweak the PATH to include the tools dir and the scripts dir.
         # Put Clang first to avoid LLVM from overriding out-of-tree clang
         # builds.
         exe_dir_props = [self.config.name.lower() + '_tools_dir',
                          'clang_tools_dir', 'llvm_tools_dir']
         paths = [getattr(self.config, pp) for pp in exe_dir_props
                  if getattr(self.config, pp, None)]
         paths = additional_tool_dirs + paths