[flang][driver] Add support for `-I`

[FarisRehman]

Summary

Add support for option -I in the new Flang driver.
This will allow for included headers and module files in other directories, as the default search path is currently the working folder.
The behaviour of this is consistent with the current f18 driver, where the current folder (i.e. ".") has the highest priority followed by the order of '-I's taking priority from first to last.

Summary of changes:

• Add SearchDirectoriesFromDashI to PreprocessorOptions, to be forwarded into the parser's searchDirectories
• Add header files and non-functional module files to be used in regression tests. The module files are just text files and are used to demonstrated that paths specified with -I are taken into account when searching for .mod files.

Depends on: D93401

[awarzynski]

Hi @FarisRehman ! Thank you for working on this! Could you add tests?

clang/lib/Driver/ToolChains/Flang.cpp

75

This would ideally go to a dedicated method for parsing preprocessor options.

flang/include/flang/Frontend/PreprocessorOptions.h

13

[nit] I'd be tempted to be more explicit here, e.g. SearchDirectoriesFromDashI;.

flang/lib/Frontend/CompilerInvocation.cpp

170

Lower case, sadly.

240–243

_Moving_ this code seems like _unrelated_ change.

[FarisRehman]

Address review comments

Summary of changes:

• Add a regression test for -I
• Move claiming of -I option to the dedicated parsing preprocessing options method
• Rename SearchDirectoriesFromI to SearchDirectoriesFromDashI
• Undo unrelated changes

[FarisRehman]

Clean up test

Summary of changes:

• Clean up test include_header.f90
• Rename SearchDirectoriesFromDashI to searchDirectoriesFromDashI

[awarzynski]

I left a few nits, but otherwise LGTM!

clang/lib/Driver/ToolChains/Flang.cpp

22

[nit] This probably can be merged with the line above.

flang/include/flang/Frontend/PreprocessorOptions.h

13

I think that once the new driver supports more search-path related options (rather than just -I), it will make sense to extract these into a separate class. That's not really needed just now, but it's probably worth adding a comment. Perhaps something like this:

// TODO: When adding support for more options related to search paths, consider collecting them in a separate aggregate. For now we keep it here as there is no point creating a class for just one field.

flang/lib/Frontend/CompilerInvocation.cpp

247–250

If these are std::vectors (which I believe is the case), then you can simplify:

fortranOptions.searchDirectories.insert(
    fortranOptions.searchDirectories.end(), 
    preprocessorOptions.searchDirectoriesFromDashI.begin(),
    preprocessorOptions.searchDirectoriesFromDashI.end());

flang/test/Flang-Driver/driver-help.f90

27 ↗

(On Diff #313536)

The help message for -I is _very_ long :) I wonder, perhaps you could just keep Add directory to include search path.?

The purpose of this test file is to verify that -help only displays the supported options. Technically speaking only -I is needed for that :) (but that could be _too_ short)

flang/test/Flang-Driver/include_header.f90

33 ↗

