Page MenuHomePhabricator

[Flang][Driver] Add infrastructure for basic frontend actions and file I/O
ClosedPublic

Authored by awarzynski on Sep 20 2020, 11:13 AM.

Details

Summary

This patch introduces the dependencies required to read and manage input files
provided by the command line option. It also adds the structure to create and
write to output files. The output is sent to either stdout or a file
(specified with the -o flag).

Separately, in order to be able to test the code for file I/O, it adds
infrastructure to create frontend actions. As a basic testable example, it adds
the InputOutputTest FrontendAction. The sole purpose of this action is to read
a file from the command line and print it either to stdout or the output file.
This action is run by using the -test-io flag also introduced in this patch, e.g.

flang-new -test-io or flang-new -fc1 -test-io

With this patch, flang-new -test-io input-file.f90 will read input-file.f90 and
print it in the output file.

The InputOutputTest action has been introduced to facilitate testing. It is
hidden from users, not being shown with --help. It is visible with
--help-hidden, and only available for FlangOption and FC1Options in Option.td.
Currently Clang doesn’t have an equivalent action.

-test-io is used to trigger the InputOutputTest action in the Flang frontend
driver. This patch also makes sure that “flang-new” forwards it to
“flang-new -fc1" by creating a preprocessor job. However, in Flang.cpp,
-test-io is passed to “flang-new -fc1” without -E so that the preprocessor is
_not_ run in the frontend driver. This is the desired behaviour: -test-io will
only read the input file and print it to the output stream.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
awarzynski added inline comments.Sep 22 2020, 10:19 AM
flang/include/flang/Frontend/CompilerInstance.h
40–42

[nit]

/// If the output doesn't support seeking (terminal, pipe), we switch
/// the stream to a buffer_ostream. These are the buffer and the original
/// stream.

vvv

/// Output stream that doesn't support seeking (e.g. terminal, pipe). This stream is normally wrapped
/// in buffer_ostream before being passed to users (e.g. via CreateOutputFile).
48

I think that:

This field holds the output stream provided by the user. Normally, users of CompilerInstance will call CreateOutputFile to obtain/create an output stream. If they want to provide their own output stream, this field will facilitate this. It is optional and will normally be just a nullptr.

would be a bit more informative :)

74

Missing /// }

77

[nit] No need to repeat the name of the method/member variable in comments. Same for the rest of the patch.

151–152

Remove Has a? Also, could you expand and clarify the differences between this method and the one below? Ta!

163

This argument is after binary

169

This argument is not documented.

176–182

[nit] I think that the comment is redundant - this is clear from the implementation and it is a fairly standard C++ idiom (e.g. accessors).

It's a good practice to use proper questions to name functions like this one, e.g. IsOutputStreamNull. Then, both possible return values answer this question. Sorry, I couldn't find a link that would document this.

Also, this would be simpler, yet semantically equivalent:

bool IsOutputStreamNull() {return (outputStream_ == nullptr);}
flang/include/flang/Frontend/FrontendAction.h
86

AFAIK, there is no source manager - only file manager.

flang/include/flang/Frontend/FrontendOptions.h
20–22

I recommend that this is replaced with:

InvalidAction = 0,

I think that this requires no comments.

106

Add message, e.g.

assert(IsBuffer() && "Requested buffer_, but it is empty!");
flang/lib/Frontend/FrontendActions.cpp
20

[nit] Get the name of the current file from FrontendInputFile

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

