[PATCH] D24933: Enable configuration files in clang

[Serge Pavlov via cfe-commits]

sepavloff marked 5 inline comments as done.
sepavloff added a comment.

> Rather than inventing a new mechanism, could we extend our existing `@file` facility to support comments and nested inclusion of further `@file`s?

Reusing `@file` is an attractive idea, but it cannot be implemented due to compatibility reason. The file in `@file` is resolved relative to current directory in both msvc and libiberty. Current directory may change during build, so use of `@file` to deliver set of flags to every invocation of clang becomes unrealistic. Config files instead are taken from well-known places and if a user set `CFLAGS='--config abc.cfg'` in call to make, it should work as intended.

> For `clang --config myfile.cfg`, should it also search the current working directory?

I believe no, current directory is not reliable place for configurations, as it may change during build.

> Whoever makes the `blah-clang` symlink should get to control what the default configuration for `blah-clang` is, I think.

The patch is changed so that config file for `blah-clang` is searched for *only* in the directory where `blah-clang` resides. It prevents a user from overwriting 'system' config files. The idea is that the files in the binary directory are prepared by SDK suppliers who adapt clang for specific needs.

> I'm not keen on it searching in `~/.llvm`.

The patch is changed so that `~/.llvm` is searched only of config files specified explicitly by `--config`. A user can specify full path to the config file: `clang --config ~/.llvm/abc.cfg`. By making the search in `~/.llvm` we make work with config files a bit more convenient.

> you can replace a blah-clang symlink with a shell script containing `exec clang @blah.cfg "@$"`

Due to intermediate shell the environment variables which were not exported would be lost. This solution is OK for build system but from viewpoint of compiler, which must work in any build system, it is too fragile.



================
Comment at: docs/UsersManual.rst:667
+extension `cfg` if it is not specified yet, and the obtained file name is searched
+for in the directories: `~/.llvm` and the directory where clang executable resides.
+The first found file is used. It is an error if the required file cannot be found.
----------------
srhines wrote:
> rsmith wrote:
> > hans wrote:
> > > The "add .cfg extension" magic seems a little awkward. It seems this is mixing the possibility of taking a filename and taking some other name.
> > > 
> > > For `clang --config myfile.cfg`, should it also search the current working directory?
> > > 
> > > I'm not keen on it searching in `~/.llvm`.
> > I'm also not keen on searching `~/.llvm`. Whoever makes the `blah-clang` symlink should get to control what the default configuration for `blah-clang` is, I think. But then this seems to immediately lead to the conclusion that we don't need this implicit-config-file feature at all, since you can replace a `blah-clang` symlink with a shell script containing `exec clang @blah.cfg "@$"` -- and it's better to handle it that way, since you get more control over where the config file lives.
> Android is essentially taking the shell script wrapper approach today (although we are using python, and we don't do a lot with it just yet).
Agree, removed this feature.


================
Comment at: docs/UsersManual.rst:694
+the including file. For instance if a config file `~/.llvm/target.cfg` contains
+directive `os/linux.opts`, the file `linux.opts` is searched for in the directory
+`~/.llvm/os`.
----------------
hans wrote:
> I guess you mean directive `@os/linux.opts`?
Yes, thank you for the catch.


================
Comment at: test/Driver/config-file.c:10
+
+// RUN: not %clang --config inexistent.cfg -c %s -### 2>&1 | FileCheck %s -check-prefix CHECK-INEX-NOSEARCH
+// CHECK-INEX-NOSEARCH: Configuration {{.*}}inexistent.cfg' specified by option '--config' cannot be found in directories:
----------------
hans wrote:
> This test will fail if I have a `~/.llvm/inexistent.cfg` file on my machine.
Changed the name to more long and awkward to reduce chance of clash.


================
Comment at: test/Driver/config-file.c:13
+// CHECK-INEX-NOSEARCH: ~/.llvm
+// CHECK-INEX-NOSEARCH: /etc/llvm
+
----------------
hans wrote:
> Where did `/etc/llvm` come from?
Remnants of the previous variant, removed.


================
Comment at: test/Driver/config-file.c:14
+// CHECK-INEX-NOSEARCH: /etc/llvm
+
+// RUN: %clang --config %S/Inputs/config-2.cfg -### 2>&1 | FileCheck %s -check-prefix CHECK-HHH-NOFILE
----------------
hans wrote:
> There is no test checking the functionality that finds a config file based on the clang executable name.
Added tests (file config-file2.c).


https://reviews.llvm.org/D24933