[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
awarzynski added a comment. 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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4c5906cffd04: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O (authored by CarolineConcatto, committed by awarzynski). Changed prior to commit: https://reviews.llvm.org/D87989?vs=299033&id=300485#toc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 Files: clang/include/clang/Driver/Options.h clang/include/clang/Driver/Options.td clang/lib/Driver/Driver.cpp clang/lib/Driver/ToolChains/Flang.cpp clang/lib/Driver/Types.cpp clang/test/Driver/immediate-options.c flang/include/flang/Frontend/CompilerInstance.h flang/include/flang/Frontend/CompilerInvocation.h flang/include/flang/Frontend/FrontendAction.h flang/include/flang/Frontend/FrontendActions.h flang/include/flang/Frontend/FrontendOptions.h flang/include/flang/FrontendTool/Utils.h flang/lib/Frontend/CMakeLists.txt flang/lib/Frontend/CompilerInstance.cpp flang/lib/Frontend/CompilerInvocation.cpp flang/lib/Frontend/FrontendAction.cpp flang/lib/Frontend/FrontendActions.cpp flang/lib/Frontend/FrontendOptions.cpp flang/lib/FrontendTool/CMakeLists.txt flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp flang/test/Flang-Driver/driver-help-hidden.f90 flang/test/Flang-Driver/driver-help.f90 flang/test/Flang-Driver/emit-obj.f90 flang/test/Frontend/Inputs/hello-world.f90 flang/test/Frontend/input-output-file.f90 flang/test/Frontend/multiple-input-files.f90 flang/test/lit.cfg.py flang/tools/flang-driver/fc1_main.cpp flang/unittests/Frontend/CMakeLists.txt flang/unittests/Frontend/CompilerInstanceTest.cpp flang/unittests/Frontend/InputOutputTest.cpp Index: flang/unittests/Frontend/InputOutputTest.cpp === --- /dev/null +++ flang/unittests/Frontend/InputOutputTest.cpp @@ -0,0 +1,76 @@ +//===- unittests/Frontend/OutputStreamTest.cpp --- FrontendAction tests --===// +// +// 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 "gtest/gtest.h" +#include "flang/Frontend/CompilerInstance.h" +#include "flang/Frontend/CompilerInvocation.h" +#include "flang/Frontend/FrontendOptions.h" +#include "flang/FrontendTool/Utils.h" +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/raw_ostream.h" + +using namespace Fortran::frontend; + +namespace { + +TEST(FrontendAction, TestInputOutputTestAction) { + std::string inputFile = "io-file-test.f"; + std::error_code ec; + + // 1. Create the input file for the file manager + // AllSources (which is used to manage files inside every compiler instance), + // works with paths. This means that it requires a physical file. Create one. + std::unique_ptr os{ + new llvm::raw_fd_ostream(inputFile, ec, llvm::sys::fs::OF_None)}; + if (ec) +FAIL() << "Failed to create the input file"; + + // Populate the input file with the pre-defined input and flush it. + *(os) << "End Program arithmetic"; + os.reset(); + + // Get the path of the input file + llvm::SmallString<64> cwd; + if (std::error_code ec = llvm::sys::fs::current_path(cwd)) +FAIL() << "Failed to obtain the current working directory"; + std::string testFilePath(cwd.c_str()); + testFilePath += "/" + inputFile; + + // 2. Prepare the compiler (CompilerInvocation + CompilerInstance) + CompilerInstance compInst; + compInst.CreateDiagnostics(); + auto invocation = std::make_shared(); + invocation->GetFrontendOpts().programAction_ = InputOutputTest; + compInst.SetInvocation(std::move(invocation)); + compInst.GetFrontendOpts().inputs_.push_back( + FrontendInputFile(/*File=*/testFilePath, Language::Fortran)); + + // 3. Set-up the output stream. Using output buffer wrapped as an output + // stream, as opposed to an actual file (or a file descriptor). + llvm::SmallVector outputFileBuffer; + std::unique_ptr outputFileStream( + new llvm::raw_svector_ostream(outputFileBuffer)); + compInst.SetOutputStream(std::move(outputFileStream)); + + // 4. Run the earlier defined FrontendAction + bool success = ExecuteCompilerInvocation(&compInst); + + EXPECT_TRUE(success); + EXPECT_TRUE(!outputFileBuffer.empty()); + EXPECT_TRUE(llvm::StringRef(outputFileBuffer.data()) + .startswith("End Program arithmetic")); + + // 5. Clear the input and the output files. Since we used an output buffer, + // there are no physical output files to delete. + ec = llvm::sys::fs::remove(inputFile); + if (ec) +FAIL() << "Failed to delete the test file"; + + compInst.ClearOutputFiles(/*EraseFiles=*/false); +} +} // namespace I
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
richard.barton.arm accepted this revision. richard.barton.arm added a comment. 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! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
SouraVX accepted this revision. SouraVX added a comment. This revision is now accepted and ready to land. 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) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
awarzynski added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
awarzynski updated this revision to Diff 299033. awarzynski added a comment. Simplify the API for creating output files The originally implemented API was overly complicated and not yet required. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 Files: clang/include/clang/Driver/Options.h clang/include/clang/Driver/Options.td clang/lib/Driver/Driver.cpp clang/lib/Driver/ToolChains/Flang.cpp clang/lib/Driver/Types.cpp clang/test/Driver/immediate-options.c flang/include/flang/Frontend/CompilerInstance.h flang/include/flang/Frontend/CompilerInvocation.h flang/include/flang/Frontend/FrontendAction.h flang/include/flang/Frontend/FrontendActions.h flang/include/flang/Frontend/FrontendOptions.h flang/include/flang/FrontendTool/Utils.h flang/lib/Frontend/CMakeLists.txt flang/lib/Frontend/CompilerInstance.cpp flang/lib/Frontend/CompilerInvocation.cpp flang/lib/Frontend/FrontendAction.cpp flang/lib/Frontend/FrontendActions.cpp flang/lib/Frontend/FrontendOptions.cpp flang/lib/FrontendTool/CMakeLists.txt flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp flang/test/Flang-Driver/driver-help-hidden.f90 flang/test/Flang-Driver/driver-help.f90 flang/test/Flang-Driver/emit-obj.f90 flang/test/Frontend/Inputs/hello-world.f90 flang/test/Frontend/input-output-file.f90 flang/test/Frontend/multiple-input-files.f90 flang/test/lit.cfg.py flang/tools/flang-driver/fc1_main.cpp flang/unittests/Frontend/CMakeLists.txt flang/unittests/Frontend/CompilerInstanceTest.cpp flang/unittests/Frontend/InputOutputTest.cpp Index: flang/unittests/Frontend/InputOutputTest.cpp === --- /dev/null +++ flang/unittests/Frontend/InputOutputTest.cpp @@ -0,0 +1,72 @@ +//===- unittests/Frontend/OutputStreamTest.cpp --- FrontendAction tests --===// +// +// 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 "gtest/gtest.h" +#include "flang/Frontend/CompilerInstance.h" +#include "flang/Frontend/CompilerInvocation.h" +#include "flang/Frontend/FrontendOptions.h" +#include "flang/FrontendTool/Utils.h" +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/raw_ostream.h" + +using namespace Fortran::frontend; + +namespace { + +TEST(FrontendAction, TestInputOutputTestAction) { + std::string inputFile = "io-file-test.f"; + std::error_code ec; + + // 1. Create the input file for the file manager + // AllSources (which is used to manage files inside every compiler instance), + // works with paths. This means that it requires a physical file. Create one. + std::unique_ptr os{ + new llvm::raw_fd_ostream(inputFile, ec, llvm::sys::fs::OF_None)}; + if (ec) +FAIL() << "Failed to create the input file"; + + // Populate the input file with the pre-defined input and flush it. + *(os) << "End Program arithmetic"; + os.reset(); + + // Get the path of the input file + llvm::SmallString<64> cwd; + if (std::error_code ec = llvm::sys::fs::current_path(cwd)) +FAIL() << "Failed to obtain the current working directory"; + std::string testFilePath(cwd.c_str()); + testFilePath += "/" + inputFile; + + // 2. Prepare the compiler (CompilerInvocation + CompilerInstance) + CompilerInstance compInst; + compInst.CreateDiagnostics(); + auto invocation = std::make_shared(); + invocation->GetFrontendOpts().programAction_ = InputOutputTest; + compInst.SetInvocation(std::move(invocation)); + compInst.GetFrontendOpts().inputs_.push_back( + FrontendInputFile(/*File=*/testFilePath, Language::Fortran)); + + // 3. Set-up the output stream. Using output buffer wrapped as an output + // stream, as opposed to an actual file (or a file descriptor). + llvm::SmallVector outputFileBuffer; + std::unique_ptr outputFileStream( + new llvm::raw_svector_ostream(outputFileBuffer)); + compInst.SetOutputStream(std::move(outputFileStream)); + + // 4. Run the earlier defined FrontendAction + bool success = ExecuteCompilerInvocation(&compInst); + + EXPECT_TRUE(success); + EXPECT_TRUE(!outputFileBuffer.empty()); + EXPECT_TRUE(llvm::StringRef(outputFileBuffer.data()) + .startswith("End Program arithmetic")); + + // 5. Clear the output files. Since we used an output buffer, there are no + // physical files to delete. + compInst.ClearOutputFiles(/*EraseFiles=*/false); +} +} // namespace Index: flang/unittests/Frontend/CompilerInstanceTest.cpp === --- flang/unittests/Frontend/CompilerInstanceTest.cpp +++ flang/unittests/Frontend/CompilerInstanceTest.cpp @@ -9,6 +9,7 @@ #include "flang/Frontend/CompilerInstance.h" #include "fl
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
awarzynski added a comment. 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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
awarzynski commandeered this revision. awarzynski edited reviewers, added: CarolineConcatto; removed: awarzynski. awarzynski added a comment. 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! Comment at: clang/include/clang/Driver/Options.td:63 +// ClangOption - This option should not be accepted by Clang. +def NoClangOption : OptionFlag; SouraVX wrote: > `NoClangOption` ? Is this a Typo, or am I missing the intent behind this ? Yup, a typo, thanks! Comment at: flang/include/flang/Frontend/CompilerInstance.h:136 + /// Add an output file onto the list of tracked output files. + /// + /// \param outFile - The output file info. SouraVX wrote: > NIT: Blank line ? That's the convention for Doxygen, isn't it? Comment at: flang/lib/Frontend/CompilerInstance.cpp:67 + // Create the name of the output file + if (!outputPath.empty()) { +outFile = std::string(outputPath); SouraVX wrote: > Can this be simplified ? Maybe a switch case ? Switch statement would be tricky, but I agree that this is unnecessarily complex. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
awarzynski updated this revision to Diff 298407. awarzynski added a comment. Address PR comments, clang-format, rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 Files: clang/include/clang/Driver/Options.h clang/include/clang/Driver/Options.td clang/lib/Driver/Driver.cpp clang/lib/Driver/ToolChains/Flang.cpp clang/lib/Driver/Types.cpp clang/test/Driver/immediate-options.c flang/include/flang/Frontend/CompilerInstance.h flang/include/flang/Frontend/CompilerInvocation.h flang/include/flang/Frontend/FrontendAction.h flang/include/flang/Frontend/FrontendActions.h flang/include/flang/Frontend/FrontendOptions.h flang/include/flang/FrontendTool/Utils.h flang/lib/Frontend/CMakeLists.txt flang/lib/Frontend/CompilerInstance.cpp flang/lib/Frontend/CompilerInvocation.cpp flang/lib/Frontend/FrontendAction.cpp flang/lib/Frontend/FrontendActions.cpp flang/lib/Frontend/FrontendOptions.cpp flang/lib/FrontendTool/CMakeLists.txt flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp flang/test/Flang-Driver/driver-help-hidden.f90 flang/test/Flang-Driver/driver-help.f90 flang/test/Flang-Driver/emit-obj.f90 flang/test/Frontend/Inputs/hello-world.f90 flang/test/Frontend/input-output-file.f90 flang/test/Frontend/multiple-input-files.f90 flang/test/lit.cfg.py flang/tools/flang-driver/fc1_main.cpp flang/unittests/Frontend/CMakeLists.txt flang/unittests/Frontend/CompilerInstanceTest.cpp flang/unittests/Frontend/InputOutputTest.cpp Index: flang/unittests/Frontend/InputOutputTest.cpp === --- /dev/null +++ flang/unittests/Frontend/InputOutputTest.cpp @@ -0,0 +1,72 @@ +//===- unittests/Frontend/OutputStreamTest.cpp --- FrontendAction tests --===// +// +// 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 "gtest/gtest.h" +#include "flang/Frontend/CompilerInstance.h" +#include "flang/Frontend/CompilerInvocation.h" +#include "flang/Frontend/FrontendOptions.h" +#include "flang/FrontendTool/Utils.h" +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/raw_ostream.h" + +using namespace Fortran::frontend; + +namespace { + +TEST(FrontendAction, TestInputOutputTestAction) { + std::string inputFile = "io-file-test.f"; + std::error_code ec; + + // 1. Create the input file for the file manager + // AllSources (which is used to manage files inside every compiler instance), + // works with paths. This means that it requires a physical file. Create one. + std::unique_ptr os{ + new llvm::raw_fd_ostream(inputFile, ec, llvm::sys::fs::OF_None)}; + if (ec) +FAIL() << "Failed to create the input file"; + + // Populate the input file with the pre-defined input and flush it. + *(os) << "End Program arithmetic"; + os.reset(); + + // Get the path of the input file + llvm::SmallString<64> cwd; + if (std::error_code ec = llvm::sys::fs::current_path(cwd)) +FAIL() << "Failed to obtain the current working directory"; + std::string testFilePath(cwd.c_str()); + testFilePath += "/" + inputFile; + + // 2. Prepare the compiler (CompilerInvocation + CompilerInstance) + CompilerInstance compInst; + compInst.CreateDiagnostics(); + auto invocation = std::make_shared(); + invocation->GetFrontendOpts().programAction_ = InputOutputTest; + compInst.SetInvocation(std::move(invocation)); + compInst.GetFrontendOpts().inputs_.push_back( + FrontendInputFile(/*File=*/testFilePath, Language::Fortran)); + + // 3. Set-up the output stream. Using output buffer wrapped as an output + // stream, as opposed to an actual file (or a file descriptor). + llvm::SmallVector outputFileBuffer; + std::unique_ptr outputFileStream( + new llvm::raw_svector_ostream(outputFileBuffer)); + compInst.SetOutputStream(std::move(outputFileStream)); + + // 4. Run the earlier defined FrontendAction + bool success = ExecuteCompilerInvocation(&compInst); + + EXPECT_TRUE(success); + EXPECT_TRUE(!outputFileBuffer.empty()); + EXPECT_TRUE(llvm::StringRef(outputFileBuffer.data()) + .startswith("End Program arithmetic")); + + // 5. Clear the output files. Since we used an output buffer, there are no + // physical files to delete. + compInst.ClearOutputFiles(/*EraseFiles=*/false); +} +} // namespace Index: flang/unittests/Frontend/CompilerInstanceTest.cpp === --- flang/unittests/Frontend/CompilerInstanceTest.cpp +++ flang/unittests/Frontend/CompilerInstanceTest.cpp @@ -9,6 +9,7 @@ #include "flang/Frontend/CompilerInstance.h" #include "flang/Frontend/TextDiagnosticPrinter.h" #include "clang/Basic/DiagnosticOptions
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
SouraVX added a comment. 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. Comment at: clang/include/clang/Driver/Options.td:63 +// ClangOption - This option should not be accepted by Clang. +def NoClangOption : OptionFlag; `NoClangOption` ? Is this a Typo, or am I missing the intent behind this ? Comment at: clang/lib/Driver/Driver.cpp:1579 - if (IsFlangMode()) + if (IsFlangMode()) { IncludedFlagsBitmask |= options::FlangOption; NIT: May choose to avoid trivial braces. https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements Comment at: flang/include/flang/Frontend/CompilerInstance.h:136 + /// Add an output file onto the list of tracked output files. + /// + /// \param outFile - The output file info. NIT: Blank line ? Comment at: flang/include/flang/Frontend/CompilerInstance.h:230 + + // Allow the frontend compiler to write in the output stream + void WriteOutputStream(const std::string &message) { NIT: Please finish the comment with a period. Comment at: flang/lib/Frontend/CompilerInstance.cpp:66 + + // Create the name of the output file + if (!outputPath.empty()) { NIT: Missing period at the end ? Comment at: flang/lib/Frontend/CompilerInstance.cpp:67 + // Create the name of the output file + if (!outputPath.empty()) { +outFile = std::string(outputPath); Can this be simplified ? Maybe a switch case ? Comment at: flang/lib/Frontend/FrontendActions.cpp:25 + + // Set/store input file info into compiler instace + CompilerInstance &ci = GetCompilerInstance(); NIT: Instance ? and period at end ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
awarzynski added a comment. @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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
awarzynski added a comment. LGTM, thanks for working on this! As this is a fairly large change, could you wait for one more reviewer to approve? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
CarolineConcatto marked 7 inline comments as done. CarolineConcatto added a comment. @awarzynski patch updated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
CarolineConcatto updated this revision to Diff 296066. CarolineConcatto edited the summary of this revision. CarolineConcatto added a comment. address reviews comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87989/new/ https://reviews.llvm.org/D87989 Files: clang/include/clang/Driver/Options.h clang/include/clang/Driver/Options.td clang/lib/Driver/Driver.cpp clang/lib/Driver/ToolChains/Flang.cpp clang/lib/Driver/Types.cpp clang/test/Driver/immediate-options.c flang/include/flang/Frontend/CompilerInstance.h flang/include/flang/Frontend/CompilerInvocation.h flang/include/flang/Frontend/FrontendAction.h flang/include/flang/Frontend/FrontendActions.h flang/include/flang/Frontend/FrontendOptions.h flang/include/flang/FrontendTool/Utils.h flang/lib/Frontend/CMakeLists.txt flang/lib/Frontend/CompilerInstance.cpp flang/lib/Frontend/CompilerInvocation.cpp flang/lib/Frontend/FrontendAction.cpp flang/lib/Frontend/FrontendActions.cpp flang/lib/Frontend/FrontendOptions.cpp flang/lib/FrontendTool/CMakeLists.txt flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp flang/test/Flang-Driver/driver-help-hidden.f90 flang/test/Flang-Driver/driver-help.f90 flang/test/Flang-Driver/emit-obj.f90 flang/test/Frontend/Inputs/hello-world.f90 flang/test/Frontend/input-output-file.f90 flang/test/Frontend/multiple-input-files.f90 flang/test/lit.cfg.py flang/tools/flang-driver/fc1_main.cpp flang/unittests/Frontend/CMakeLists.txt flang/unittests/Frontend/CompilerInstanceTest.cpp flang/unittests/Frontend/InputOutputTest.cpp llvm/include/llvm/Option/OptTable.h Index: llvm/include/llvm/Option/OptTable.h === --- llvm/include/llvm/Option/OptTable.h +++ llvm/include/llvm/Option/OptTable.h @@ -243,7 +243,8 @@ /// \param Usage - USAGE: Usage /// \param Title - OVERVIEW: Title /// \param FlagsToInclude - If non-zero, only include options with any - /// of these flags set. + /// of these flags set. Takes precedence over + /// FlagsToExclude. /// \param FlagsToExclude - Exclude options with any of these flags set. /// \param ShowAllAliases - If true, display all options including aliases /// that don't have help texts. By default, we display Index: flang/unittests/Frontend/InputOutputTest.cpp === --- /dev/null +++ flang/unittests/Frontend/InputOutputTest.cpp @@ -0,0 +1,72 @@ +//===- unittests/Frontend/OutputStreamTest.cpp --- FrontendAction tests --===// +// +// 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/Frontend/CompilerInstance.h" +#include "flang/Frontend/CompilerInvocation.h" +#include "flang/Frontend/FrontendOptions.h" +#include "flang/FrontendTool/Utils.h" +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/raw_ostream.h" +#include "gtest/gtest.h" + +using namespace Fortran::frontend; + +namespace { + +TEST(FrontendAction, TestInputOutputTestAction) { + std::string inputFile = "io-file-test.f"; + std::error_code ec; + + // 1. Create the input file for the file manager + // AllSources (which is used to manage files inside every compiler instance), + // works with paths. This means that it requires a physical file. Create one. + std::unique_ptr os{ + new llvm::raw_fd_ostream(inputFile, ec, llvm::sys::fs::OF_None)}; + if (ec) +FAIL() << "Failed to create the input file"; + + // Populate the input file with the pre-defined input and flush it. + *(os) << "End Program arithmetic"; + os.reset(); + + // Get the path of the input file + llvm::SmallString<64> cwd; + if (std::error_code ec = llvm::sys::fs::current_path(cwd)) +FAIL() << "Failed to obtain the current working directory"; + std::string testFilePath(cwd.c_str()); + testFilePath += "/" + inputFile; + + // 2. Prepare the compiler (CompilerInvocation + CompilerInstance) + CompilerInstance compInst; + compInst.CreateDiagnostics(); + auto invocation = std::make_shared(); + invocation->GetFrontendOpts().programAction_ = InputOutputTest; + compInst.SetInvocation(std::move(invocation)); + compInst.GetFrontendOpts().inputs_.push_back( + FrontendInputFile(/*File=*/testFilePath, Language::Fortran)); + + // 3. Set-up the output stream. Using output buffer wrapped as an output + // stream, as opposed to an actual file (or a file descriptor). + llvm::SmallVector outputFileBuffer; + std::unique_ptr outputFileStream( + new llvm::raw_svector_ostream(outputFileBuffer)); + compInst.SetOutputStream(std::move(outputFileStream)); + + // 4
[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O
awarzynski added a comment. @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 `` More comments inline. Otherwise, I think that this is almost ready :) Comment at: clang/include/clang/Driver/Options.td:2795 def object : Flag<["-"], "object">; -def o : JoinedOrSeparate<["-"], "o">, Flags<[DriverOption, RenderAsInput, CC1Option, CC1AsOption]>, +def o : JoinedOrSeparate<["-"], "o">, Flags<[DriverOption, RenderAsInput, CC1Option, CC1AsOption, FC1Option]>, HelpText<"Write output to ">, MetaVarName<"">; What about FlangOption? Comment at: clang/lib/Driver/Driver.cpp:1576-1579 + } else { +ExcludedFlagsBitmask |= options::FlangOption; +ExcludedFlagsBitmask |= options::FC1Option; + } 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. Comment at: clang/test/Driver/immediate-options.c:5 // HELP-NOT: driver-mode +// HEKP-NOT: test-io HELP instead of HEKP Could add a comment why this particular test is here? Comment at: flang/include/flang/Frontend/CompilerInstance.h:58 + + /// } + /// @name Compiler Invocation I can't see a matching `/// }` for this. DELETEME Comment at: flang/include/flang/Frontend/CompilerInstance.h:85 + /// CompilerInvocation object. + /// Note that this routine may write output to 'stderr'. + /// \param act - The action to execute. From what I can see, this is not the case. Could you update? Comment at: flang/include/flang/Frontend/FrontendAction.h:23 +protected: + /// @} + /// @name Implementation Action Interface DELETEME (missing matching `/// {`) Comment at: flang/include/flang/Frontend/FrontendAction.h:42 + + /// @} + /// @name Compiler Instance Access I think that this should be on line 38. Comment at: flang/include/flang/Frontend/FrontendOptions.h:16 #include namespace Fortran::frontend { [nit] Empty line Comment at: flang/include/flang/Frontend/FrontendOptions.h:19 +enum ActionKind { + // This is temporary solution + // Avoids Action = InputOutputTest as option 0 Temporary solution to what? Comment at: flang/include/flang/Frontend/FrontendOptions.h:56-59 + Fortran77, + Fortran90, + Fortran95, + FortranF18 Could we defer adding this to later? This patch is already rather larger. Comment at: flang/lib/Frontend/CompilerInvocation.cpp:72 dashX = llvm::StringSwitch(XValue) -.Case("f90", Language::Fortran) +.Case("f90", Language::Fortran90) .Default(Language::Unknown); Please, could we defer this to later? Comment at: flang/lib/Frontend/FrontendOptions.cpp:18-23 + .Cases("ff", "fpp", "FPP", "cuf", "CUF", "fir", "FOR", "for", + Language::Fortran) + .Cases("f", "F", "f77", Language::Fortran77) + .Cases("f90", "F90", ".ff90", Language::Fortran90) + .Cases("f95", "F95", "ff95", Language::Fortran95) + .Cases("f18", "F18", Language::FortranF18) Could you reduce it to `Language::Fortran` only? We can deal with various variants in a separate patch when it becomes relevant. Comment at: flang/test/Flang-Driver/driver-help.f90:14 + ! REQUIRES: new-flang-driver Probably should be at the top (for consistency with driver-help-hidden.f90) Comment at: flang/test/Flang-Driver/driver-help.f90:22 +! CHECK-FLANG-NEXT:OPTIONS: +! CHECK-FLANG-NEXT: -help Display available options +! CHECK-FLANG-NEXT: --version Print version information What about `-o`? Comment at: flang/test/Flang-Driver/emit-obj.f90:2 ! RUN: not %flang-new %s 2>&1 | FileCheck %s --check-prefix=ERROR-IMPLICIT -! RUN: not %flang-new -emit-obj %s 2>&1 | FileCheck %s --check-prefix=ERROR-EXPLICIT ! RUN: not %flang-new -fc1 -emit-obj %s 2>&1 | FileCheck %s --check-prefix=ERROR-FC1 CarolineConcatto wrote: > awarzynski wrote: > > Why is this line deleted? `flang-new -emit-obj` should still fail, right? > I remove this because the text does not apply any more > ! ERROR-IMPLICIT: error: unknown