[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

79

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

flang/include/flang/Frontend/PreprocessorOptions.h

27

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

flang/lib/Frontend/CompilerInvocation.cpp

176

Lower case, sadly.

248–251

_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

25

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

flang/include/flang/Frontend/PreprocessorOptions.h

27

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

264–267

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

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

60

-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

39

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

39

I've fixed this, thanks.

60

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

 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 void Flang::AddPreprocessingOptions(const ArgList &Args,
                                     ArgStringList &CmdArgs) const {
-  Args.AddAllArgs(CmdArgs, {options::OPT_D, options::OPT_U});
+  Args.AddAllArgs(CmdArgs, {options::OPT_D, options::OPT_U, options::OPT_I});
 }
 
 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();
 
void Flang::ConstructJob(Compilation &C, const JobAction &JA,
   const InputInfo &Input = Inputs[0];
   types::ID InputType = Input.getType();
 
   // Add preprocessing options like -I, -D, etc. if we are using the
   // preprocessor (i.e. skip when dealing with e.g. binary files).
   if (types::getPreprocessedType(InputType) != types::TY_INVALID)
     AddPreprocessingOptions(Args, CmdArgs);
 
   if (Output.isFilename()) {
     CmdArgs.push_back("-o");
     CmdArgs.push_back(Output.getFilename());
   } else {
     assert(Output.isNothing() && "Invalid output.");
   }
 
   assert(Input.isFilename() && "Invalid input.");
   CmdArgs.push_back(Input.getFilename());

flang/include/flang/Frontend/PreprocessorOptions.h

 
 namespace Fortran::frontend {
 
 /// 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, /*isUndef*/ bool>> macros;
+  // Search directories specified by the user with -I
+  // 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.
+  std::vector<std::string> searchDirectoriesFromDashI;
 
 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

static void parsePreprocessorArgs(
   for (const auto *currentArg : args.filtered(
            clang::driver::options::OPT_D, clang::driver::options::OPT_U)) {
     if (currentArg->getOption().matches(clang::driver::options::OPT_D)) {
       opts.addMacroDef(currentArg->getValue());
     } else {
       opts.addMacroUndef(currentArg->getValue());
     }
   }
+
+  // Add the ordered list of -I's.
+  for (const auto *currentArg : args.filtered(clang::driver::options::OPT_I))
+    opts.searchDirectoriesFromDashI.emplace_back(currentArg->getValue());
 }
 
 bool CompilerInvocation::CreateFromArgs(CompilerInvocation &res,
     llvm::ArrayRef<const char *> commandLineArgs,
     clang::DiagnosticsEngine &diags) {
 
   bool success = true;
 
for (unsigned i = 0, n = ppOpts.macros.size(); i != n; ++i) {
     opts.predefinitions.emplace_back(
         macroName, std::optional<std::string>(macroBody.str()));
   }
 }
 
 void CompilerInvocation::SetDefaultFortranOpts() {
   auto &fortranOptions = fortranOpts();
 
   // These defaults are based on the defaults in f18/f18.cpp.
   std::vector<std::string> searchDirectories{"."s};
   fortranOptions.searchDirectories = searchDirectories;
   fortranOptions.isFixedForm = false;
 }
 
 void CompilerInvocation::setFortranOpts() {
   auto &fortranOptions = fortranOpts();
   const auto &preprocessorOptions = preprocessorOpts();
 
   collectMacroDefinitions(preprocessorOptions, fortranOptions);
-}
+
+  fortranOptions.searchDirectories.insert(
+      fortranOptions.searchDirectories.end(),
+      preprocessorOptions.searchDirectoriesFromDashI.begin(),
+      preprocessorOptions.searchDirectoriesFromDashI.end());
+}

flang/test/Flang-Driver/Inputs/basic-test-header.h

+#define X
 No newline at end of file

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

 ! CHECK-EMPTY:
 ! CHECK-NEXT:OPTIONS:
 ! CHECK-NEXT: -###      Print (but do not run) the commands to run for this compilation
 ! CHECK-NEXT: -D <macro>=<value>     Define <macro> to <value> (or 1 if <value> omitted)
 ! CHECK-NEXT: -E        Only run the preprocessor
 ! CHECK-NEXT: -fcolor-diagnostics    Enable colors in diagnostics
 ! CHECK-NEXT: -fno-color-diagnostics Disable colors in diagnostics
 ! CHECK-NEXT: -help     Display available options
+! CHECK-NEXT: -I <dir>               Add directory to include search path
 ! CHECK-NEXT: -o <file> Write output to <file>
 ! CHECK-NEXT: -test-io  Run the InputOuputTest action. Use for development and testing only.
 ! CHECK-NEXT: -U <macro>             Undefine macro <macro>
 ! CHECK-NEXT: --version Print version information
 
 !-------------------------------------------------------------
 ! EXPECTED OUTPUT FOR FLANG DRIVER (flang-new)
 !-------------------------------------------------------------
 ! ERROR-FLANG: error: unknown argument '-help-hidden'; did you mean '--help-hidden'?
 
 !-------------------------------------------------------------
 ! EXPECTED OUTPUT FOR FLANG FRONTEND DRIVER (flang-new -fc1)
 !-------------------------------------------------------------
 ! Frontend driver -help-hidden is not supported
 ! ERROR-FLANG-FC1: error: unknown argument: '{{.*}}'
 

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

 ! HELP-EMPTY:
 ! HELP-NEXT:OPTIONS:
 ! HELP-NEXT: -###                   Print (but do not run) the commands to run for this compilation
 ! HELP-NEXT: -D <macro>=<value>     Define <macro> to <value> (or 1 if <value> omitted)
 ! HELP-NEXT: -E                     Only run the preprocessor
 ! HELP-NEXT: -fcolor-diagnostics    Enable colors in diagnostics
 ! HELP-NEXT: -fno-color-diagnostics Disable colors in diagnostics
 ! HELP-NEXT: -help                  Display available options
+! HELP-NEXT: -I <dir>               Add directory to include search path
 ! HELP-NEXT: -o <file>              Write output to <file>
 ! HELP-NEXT: -U <macro>             Undefine macro <macro>
 ! HELP-NEXT: --version              Print version information
 
 !-------------------------------------------------------------
 ! EXPECTED OUTPUT FOR FLANG FRONTEND DRIVER (flang-new -fc1)
 !-------------------------------------------------------------
 ! HELP-FC1:USAGE: flang-new
 ! HELP-FC1-EMPTY:
 ! HELP-FC1-NEXT:OPTIONS:
 ! HELP-FC1-NEXT: -D <macro>=<value>     Define <macro> to <value> (or 1 if <value> omitted)
 ! HELP-FC1-NEXT: -E                     Only run the preprocessor
 ! HELP-FC1-NEXT: -help                  Display available options
+! HELP-FC1-NEXT: -I <dir>               Add directory to include search path
 ! HELP-FC1-NEXT: -o <file>              Write output to <file>
 ! HELP-FC1-NEXT: -U <macro>             Undefine macro <macro>
 ! HELP-FC1-NEXT: --version              Print version information
 
 !---------------
 ! EXPECTED ERROR
 !---------------
 ! ERROR: error: unknown argument '-helps'; did you mean '-help'

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

+! Ensure argument -I works as expected.
+
+! REQUIRES: new-flang-driver
+
+!--------------------------
+! FLANG DRIVER (flang-new)
+!--------------------------
+! RUN: %flang-new -E %s  2>&1 | FileCheck %s --check-prefix=UNINCLUDED
+! RUN: %flang-new -E -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: %flang-new -E -I %S/InvalidFolder %s  2>&1 | FileCheck %s --check-prefix=UNINCLUDED
+
+!-----------------------------------------
+! FRONTEND FLANG DRIVER (flang-new -fc1)
+!-----------------------------------------
+! RUN: %flang-new -fc1 -E %s  2>&1 | FileCheck %s --check-prefix=UNINCLUDED
+! RUN: %flang-new -fc1 -E -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: %flang-new -fc1 -E -I %S/InvalidFolder %s  2>&1 | FileCheck %s --check-prefix=UNINCLUDED
+
+!--------------------------------------------
+! EXPECTED OUTPUT FOR MISSING INCLUDED FILE
+!--------------------------------------------
+! UNINCLUDED:program b
+! UNINCLUDED-NOT:program a
+! UNINCLUDED-NEXT:end
+
+!------------------------------
+! EXPECTED OUTPUT FOR INCLUDE
+!------------------------------
+! INCLUDED:program a
+! INCLUDED-NOT:program b
+! INCLUDED-NEXT:end
+
+#include <basic-test-header.h>
+#ifdef X
+program A
+#else
+program B
+#endif
+end
 No newline at end of file