(On Diff #313536)

[nit] IMO the naming is a bit unfortunate, e.g. there's include and included on one line. Perhaps: basic-test-header.h?

[sscalpone]

Flang supports include files and module files. The search path is used to look for both include files and module files. The help text doesn't include info about module files.

The help text refers to the -isystem option. Is that implemented for flang, and if so, ought tests be added to make sure -I and -isystem co-exist as documented for flang?

Not that there's also a special include directory for intrinsic modules. Intrinsic modules are treated specially, and right now, they are special by virtue of being in a special directory. In the throwaway driver, the option is -intrinsic-module-directory.

There's also the -module option to specify the directory into which newly created module files are written. I don't recall if flang or other compilers generally add that directory to the search path as well.

[awarzynski]

In D93453#2481720, @sscalpone wrote:

Flang supports include files and module files. The search path is used to look for both include files and module files. The help text doesn't include info about module files.

Good point - the help message is inaccurate. It comes from Options.td, which currently is shared with Clang: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td#L650-L657. It documents Clang's internal implementation. IMO Options.td should not reveal internal implementation. In fact, that information is duplicated here: https://clang.llvm.org/docs/ClangCommandLineReference.html#include-path-management. I suggested a fix here: https://reviews.llvm.org/D94169.

The help text refers to the -isystem option. Is that implemented for flang, and if so, ought tests be added to make sure -I and -isystem co-exist as documented for flang?

-isystem is neither implemented nor planned in the near future for the new Flang driver [2]. Also, the updated help message won't contain any references to -isystem.

Not that there's also a special include directory for intrinsic modules. Intrinsic modules are treated specially, and right now, they are special by virtue of being in a special directory. In the throwaway driver, the option is -intrinsic-module-directory.

There's also the -module option to specify the directory into which newly created module files are written. I don't recall if flang or other compilers generally add that directory to the search path as well.

Yea, we are aware of that. As proposed in [1] and [2]:

• -intrinsic-module-directory will be replaced with -fintrinsic-modules-path
• -module will be replaced with -J/-module-dir

There will be separate patches for these options.

[1] [flang-dev] RFC: new Flang driver options: https://lists.llvm.org/pipermail/flang-dev/2020-November/000588.html
[2] LLVM/flang new driver options: https://docs.google.com/spreadsheets/d/1JRe39lP_KhtkYxFEIvwrCFlE5v1Ofa_krOHI-XXXWPY/edit#gid=0

[FarisRehman]

Address minor changes in review comments

Summary of changes

• Add a TODO for future search-path related options
• Change the text which the driver-help tests look for, which matches the help message in https://reviews.llvm.org/D94169
• Rename the test header

[FarisRehman]

Rename test files

Rename test files to use dashes instead of underscores, as other test files do.

[FarisRehman]

Add a new test

Update the regression test to check the behaviour of multiple -I's specified.

Summary of changes:

• Update regression test with new checks
• Fix regression tests that broke when patch D93712 was implemented

[tskeith]

flang/test/Flang-Driver/include-header.f90

59 ↗

(On Diff #316423)

-I also is supposed to affect INCLUDE lines so it would be good to have a test for that too. They are handled during preprocessing and so can be tested the same way.

It also affects searching for .mod files but I think to test that requires semantics (i.e. to report an error if the module can't be found). It would be good to test that somewhere too.

[sameeranjoshi]

flang/test/Flang-Driver/include-header.f90

38 ↗

(On Diff #316423)

Nit - Shouldn't this be Inputs/ starting with a / changes the meaning(at least on Linux systems) ?

[FarisRehman]

flang/test/Flang-Driver/include-header.f90

38 ↗

(On Diff #316423)

I've fixed this, thanks.

59 ↗

(On Diff #316423)

Thanks, I've updated the test to check INCLUDE lines and added a new test for module files.
I've added some .mod files to the Inputs directory, however these files are not real module files and are just there for the test.

[FarisRehman]

Add include-module test

Update the regression test to check the behaviour of -I with an INCLUDE line in the source code.
Also add a regression test to check the behaviour of -I with module files.

Summary of changes:

• Update include-header.f90 to check INCLUDE
• Add include-module.f90 to check the behaviour with module files
• Rename SecondaryInputs directory to header-dir and create a module-dir directory

[awarzynski]

Thank you @FarisRehman , LGTM!

I believe that you addressed all PR comments. As requested, there are tests for:

• #include
• INCLUDE
• USE

Your tests also demonstrate that the order of include paths is significant and that that's preserved by the driver. Nice!

clang/include/clang/Driver/Options.td

 def G_EQ : Joined<["-"], "G=">, Flags<[NoXarchOption]>, Group<m_Group>, Alias<G>;
 def H : Flag<["-"], "H">, Flags<[CC1Option]>, Group<Preprocessor_Group>,
     HelpText<"Show header includes and nesting depth">,
     MarshallingInfoFlag<"DependencyOutputOpts.ShowHeaderIncludes">;
 def I_ : Flag<["-"], "I-">, Group<I_Group>,
     HelpText<"Restrict all prior -I flags to double-quoted inclusion and "
              "remove current directory from include path">;
 def I : JoinedOrSeparate<["-"], "I">, Group<I_Group>,
-    Flags<[CC1Option,CC1AsOption]>, MetaVarName<"<dir>">,
+    Flags<[CC1Option,CC1AsOption,FlangOption,FC1Option]>, MetaVarName<"<dir>">,
     HelpText<"Add directory to include search path. If there are multiple -I "
              "options, these directories are searched in the order they are "
              "given before the standard system directories are searched. "
              "If the same directory is in the SYSTEM include search paths, for "
              "example if also specified with -isystem, the -I option will be "
              "ignored">;
 def L : JoinedOrSeparate<["-"], "L">, Flags<[RenderJoined]>, Group<Link_Group>,
     MetaVarName<"<dir>">, HelpText<"Add directory to library search path">;

clang/lib/Driver/ToolChains/Flang.cpp

 //===-- Flang.cpp - Flang+LLVM ToolChain Implementations --------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 
 #include "Flang.h"
 #include "CommonArgs.h"
 
 #include "clang/Driver/Options.h"
 
 #include <cassert>
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 void Flang::ConstructJob(Compilation &C, const JobAction &JA,
                          const InputInfo &Output, const InputInfoList &Inputs,
                          const ArgList &Args, const char *LinkingOutput) const {
   const auto &TC = getToolChain();
   const llvm::Triple &Triple = TC.getEffectiveTriple();
   const std::string &TripleStr = Triple.getTriple();
 
   const auto &D = C.getDriver();
   ArgStringList CmdArgs;
void Flang::ConstructJob(Compilation &C, const JobAction &JA,
   }
 
   types::ID InputType = Input.getType();
   if (types::getPreprocessedType(InputType) != types::TY_INVALID) {
     Args.AddAllArgs(CmdArgs, {options::OPT_D, options::OPT_U});
   }
   Args.ClaimAllArgs(options::OPT_D);
 
+  Args.AddAllArgs(CmdArgs, options::OPT_I);
+
   if (Output.isFilename()) {
     CmdArgs.push_back("-o");
     CmdArgs.push_back(Output.getFilename());
   } else {
     assert(Output.isNothing() && "Invalid output.");
   }
 
   assert(Input.isFilename() && "Invalid input.");

flang/include/flang/Frontend/PreprocessorOptions.h

 #ifndef LLVM_FLANG_PREPROCESSOROPTIONS_H
 #define LLVM_FLANG_PREPROCESSOROPTIONS_H
 
 #include "llvm/ADT/StringRef.h"
 
 namespace Fortran::frontend {
 
 /// PreprocessorOptions - This class is used for passing the various options
 /// used in preprocessor initialization to the parser options.
 class PreprocessorOptions {
 public:
   std::vector<std::pair<std::string, bool /*isUndef*/>> Macros;
+  std::vector<std::string> SearchDirectoriesFromI;
 
 public:
   PreprocessorOptions() {}
 
   void addMacroDef(llvm::StringRef Name) {
     Macros.emplace_back(std::string(Name), false);
   }
 
   void addMacroUndef(llvm::StringRef Name) {
     Macros.emplace_back(std::string(Name), true);
   }
 };
 
 } // namespace Fortran::frontend
 
 #endif // LLVM_FLANG_PREPROCESSOROPTIONS_H
 No newline at end of file

flang/lib/Frontend/CompilerInvocation.cpp

 //===- CompilerInvocation.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
static void ParsePreprocessorArgs(
   for (const auto *A : args.filtered(
            clang::driver::options::OPT_D, clang::driver::options::OPT_U)) {
     if (A->getOption().matches(clang::driver::options::OPT_D)) {
       opts.addMacroDef(A->getValue());
     } else {
       opts.addMacroUndef(A->getValue());
     }
   }
+
+  // Add the ordered list of -I's.
+  for (const auto *A : args.filtered(clang::driver::options::OPT_I))
+    opts.SearchDirectoriesFromI.emplace_back(A->getValue());
 }
 
 bool CompilerInvocation::CreateFromArgs(CompilerInvocation &res,
     llvm::ArrayRef<const char *> commandLineArgs,
     clang::DiagnosticsEngine &diags) {
 
   bool success = true;
 
opts.predefinitions.emplace_back(
         MacroName, std::optional<std::string>(MacroBody.str()));
   }
 }
 
 void CompilerInvocation::SetFortranOpts() {
   auto &fortranOptions = fortranOpts();
   const auto &preprocessorOptions = preprocessorOpts();
 
-  // These defaults are based on the defaults in f18/f18.cpp.
-  std::vector<std::string> searchDirectories{"."s};
-  fortranOptions.searchDirectories = searchDirectories;
   fortranOptions.isFixedForm = false;
 
   collectMacroDefinitions(fortranOptions, preprocessorOptions);
+
+  // These defaults are based on the defaults in f18/f18.cpp.
+  std::vector<std::string> searchDirectories{"."s};
+  for (const auto &searchDirectoryFromI :
+      preprocessorOptions.SearchDirectoriesFromI) {
+    searchDirectories.emplace_back(searchDirectoryFromI);
+  }
+  fortranOptions.searchDirectories = searchDirectories;
 }