[Clang] Added functionality to provide config file name via env variable

[jolanta.jensen]

Summary

Also added functionality to set the name of the config file env variable
via CMake.

[MaskRay]

Behaviors due to a new environment variable should be very careful. How is this useful? If you want this, you can add a wrapper around clang to specify --config= by yourself.

Think about this: if the user sets CONFIG_FILE= to something, it will be difficult for toolchain maintainers to triage a bug.

[jolanta.jensen]

Extended the test to cover Windows OS.

[jolanta.jensen]

In D144179#4132597, @MaskRay wrote:

Behaviors due to a new environment variable should be very careful. How is this useful? If you want this, you can add a wrapper around clang to specify --config= by yourself.

Think about this: if the user sets CONFIG_FILE= to something, it will be difficult for toolchain maintainers to triage a bug.

The feature is aimed at system administrators rather than an individual user. It simplifies system configuration so the user does not need to configure anything him/herself.

[jolanta.jensen]

Adjusted the test for Windows OS. Once again.

[jolanta.jensen]

Adjusting the test for Windows OS again.

[MaskRay]

This looks like introducing a footgun (when something behaves differently from an upstream Clang, it would be difficult for toolchain maintainers to know why).
Why can't your user specify CLANG_CONFIG_FILE_SYSTEM_DIR?

[jolanta.jensen]

In D144179#4146599, @MaskRay wrote:

This looks like introducing a footgun (when something behaves differently from an upstream Clang, it would be difficult for toolchain maintainers to know why).
Why can't your user specify CLANG_CONFIG_FILE_SYSTEM_DIR?

Because I don't want the user to be specifying any flags compiling. The feature is intended to be used by a system administrator who will configure the system for the user so no compile flags are necessary. That way the machines can be configured the same way for all users.
Since there were concerns about visibility of what configuration is used, I checked the driver crash report generated when a configuration file set via an environmental variable is used. The crash report will print the configuration file used and the flags specified in the configuration file will be appended to the driver args in the crash reproducer.
And finally, you already use an env variable, CLANG_NO_DEFAULT_CONFIG, to disable use of default configuration so it isn't really an introduction but rather an extension of what is already used.

[jolanta.jensen]

Adjusting the test for Windows. Setting the read bits only for the config file as Windows does not have full support for chmod.

[jolanta.jensen]

Test adjustment for Windows. Disabling read permissions using chmod does not seem to work on Windows.

[mgabka]

In D144179#4146599, @MaskRay wrote:

This looks like introducing a footgun (when something behaves differently from an upstream Clang, it would be difficult for toolchain maintainers to know why).
Why can't your user specify CLANG_CONFIG_FILE_SYSTEM_DIR?

hi @MaskRay,
The reason we wanted to suggest use of environment variable is that the CLANG_CONFIG_FILE_SYSTEM_DIR is only defined at compilation time, after discussing it once again we would rather lean towards introducing an environment variable with similar semantics as CLANG_CONFIG_FILE_SYSTEM_DIR or rather `CLANG_CONFIG_FILE_USER_DIR`, the motivation here is that it will allow to specify the directory to search for config files in a dynamic way, without need to recompile the compiler.
It is for user convenience in situations when they are using a system wide installation in a location where they do not have access right, and the `CLANG_CONFIG_FILE_SYSTEM_DIR and CLANG_CONFIG_FILE_USER_DIR` were not defined at build time.

We realised that environment variables are already used in this area, for example CLANG_NO_DEFAULT_CONFIG, so adding another one is not breaking existing convention.

What do you think about it?

[MaskRay]

In D144179#4166909, @mgabka wrote:

In D144179#4146599, @MaskRay wrote:

This looks like introducing a footgun (when something behaves differently from an upstream Clang, it would be difficult for toolchain maintainers to know why).
Why can't your user specify CLANG_CONFIG_FILE_SYSTEM_DIR?

hi @MaskRay,
The reason we wanted to suggest use of environment variable is that the CLANG_CONFIG_FILE_SYSTEM_DIR is only defined at compilation time, after discussing it once again we would rather lean towards introducing an environment variable with similar semantics as CLANG_CONFIG_FILE_SYSTEM_DIR or rather `CLANG_CONFIG_FILE_USER_DIR`, the motivation here is that it will allow to specify the directory to search for config files in a dynamic way, without need to recompile the compiler.
It is for user convenience in situations when they are using a system wide installation in a location where they do not have access right, and the `CLANG_CONFIG_FILE_SYSTEM_DIR and CLANG_CONFIG_FILE_USER_DIR` were not defined at build time.

