On 08/06/2017 01:15 PM, Serge Pavlov wrote:
2017-08-06 6:43 GMT+07:00 Hal Finkel <hfin...@anl.gov <mailto:hfin...@anl.gov>>:


    On 07/24/2017 10:18 AM, Serge Pavlov wrote:

    I am thinking about reducing the patch further to leave only the
    ability to include config file when clang is called as
    `target-clang-drivermode`. It is still useful for cross
    compilation tasks because:
    - It is a convenient way to switch between supported targets,
    - SDK producer can ship compiler with a set of appropriate
    options or prepare them during installation.
    In this case if clang is called as `target-clang-drivermode`, it
    first tries to find file `target-drivermode.cfg` or `target.cfg`
    in a set of well-known directories, which in minimal case
    includes the directory where clang executable resides. If such
    file is found, options are  read from it, otherwise only option
    --target is added as clang does it now.

    This solution has obvious drawbacks:
    - User cannot specify config file in command line in the same way
    as he can choose a target: `clang --target <target>`,
    - On Windows symlinks are implemented as file copy, the solution
    looks awkward.
    So more or less complete solution needs to allow specifying
    config file in command line.

    I'd rather not reduce the patch in this way, and you didn't
    describe why you're considering reducing the patch. Can you please
    elaborate?


The only intent was to facilitate review process.

As someone who's worked on reviewing the patches, I don't think this makes things any easier or harder. Once we decide on what we want to do, the rest of the review process should be straightforward.


    Using `@file` has some problems. Config file is merely a set of
    options, just as file included by `@file`. Different include file
    search is only a convenience and could be sacrificed. Comments
    and unused option warning suppression could be extended for all
    files included with `@file`. The real problem is the search path.
    To be useful, config files must be searched for in well-known
    directories, so that meaning of `clang @config_fille` does not
    depend on the current directory. So clang must have some rule to
    distinguish between config file and traditional use of `@file`.
    For instance, if file name ends with `.cfg` and there is a file
    with this name in config search directories, this is a config
    file and it is interpreted a bit differently. Of course, the file
    may be specified with full path, but this way is inconvenient.

    I see no reason why we can't unify the processing but have
    different search-path rules for @file vs. --config file.


Now I think we can use @file without breaking compatibility.

libiberty resolves `file` in `@file` always relative to current directory. If such file is not found, it tries to open file with name `@file`. We must keep this behavior for the sake of compatibility. If after these steps `file` is not found and `file` does not contain directory separator, clang could try to treat `file` as config file and search it using special search path. If such solution is acceptable, we can get rid of `--config`.

I think that I'd prefer --config to this scheme. For one thing, it means that if I have a wrapper script that adds --config foo, this will break if the user happens to have a file named foo in their directory. I think that unifying the implementation of @foo and --config foo is a good idea, but combining them all into the same interface is not obviously optimal.

Thanks again,
Hal


    Another possible solution is to extend meaning of `--target` so
    that it fully matches with the use of `target-clang-drivermode`,
    that is the option `--target=hexagon` causes clang first to look
    for the file `hexagon.cfg` in well-known directories and use it
    if found. In this case treatment of `--target` is different if
    the option is specified in command line or in the content of
    config file (in the latter case it is processed as target name
    only), it may be confusing. Besides, use of config files is not
    restricted to the choice of target.

    I think we should do this, so long as the implementation is
    reasonable, and the special case doesn't bother me in this regard.
    I don't view this as a replacement for '--config file', however,
    because, as you mention, the config files need not be restricted
    to target triples.


Different treatment of `--target` in config file and in command line is still a concern, to do or not to do this depends on which is looks more intuitive. I would try implementing it is a separate patch.

Thanks,
--Serge


    Thanks again,
    Hal


    Using special option for config files does not bring risk of
    compatibility breakage and does not change meaning of existing
    options.


    Thanks,
    --Serge

    2017-05-10 11:25 GMT+07:00 Serge Pavlov <sepavl...@gmail.com
    <mailto:sepavl...@gmail.com>>:

        2017-05-10 3:46 GMT+07:00 Richard Smith
        <rich...@metafoo.co.uk <mailto:rich...@metafoo.co.uk>>:

            On 1 March 2017 at 02:50, Serge Pavlov via Phabricator
            <revi...@reviews.llvm.org
            <mailto:revi...@reviews.llvm.org>> wrote:


                Format of configuration file is similar to file used
                in the construct `@file`, it is a set of options.
                Configuration file have advantage over this construct:

                - it is searched for in well-known places rather than
                in current directory,


            This (and suppressing unused-argument warnings) might
            well be sufficient to justify a different command-line
            syntax rather than @file...


        Construct `@file` in this implementation is used only to read
        parts of config file inside containing file. Driver knows
        that it processes config file and can adjust treatment of
        `@file`. On the other hand, driver might parse config files
        in a more complicated way, for instance, it could treat line
        `# include(file_name)` as a command to include another file.

                - it may contain comments, long options may be split
                between lines using trailing backslashes,
                - other files may be included by `@file` and they
                will be resolved relative to the including file,


            ... but I think we should just add these extensions to
            our @file handling, and then use the exact same syntax
            and code to handle config files and @file files. That is,
            the difference between @ and --config would be that the
            latter looks in a different directory and suppresses
            "unused argument" warnings, but they would otherwise be
            identical.


        Changing treatment of `@file` can cause compatibility issues,
        in particular, both libiberty and cl resolves file name
        relative to current directory. So driver must deduce that
        `@file` is used to load config file rather than merely to
        organize arguments. Another difference is that `@file`
        inserts its content in the place where it occurs, while
        `--config` always puts arguments before user specified
        options. The following invocations:

            clang --config a.cfg -opt1 -opt2 file1.cpp
            clang -opt1 -opt2 file1.cpp --config a.cfg

        are equivalent, but variants with `@file` can have different
        effect.


                - the file may be encoded in executable name,
                - unused options from configuration file do not
                produce warnings.


                https://reviews.llvm.org/D24933
                <https://reviews.llvm.org/D24933>







-- Hal Finkel
    Lead, Compiler Technology and Programming Languages
    Leadership Computing Facility
    Argonne National Laboratory



--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to