Page MenuHomePhabricator

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

Authored by FarisRehman on Dec 17 2020, 5:09 AM.

Details

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

Diff Detail

Event Timeline

FarisRehman created this revision.Dec 17 2020, 5:09 AM
FarisRehman requested review of this revision.Dec 17 2020, 5:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 5:09 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
FarisRehman edited the summary of this revision. (Show Details)Dec 17 2020, 5:11 AM
FarisRehman added a project: Restricted Project.

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

clang/lib/Driver/ToolChains/Flang.cpp
82

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
181

Lower case, sadly.

253–261

_Moving_ this code seems like _unrelated_ change.

FarisRehman marked 4 inline comments as done.Dec 22 2020, 6:13 AM

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

Clean up test

Summary of changes:

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

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
274–277

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
28

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?

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.

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 marked 5 inline comments as done.Jan 6 2021, 8:13 AM
FarisRehman updated this revision to Diff 314905.EditedJan 6 2021, 8:17 AM

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

Rename test files

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

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 added inline comments.Jan 13 2021, 9:31 AM
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 added inline comments.Jan 14 2021, 6:54 AM
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 marked 2 inline comments as done.Jan 15 2021, 5:03 AM
FarisRehman added inline comments.
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 marked 2 inline comments as done.

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
FarisRehman edited the summary of this revision. (Show Details)Jan 15 2021, 5:04 AM
awarzynski accepted this revision.Jan 18 2021, 2:25 AM

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!

This revision is now accepted and ready to land.Jan 18 2021, 2:25 AM
FarisRehman edited the summary of this revision. (Show Details)Jan 18 2021, 2:44 AM
This revision was landed with ongoing or failed builds.Jan 19 2021, 3:22 AM
This revision was automatically updated to reflect the committed changes.