We realised that environment variables are already used in this area, for example CLANG_NO_DEFAULT_CONFIG, so adding another one is not breaking existing convention.

What do you think about it?

Environment variables are evil and make troubleshooting difficult. CLANG_NO_DEFAULT_CONFIG disables configuration files and makes the behavior is less magical, so I am fine with it.
It's different from the environment variable being introduced in this patch.

If your users want to use a system installed clang with an environment variable, why not give them a Clang wrapper like:

#!/bin/zsh
exec path/to/clang ${CONFIG_FILE:+--config=$CONFIG_FILE} "$@"

You can even replace the current clang symlink with the wrapper.

clang/include/clang/Config/config.h.cmake

 
 /* Directories clang will search for headers */
 #define C_INCLUDE_DIRS "${C_INCLUDE_DIRS}"
 
 /* Directories clang will search for configuration files */
 #cmakedefine CLANG_CONFIG_FILE_SYSTEM_DIR "${CLANG_CONFIG_FILE_SYSTEM_DIR}"
 #cmakedefine CLANG_CONFIG_FILE_USER_DIR "${CLANG_CONFIG_FILE_USER_DIR}"
 
+/* Name of the env variable for the configuration file */
+#cmakedefine ENV_CONFIG_FILE_NAME "${ENV_CONFIG_FILE_NAME}"
+
 /* Default <path> to all compiler invocations for --sysroot=<path>. */
 #define DEFAULT_SYSROOT "${DEFAULT_SYSROOT}"
 
 /* Directory where gcc is installed. */
 #define GCC_INSTALL_PREFIX "${GCC_INSTALL_PREFIX}"
 
 /* Define if we have libxml2 */
 #cmakedefine CLANG_HAVE_LIBXML ${CLANG_HAVE_LIBXML}

clang/include/clang/Driver/Driver.h

private:
   bool loadConfigFiles();
 
   /// Tries to load options from default configuration files (deduced from
   /// executable filename).
   ///
   /// \returns true if error occurred.
   bool loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx);
 
+  /// Tries to load options from configuration file specified by environment
+  //  variable.
+  //
+  //  \returns true if error occurred.
+  bool readConfigFileFromEnv();
+
   /// Read options from the specified file.
   ///
   /// \param [in] FileName File to read.
   /// \param [in] Search and expansion options.
   /// \returns true, if error occurred while reading.
   bool readConfigFile(StringRef FileName, llvm::cl::ExpansionContext &ExpCtx);
 
   /// Set the driver mode (cl, gcc, etc) from the value of the `--driver-mode`

clang/lib/Driver/Driver.cpp

for (auto *Opt : *NewOptions) {
         BaseArg = nullptr;
       appendOneArg(*CfgOptions, Opt, BaseArg);
     }
   }
   ConfigFiles.push_back(std::string(CfgFileName));
   return false;
 }
 