[nit] Both --check-prefix=ERROR and --check-prefix=ERROR-HIDDEN check the help-hidden flang`. Perhaps --check-prefix=ERROR-FLANG-NEW and --check-prefix=ERROR-FLANG-NEW-FC1 would make the difference between the two cases more obvious?

28

This is the expected error for both flang-new (compiler driver) and flang-new -fc1 (the frontend driver).

flang/test/Flang-Driver/emit-obj.f90
2

Why is this line deleted? flang-new -emit-obj should still fail, right?

11–12

I'd be tempted just to delete this line - we have tests elsewhere to show that -o is indeed supported.

flang/test/Frontend/multiple-input-files.f90
2

I think that this test will be much more interesting and useful if empty-file.f90 is not empty :)

tskeith added a project: Restricted Project.Sep 23 2020, 9:19 AM

Solve review comments

Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2020, 5:42 AM
CarolineConcatto marked 23 inline comments as done.Sep 25 2020, 5:54 AM

@awarzynski thank you for the review.
It is a big patch to review at once.
I accepted almost all of your changes.

flang/test/Flang-Driver/emit-obj.f90
2

I remove this because the text does not apply any more
! ERROR-IMPLICIT: error: unknown argument: '-o'

Rebase on top of master

CarolineConcatto retitled this revision from [Flang][Driver] Add InputOutputTest frontend action with new -test-IO flag to [Flang][Driver] Add infrastructure for basic frontend actions and file I/O.Sep 28 2020, 1:03 AM
CarolineConcatto edited the summary of this revision. (Show Details)

@CarolineConcatto thank you again for working on this! The structure is good, but IMHO this patch could be polished a bit more. Overall:

  • could you make sure that this patch does not change the output from clang -help?
  • doxygen comments are consistent
  • unittests are a bit better documentated (what and why is tested?)
  • use llvm/Support/FileSystem.h instead of <filesystem>

More comments inline. Otherwise, I think that this is almost ready :)

clang/include/clang/Driver/Options.td
2818–2819

What about FlangOption?

clang/lib/Driver/Driver.cpp
1581–1584

With this, the following is empty:

$ bin/clang --help | grep help

This is what I'd expect instead (i.e. the behavior shouldn't change):

$ bin/clang --help | grep help
  --help-hidden           Display help for hidden options
  -help                   Display available options

This needs a bit more polishing.

clang/test/Driver/immediate-options.c
5

HELP instead of HEKP

Could add a comment why this particular test is here?

flang/include/flang/Frontend/CompilerInstance.h
58

I can't see a matching /// } for this. DELETEME

84

From what I can see, this is not the case. Could you update?

flang/include/flang/Frontend/FrontendAction.h
24

DELETEME (missing matching /// {)

43

I think that this should be on line 38.

flang/include/flang/Frontend/FrontendOptions.h
17

[nit] Empty line

20

Temporary solution to what?

54–55

Could we defer adding this to later? This patch is already rather larger.

flang/lib/Frontend/CompilerInvocation.cpp
115

Please, could we defer this to later?

flang/lib/Frontend/FrontendOptions.cpp
18–23

Could you reduce it to Language::Fortran only? We can deal with various variants in a separate patch when it becomes relevant.

flang/test/Flang-Driver/driver-help.f90
9–13

Probably should be at the top (for consistency with driver-help-hidden.f90)

24

What about -o?

flang/test/Flang-Driver/emit-obj.f90
2

flang-new -emit-obj is still failing (which is expected) and we should have a test for it. Please revert this.

flang/unittests/Frontend/CompilerInstanceTest.cpp
18

Please use "llvm/Support/FileSystem.h" instead.

32
42

[nit] allSources is only used in one place - this variable is not needed

54

Forgot to test check the error code.

flang/unittests/Frontend/InputOutputTest.cpp
19

Please, use "llvm/Support/FileSystem.h" instead.

23

[nit] This is a test for InputOutputTestAction, perhaps:

TEST(FrontendAction, TestInputOutputStreamOwned)

?

llvm/lib/Option/OptTable.cpp
642 ↗(On Diff #294356)

Revert

CarolineConcatto edited the summary of this revision. (Show Details)

address reviews comment

CarolineConcatto marked 7 inline comments as done.Oct 4 2020, 11:55 AM

@awarzynski patch updated.

LGTM, thanks for working on this!

As this is a fairly large change, could you wait for one more reviewer to approve? Thanks!

awarzynski edited the summary of this revision. (Show Details)Oct 5 2020, 2:08 AM

@reviewers A note regarding the changes in Clang.

This patch introduces a Flang option (-test-io), that should not be available/visible in Clang. AFAIK, there's no precedent of that, hence options::NoClangOption is introduced. This is discussed in more detail here: http://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html.

Thanks for the patch! Overall it's in pretty nice state and conformant to design highlighted.
Could you please clang-format this patch, There are lot of lint warning that causes lot of noise while reviewing.
Couple of NIT comments inline. I've tried marking all but, Could you please finish comments with period.

I think it would good if someone from clang community can also take a brief look.

clang/include/clang/Driver/Options.td
63

NoClangOption ? Is this a Typo, or am I missing the intent behind this ?

clang/lib/Driver/Driver.cpp
1579
flang/include/flang/Frontend/CompilerInstance.h
136

NIT: Blank line ?

201

NIT: Please finish the comment with a period.

flang/lib/Frontend/CompilerInstance.cpp
66

NIT: Missing period at the end ?

67

Can this be simplified ? Maybe a switch case ?

flang/lib/Frontend/FrontendActions.cpp
26

NIT: Instance ? and period at end ?

awarzynski commandeered this revision.Oct 15 2020, 10:02 AM
awarzynski edited reviewers, added: CarolineConcatto; removed: awarzynski.

Thank you for reviewing @SouraVX! I'm just about to submit an updated patch with the requested changes.

@CarolineConcatto has recently moved to a different project and so it will be mostly me updating this. @CarolineConcatto , thanks for all the effort!

clang/include/clang/Driver/Options.td
63

Yup, a typo, thanks!

flang/include/flang/Frontend/CompilerInstance.h
136

That's the convention for Doxygen, isn't it?

flang/lib/Frontend/CompilerInstance.cpp
67

Switch statement would be tricky, but I agree that this is unnecessarily complex.

Address PR comments, clang-format, rebase

I've added more reviewers for the Clang side of this patch. I choose people who most recently changed the functions/files that this patch modifies. Any input much appreciated! For more context regarding Clang changes: http://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html

Simplify the API for creating output files

The originally implemented API was overly complicated and not yet required.

SouraVX accepted this revision.Oct 22 2020, 9:19 AM

Thanks for your patience. LGTM! at least the part I reviewed. Though I would vote for having another approval(From some senior folks in community)

This revision is now accepted and ready to land.Oct 22 2020, 9:19 AM

I'm happy to accept this revision based on @SouraVX 's code review and the fact that Andrzej has sent multiple RFCs covering the clang-side changes, including the Options flags (latest here http://lists.llvm.org/pipermail/cfe-dev/2020-October/067079.html). I think this code is good enough to commit.

Thanks for the code and reviews!

This revision was landed with ongoing or failed builds.Oct 24 2020, 6:58 AM
This revision was automatically updated to reflect the committed changes.

Thank you all for your input! Before merging I took the liberty to rename NoClangOption as FlangOnlyOption. The new name reflects better what the flag is introduced for. Also, based on responses to [1], it is unlikely that it will be used beyond flang-only options. If that changes we can always rename it.

[1] http://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html