+bool Driver::readConfigFileFromEnv() {
+  llvm::cl::ExpansionContext ExpCtx(Saver.getAllocator(),
+                                    llvm::cl::tokenizeConfigFile);
+  ExpCtx.setVFS(&getVFS());
+  std::string EnvCfgName = "CONFIG_FILE";
+
+#if defined(ENV_CONFIG_FILE_NAME)
+  EnvCfgName = ENV_CONFIG_FILE_NAME;
+#endif
+  const char *EnvCfgFileName = getenv(EnvCfgName.c_str());
+
+  if (!EnvCfgFileName)
+    return false;
+
+  std::string CfgFileName = EnvCfgFileName;
+  if (CfgFileName.empty()) {
+    Diag(diag::err_drv_cannot_read_config_file) << CfgFileName << "";
+    return true;
+  }
+
+  // If argument contains directory separator, treat it as a path to
+  // configuration file.
+  SmallString<128> CfgFilePath;
+  if (llvm::sys::path::is_relative(CfgFileName))
+    llvm::sys::fs::current_path(CfgFilePath);
+  llvm::sys::path::append(CfgFilePath, CfgFileName);
+  return readConfigFile(CfgFilePath, ExpCtx);
+}
+
 bool Driver::loadConfigFiles() {
   llvm::cl::ExpansionContext ExpCtx(Saver.getAllocator(),
                                     llvm::cl::tokenizeConfigFile);
   ExpCtx.setVFS(&getVFS());
 
   // Process options that change search path for config files.
   if (CLOptions) {
     if (CLOptions->hasArg(options::OPT_config_system_dir_EQ)) {
for (auto CfgFileName : CLOptions->getAllArgValues(options::OPT_config)) {
       }
 
       // Try to read the config file, return on error.
       if (readConfigFile(CfgFilePath, ExpCtx))
         return true;
     }
   }
 
+  if (ConfigFiles.empty())
+    return readConfigFileFromEnv();
+
   // No error occurred.
   return false;
 }
 
 bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
   // Disable default config if CLANG_NO_DEFAULT_CONFIG is set to a non-empty
   // value.
   if (const char *NoConfigEnv = ::getenv("CLANG_NO_DEFAULT_CONFIG")) {

clang/test/Driver/config-file-from-env.c

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/empty_dir
+// RUN: echo "-Wall" > %t/c.cfg
+// RUN: echo "-ffast-math" > %t/cxx.cfg
+// RUN: touch %t/unreadable.cfg
+// RUN: chmod a-r %t/unreadable.cfg
+// RUN: touch %t/test.c
+// RUN: touch %t/test.cpp
+
+// RUN: env CONFIG_FILE=%t/c.cfg %clang -S %t/test.c -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-C1
+// CHECK-C1: Configuration file: {{.*[\/\\]}}c.cfg
+// CHECK-C1: -Wall
+
+// RUN: env CONFIG_FILE=%t/unreadable.cfg not %clang -S %t/test.c -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-C2
+// CHECK-C2: error: cannot read configuration file '{{.*[\/\\]}}unreadable.cfg'
+
+// RUN: env CONFIG_FILE=%t/c.cfg %clang -S %t/test.c --config %S/Inputs/config-1.cfg -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-C3
+// CHECK-C3: Configuration file: {{.*[\/\\]}}config-1.cfg
+// CHECK-C3: -Werror
+// CHECK-C3-NOT: -Wall
+
+// RUN: env CONFIG_FILE= not %clang -S %t/test.c -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-C4
+// CHECK-C4: error: cannot read configuration file ''
+
+// RUN: env CONFIG_FILE=%t/empty_dir not %clang -S %t/test.c -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-C5
+// CHECK-C5: error: configuration file '{{.*[\/\\]}}empty_dir' cannot be opened: not a regular file
+
+// RUN: env CONFIG_FILE=%t/cxx.cfg %clang++ -S %t/test.cpp -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-CXX1
+// CHECK-CXX1: Configuration file: {{.*[\/\\]}}cxx.cfg
+// CHECK-CXX1: -ffast-math
+
+// RUN: env CONFIG_FILE=%t/unreadable.cfg not %clang++ -S %t/test.cpp -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-CXX2
+// CHECK-CXX2: error: cannot read configuration file '{{.*[\/\\]}}unreadable.cfg'
+
+// RUN: env CONFIG_FILE=%t/cxx.cfg %clang++ -S %t/test.cpp --config %S/Inputs/config-1.cfg -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-CXX3
+// CHECK-CXX3: Configuration file: {{.*[\/\\]}}config-1.cfg
+// CHECK-CXX3: -Werror
+// CHECK-CXX3-NOT: -ffast-math
+
+// RUN: env CONFIG_FILE= not %clang++ -S %t/test.cpp -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-CXX4
+// CHECK-CXX4: error: cannot read configuration file ''
+
+// RUN: env CONFIG_FILE=%t/empty_dir not %clang++ -S %t/test.cpp -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-CXX5
+// CHECK-CXX5: error: configuration file '{{.*[\/\\]}}empty_dir' cannot be opened: not a regular file
+
+// RUN: env CONFIG_FILE=%t/c.cfg %clang++ --config=%t/cxx.cfg -S %t/test.cpp -o /dev/null -### 2>&1 | FileCheck %s -check-prefix CHECK-CXX6
+// CHECK-CXX6-NOT: Configuration file: {{.*[\/\\]}}c.cfg
+// CHECK-CXX6-NOT: -Wall
+
+// RUN: rm -rf %t