[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

We forgot to update the signature and definition of `OptTable::findByPrefix`: 
https://github.com/llvm/llvm-project/blob/257b29715bb27b7d9f6c3c40c481b6a4af0b37e5/llvm/include/llvm/Option/OptTable.h#L154-L155.
 That's required after updating the definition of `OptTable::Flags`. Not sure 
none of the compilers complained.

Fixed here: 
https://github.com/llvm/llvm-project/commit/4eed800b18abaeba3082bf950fbe5c3020c4b592


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Caroline via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG257b29715bb2: [flang][driver] Add the new flang compiler and 
frontend drivers (authored by CarolineConcatto).

Changed prior to commit:
  https://reviews.llvm.org/D86089?vs=291068=291169#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

Files:
  clang/include/clang/Driver/Driver.h
  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/Frontend/CreateInvocationFromCommandLine.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/unittests/Driver/SanitizerArgsTest.cpp
  clang/unittests/Driver/ToolChainTest.cpp
  flang/CMakeLists.txt
  flang/README.md
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/CMakeLists.txt
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Flang-Driver/driver-error-cc1.c
  flang/test/Flang-Driver/driver-error-cc1.cpp
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/driver-version.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/Flang-Driver/missing-input.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/CMakeLists.txt
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.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
@@ -50,7 +50,7 @@
 unsigned ID;
 unsigned char Kind;
 unsigned char Param;
-unsigned short Flags;
+unsigned int Flags;
 unsigned short GroupID;
 unsigned short AliasID;
 const char *AliasArgs;
Index: flang/unittests/Frontend/CompilerInstanceTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -0,0 +1,52 @@
+//===- unittests/Frontend/CompilerInstanceTest.cpp - CI 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 "gtest/gtest.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Options.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include 
+using namespace llvm;
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(CompilerInstance, AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
+  // 1. Set-up a basic DiagnosticConsumer
+  std::string diagnosticOutput;
+  llvm::raw_string_ostream diagnosticsOS(diagnosticOutput);
+  auto diagPrinter = std::make_unique(
+  diagnosticsOS, new clang::DiagnosticOptions());
+
+  // 2. Create a CompilerInstance (to manage a DiagnosticEngine)
+  CompilerInstance compInst;
+
+  // 3. Set-up DiagnosticOptions
+  auto diagOpts = new clang::DiagnosticOptions();
+  // Tell the diagnostics engine to emit the diagnostic log to STDERR. This
+  // ensures that a chained diagnostic consumer is created so that the test can
+  // exercise the unowned diagnostic consumer in a chained consumer.
+  diagOpts->DiagnosticLogFile = "-";
+
+  // 4. Create a DiagnosticEngine with an unowned consumer
+  IntrusiveRefCntPtr diags =
+  compInst.CreateDiagnostics(diagOpts, diagPrinter.get(),
+  /*ShouldOwnClient=*/false);
+
+  // 5. Report a diagnostic
+  diags->Report(clang::diag::err_expected) << "no crash";
+
+  // 6. Verify that the reported diagnostic wasn't lost and did end up in the
+  // output stream
+  ASSERT_EQ(diagnosticsOS.str(), "error: expected no crash\n");
+}
+} // namespace
Index: flang/unittests/Frontend/CMakeLists.txt
===
--- /dev/null
+++ flang/unittests/Frontend/CMakeLists.txt
@@ 

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm accepted this revision.
richard.barton.arm added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 291068.
awarzynski marked 2 inline comments as done.
awarzynski added a comment.

Address final comments

I also took the liberty and made the following changes in CMake scripts:

- replaced add_library wiht add_flang_library
- replaced add_executable with add_flang_tool

These are all NFCs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

Files:
  clang/include/clang/Driver/Driver.h
  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/Frontend/CreateInvocationFromCommandLine.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/unittests/Driver/SanitizerArgsTest.cpp
  clang/unittests/Driver/ToolChainTest.cpp
  flang/CMakeLists.txt
  flang/README.md
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/CMakeLists.txt
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Flang-Driver/driver-error-cc1.c
  flang/test/Flang-Driver/driver-error-cc1.cpp
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/driver-version.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/Flang-Driver/missing-input.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/CMakeLists.txt
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.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
@@ -50,7 +50,7 @@
 unsigned ID;
 unsigned char Kind;
 unsigned char Param;
-unsigned short Flags;
+unsigned int Flags;
 unsigned short GroupID;
 unsigned short AliasID;
 const char *AliasArgs;
Index: flang/unittests/Frontend/CompilerInstanceTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -0,0 +1,52 @@
+//===- unittests/Frontend/CompilerInstanceTest.cpp - CI 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 "gtest/gtest.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Options.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include 
+using namespace llvm;
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(CompilerInstance, AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
+  // 1. Set-up a basic DiagnosticConsumer
+  std::string diagnosticOutput;
+  llvm::raw_string_ostream diagnosticsOS(diagnosticOutput);
+  auto diagPrinter = std::make_unique(
+  diagnosticsOS, new clang::DiagnosticOptions());
+
+  // 2. Create a CompilerInstance (to manage a DiagnosticEngine)
+  CompilerInstance compInst;
+
+  // 3. Set-up DiagnosticOptions
+  auto diagOpts = new clang::DiagnosticOptions();
+  // Tell the diagnostics engine to emit the diagnostic log to STDERR. This
+  // ensures that a chained diagnostic consumer is created so that the test can
+  // exercise the unowned diagnostic consumer in a chained consumer.
+  diagOpts->DiagnosticLogFile = "-";
+
+  // 4. Create a DiagnosticEngine with an unowned consumer
+  IntrusiveRefCntPtr diags =
+  compInst.CreateDiagnostics(diagOpts, diagPrinter.get(),
+  /*ShouldOwnClient=*/false);
+
+  // 5. Report a diagnostic
+  diags->Report(clang::diag::err_expected) << "no crash";
+
+  // 6. Verify that the reported diagnostic wasn't lost and did end up in the
+  // output stream
+  ASSERT_EQ(diagnosticsOS.str(), "error: expected no crash\n");
+}
+} // namespace
Index: flang/unittests/Frontend/CMakeLists.txt
===
--- /dev/null
+++ flang/unittests/Frontend/CMakeLists.txt
@@ -0,0 

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm accepted this revision.
richard.barton.arm added a comment.
This revision is now accepted and ready to land.

This LGTM. I think all the previous comments from other reviewers and me have 
been dealt with so I am happy to accept this revision based on the reviews so 
far.

I have a few small inline comments to resolve in PrintHelp now we have reverted 
the functional changes there. Happy to approve on the assumption that they are 
dealt with and I don't need to see another patch, you can accept it yourself.

I think the non-flang changes to clang and llvm are in-line with what we said 
in our RFC. I think the summary of these changes are:

- Don't hard-code the name of the driver in the object constructor, pass it in 
as an argument + fix up all the clang callsites.
- Tweak the --help and --version logic to be conditional on driver mode
- Add some new FlangOption flags to Options.td
- Change the flang driver binary name to flang-new (in the already approved 
flang mods)




Comment at: flang/test/Flang-Driver/emit-obj.f90:2
+! RUN: %flang-new  %s 2>&1 | FileCheck %s --check-prefix=ERROR
+! RUN: %flang-new  -fc1 -emit-obj %s 2>&1 | FileCheck %s --check-prefix=ERROR
+

awarzynski wrote:
> richard.barton.arm wrote:
> > Seems like it would be helpful to have also implemented `-###` in this 
> > patch so that you don't need to write tests like this. You could simply run 
> > flang-new -### then check the -fc1 line that would have been emitted for 
> > the presence of -emit-obj.
> > 
> > Same comment above regarding exit code.
> > Seems like it would be helpful to have also implemented -### in this patch
> 
> As`flang-new` is based on libclangDriver, we get `-###` for free (i.e. it's 
> already supported).
> 
> However, there's a catch: 
> https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Driver/Options.td#L369-L370.
>  Currently `flang-new --help` will not display `-###`  because the the 
> corresponding option definition hasn't been updated (i.e. it is not flagged 
> as a Flang option):
> ```
> def _HASH_HASH_HASH : Flag<["-"], "###">, Flags<[DriverOption, CoreOption]>,
> HelpText<"Print (but do not run) the commands to run for this 
> compilation">;
> ``` 
> We have three options:
> 1. Make `flang-new --help` display options that are flagged as `DriverOption` 
> or `CoreOption`. Note this will include many other options (currently 
> unsupported) and extra filtering would be required.
> 2. Add `FlangOption` to the definition of `_HASH_HASH_HASH`, but IMO that's a 
> bit controversial. Is that what we want to do long-term? Or shall we create a 
> separate category for generic driver options that apply to both Clang and 
> Flang?
> 3. Delay the decision until there is more code to base our design on.
> 
> I'm in favor of Option 3.
Fair enough. Happy to cross this bridge when we come to it later on. I 
certainly think that flang-new should support -### one day.



Comment at: llvm/include/llvm/Option/OptTable.h:228
   /// \param FlagsToInclude - If non-zero, only include options with any
-  /// of these flags set.
   /// \param FlagsToExclude - Exclude options with any of these flags set.

I don't think this change is correct now that you have reverted the code change 
to the function.

If I have the same bit set in FlagsToInclude and FlagsToExclude, the 
FlagsToExclude check fires second and would mean the option is not printed. So 
FlagsToExclude takes precedence. 

Suggest to drop the edit or to correct it.



Comment at: llvm/lib/Option/OptTable.cpp:613
 unsigned Flags = getInfo(Id).Flags;
+
 if (FlagsToInclude && !(Flags & FlagsToInclude))

With the diff to this logic gone, we should also remove the new newlines so as 
not to make textual changes unrelated to this patch.



Comment at: llvm/lib/Option/OptTable.cpp:621
+// If `Flags` is empty (i.e. it's an option without any flags) then this is
+// a Clang-only option. If:
+// * we _are not_ in Flang Mode (FlagsToExclude contains FlangMode), then

awarzynski wrote:
> richard.barton.arm wrote:
> > awarzynski wrote:
> > > richard.barton.arm wrote:
> > > > This is not the sort of change I expect to see in an llvm support 
> > > > library given that it seems very clang and flang specific. 
> > > > 
> > > > I think this needs to be re-written to be more generic, perhaps 
> > > > tweaking the interface to be able to express the logic you want to use 
> > > > for flang and clang.
> > > > 
> > > > Why is it not sufficient to call this function unchanged from both 
> > > > flang and clang but with a different set of 
> > > > FlagsToInclude/FlagsToExclude passed in using this logic to set that on 
> > > > the clang/flang side?
> > > I agree and have updated this. Thanks for pointing it out!
> > > 
> > > This part is particularly tricky for Flang. Flang has 

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 290435.
awarzynski added a comment.

Add missing update in lit.cfg.py


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

Files:
  clang/include/clang/Driver/Driver.h
  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/Frontend/CreateInvocationFromCommandLine.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/unittests/Driver/SanitizerArgsTest.cpp
  clang/unittests/Driver/ToolChainTest.cpp
  flang/CMakeLists.txt
  flang/README.md
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/CMakeLists.txt
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Flang-Driver/driver-error-cc1.c
  flang/test/Flang-Driver/driver-error-cc1.cpp
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/driver-version.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/Flang-Driver/missing-input.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/CMakeLists.txt
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  llvm/include/llvm/Option/OptTable.h
  llvm/lib/Option/OptTable.cpp

Index: llvm/lib/Option/OptTable.cpp
===
--- llvm/lib/Option/OptTable.cpp
+++ llvm/lib/Option/OptTable.cpp
@@ -610,8 +610,10 @@
   continue;
 
 unsigned Flags = getInfo(Id).Flags;
+
 if (FlagsToInclude && !(Flags & FlagsToInclude))
   continue;
+
 if (Flags & FlagsToExclude)
   continue;
 
Index: llvm/include/llvm/Option/OptTable.h
===
--- llvm/include/llvm/Option/OptTable.h
+++ llvm/include/llvm/Option/OptTable.h
@@ -48,7 +48,7 @@
 unsigned ID;
 unsigned char Kind;
 unsigned char Param;
-unsigned short Flags;
+unsigned int Flags;
 unsigned short GroupID;
 unsigned short AliasID;
 const char *AliasArgs;
@@ -225,7 +225,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/CompilerInstanceTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -0,0 +1,52 @@
+//===- unittests/Frontend/CompilerInstanceTest.cpp - CI 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 "gtest/gtest.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Options.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include 
+using namespace llvm;
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(CompilerInstance, AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
+  // 1. Set-up a basic DiagnosticConsumer
+  std::string diagnosticOutput;
+  llvm::raw_string_ostream diagnosticsOS(diagnosticOutput);
+  auto diagPrinter = std::make_unique(
+  diagnosticsOS, new clang::DiagnosticOptions());
+
+  // 2. Create a CompilerInstance (to manage a DiagnosticEngine)
+  CompilerInstance compInst;
+
+  // 3. Set-up DiagnosticOptions
+  auto diagOpts = new clang::DiagnosticOptions();
+  // Tell the diagnostics engine to emit the diagnostic log to STDERR. This
+  // ensures that a chained diagnostic consumer is created so that the test can
+  

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D86089#2256139 , 
@richard.barton.arm wrote:

> Another random thought that just came to me: what does the new driver do when 
> you invoke it with no input files or options? I could imagine a few sensible 
> outcomes (error: no input (clang and gcc/gfortran behaviour), print version 
> and exit, print help and exit, etc.) and I don't have a strong preference 
> over which we chose here but I think there should be a test for it in 
> test/Driver

`flang-new` inherits generic things like this from libclangDriver. In this case:

  error: no input files

This is consistent with `clang`. I've added a test for this. As for `flang-new 
-fc1` - it in general does very little right now :) `clang -cc1` just sits 
there waiting




Comment at: flang/test/CMakeLists.txt:8
 
+llvm_canonicalize_cmake_booleans(FLANG_BUILD_NEW_DRIVER)
+

richard.barton.arm wrote:
> So do the other bools like LINK_WITH_FIR also need this treatment and this is 
> a latent bug? Seems amazing we've not hit that before now.
> 
> From an offline conversation ISTR you saying this was to do with how the 
> variable is translated into the lit config? If that is the case then I think 
> there is a function you can use called lit.util.pythonize_bool which converts 
> various strings that mean "True/False" into a real bool for python. That 
> would also let you clean up the lit.cfg.py later on (separate comments).
AFAIK, `LINK_WITH_FIR` is not used in LIT tests.

I switched to `lit.util.pythonize_bool `, thanks for the suggestion!



Comment at: flang/test/Flang-Driver/driver-error-cc1.c:7
+
+// CHECK:error: unknown integrated tool '-cc1'. Valid tools include '-fc1'.

richard.barton.arm wrote:
> I am surprised that you don't need to prefix this run line with 'not' 
> indicating that flang-new returns 0 exit code in this instance, which seems 
> wrong to me.
Good catch, thanks! Fixed in the latest revision.



Comment at: flang/test/Flang-Driver/driver-error-cc1.cpp:1
+// RUN: %flang-new %s 2>&1 | FileCheck %s
+

richard.barton.arm wrote:
> Same comment as above on exit code.
Good catch, thanks! Fixed in the latest revision.



Comment at: flang/test/Flang-Driver/emit-obj.f90:2
+! RUN: %flang-new  %s 2>&1 | FileCheck %s --check-prefix=ERROR
+! RUN: %flang-new  -fc1 -emit-obj %s 2>&1 | FileCheck %s --check-prefix=ERROR
+

richard.barton.arm wrote:
> Seems like it would be helpful to have also implemented `-###` in this patch 
> so that you don't need to write tests like this. You could simply run 
> flang-new -### then check the -fc1 line that would have been emitted for the 
> presence of -emit-obj.
> 
> Same comment above regarding exit code.
> Seems like it would be helpful to have also implemented -### in this patch

As`flang-new` is based on libclangDriver, we get `-###` for free (i.e. it's 
already supported).

However, there's a catch: 
https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Driver/Options.td#L369-L370.
 Currently `flang-new --help` will not display `-###`  because the the 
corresponding option definition hasn't been updated (i.e. it is not flagged as 
a Flang option):
```
def _HASH_HASH_HASH : Flag<["-"], "###">, Flags<[DriverOption, CoreOption]>,
HelpText<"Print (but do not run) the commands to run for this compilation">;
``` 
We have three options:
1. Make `flang-new --help` display options that are flagged as `DriverOption` 
or `CoreOption`. Note this will include many other options (currently 
unsupported) and extra filtering would be required.
2. Add `FlangOption` to the definition of `_HASH_HASH_HASH`, but IMO that's a 
bit controversial. Is that what we want to do long-term? Or shall we create a 
separate category for generic driver options that apply to both Clang and Flang?
3. Delay the decision until there is more code to base our design on.

I'm in favor of Option 3.



Comment at: flang/test/lit.cfg.py:39
 # directories.
+# exclude the tests for flang_new driver while there are two drivers
 config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt']

richard.barton.arm wrote:
> This comment should be on line 41
Updated, ta!



Comment at: flang/test/lit.site.cfg.py.in:14
+# control the regression test for flang-new driver
+config.include_flang_new_driver_test="@FLANG_BUILD_NEW_DRIVER@"
+

richard.barton.arm wrote:
> awarzynski wrote:
> > `FLANG_BUILD_NEW_DRIVER` should be canonicalized in CMake first: 
> > https://github.com/llvm/llvm-project/blob/c79a366ec0f87eafca123138b550b039618bf880/llvm/cmake/modules/AddLLVM.cmake#L1424-L1435
> I think it might be better to use lit.util.pythonize_bool to do the 
> canonicalisation. It would let you use config.include_flang_new_driver_test 
> as a real bool later in the file rather 

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 290337.
awarzynski marked 2 inline comments as done.
awarzynski added a comment.

- Adddressed comments from @richard.barton.arm
- Added FC1Option to ClangFlags (and made other related changes)
- Added missing code to check return codes from subcommands (flang-new)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

Files:
  clang/include/clang/Driver/Driver.h
  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/Frontend/CreateInvocationFromCommandLine.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/unittests/Driver/SanitizerArgsTest.cpp
  clang/unittests/Driver/ToolChainTest.cpp
  flang/CMakeLists.txt
  flang/README.md
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/CMakeLists.txt
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Flang-Driver/driver-error-cc1.c
  flang/test/Flang-Driver/driver-error-cc1.cpp
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/driver-version.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/Flang-Driver/missing-input.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/CMakeLists.txt
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  llvm/include/llvm/Option/OptTable.h
  llvm/lib/Option/OptTable.cpp

Index: llvm/lib/Option/OptTable.cpp
===
--- llvm/lib/Option/OptTable.cpp
+++ llvm/lib/Option/OptTable.cpp
@@ -610,8 +610,10 @@
   continue;
 
 unsigned Flags = getInfo(Id).Flags;
+
 if (FlagsToInclude && !(Flags & FlagsToInclude))
   continue;
+
 if (Flags & FlagsToExclude)
   continue;
 
Index: llvm/include/llvm/Option/OptTable.h
===
--- llvm/include/llvm/Option/OptTable.h
+++ llvm/include/llvm/Option/OptTable.h
@@ -48,7 +48,7 @@
 unsigned ID;
 unsigned char Kind;
 unsigned char Param;
-unsigned short Flags;
+unsigned int Flags;
 unsigned short GroupID;
 unsigned short AliasID;
 const char *AliasArgs;
@@ -225,7 +225,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/CompilerInstanceTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -0,0 +1,52 @@
+//===- unittests/Frontend/CompilerInstanceTest.cpp - CI 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 "gtest/gtest.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Options.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include 
+using namespace llvm;
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(CompilerInstance, AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
+  // 1. Set-up a basic DiagnosticConsumer
+  std::string diagnosticOutput;
+  llvm::raw_string_ostream diagnosticsOS(diagnosticOutput);
+  auto diagPrinter = std::make_unique(
+  diagnosticsOS, new clang::DiagnosticOptions());
+
+  // 2. Create a CompilerInstance (to manage a DiagnosticEngine)
+  CompilerInstance compInst;
+
+  // 3. Set-up DiagnosticOptions
+  auto 

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

Another random thought that just came to me: what does the new driver do when 
you invoke it with no input files or options? I could imagine a few sensible 
outcomes (error: no input (clang and gcc/gfortran behaviour), print version and 
exit, print help and exit, etc.) and I don't have a strong preference over 
which we chose here but I think there should be a test for it in test/Driver


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.
This revision now requires changes to proceed.

Requesting changes mostly because of the exit status issue on the Driver tests.

A few general questions as well:

1. Why not implement `-###` as part of this patch to enable testing more easily?
2. How come there are no regression tests for -fc1 in flang/test/Frontend? I 
suppose these come when the first real FrontendAction is implemented?




Comment at: flang/test/CMakeLists.txt:8
 
+llvm_canonicalize_cmake_booleans(FLANG_BUILD_NEW_DRIVER)
+

So do the other bools like LINK_WITH_FIR also need this treatment and this is a 
latent bug? Seems amazing we've not hit that before now.

From an offline conversation ISTR you saying this was to do with how the 
variable is translated into the lit config? If that is the case then I think 
there is a function you can use called lit.util.pythonize_bool which converts 
various strings that mean "True/False" into a real bool for python. That would 
also let you clean up the lit.cfg.py later on (separate comments).



Comment at: flang/test/Flang-Driver/driver-error-cc1.c:7
+
+// CHECK:error: unknown integrated tool '-cc1'. Valid tools include '-fc1'.

I am surprised that you don't need to prefix this run line with 'not' 
indicating that flang-new returns 0 exit code in this instance, which seems 
wrong to me.



Comment at: flang/test/Flang-Driver/driver-error-cc1.cpp:1
+// RUN: %flang-new %s 2>&1 | FileCheck %s
+

Same comment as above on exit code.



Comment at: flang/test/Flang-Driver/emit-obj.f90:2
+! RUN: %flang-new  %s 2>&1 | FileCheck %s --check-prefix=ERROR
+! RUN: %flang-new  -fc1 -emit-obj %s 2>&1 | FileCheck %s --check-prefix=ERROR
+

Seems like it would be helpful to have also implemented `-###` in this patch so 
that you don't need to write tests like this. You could simply run flang-new 
-### then check the -fc1 line that would have been emitted for the presence of 
-emit-obj.

Same comment above regarding exit code.



Comment at: flang/test/lit.cfg.py:39
 # directories.
+# exclude the tests for flang_new driver while there are two drivers
 config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt']

This comment should be on line 41



Comment at: flang/test/lit.cfg.py:40
+# exclude the tests for flang_new driver while there are two drivers
+if config.include_flang_new_driver_test == "OFF":
+  config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt', 
'Flang-Driver']

awarzynski wrote:
> CarolineConcatto wrote:
> > richard.barton.arm wrote:
> > > richard.barton.arm wrote:
> > > > I think it would be cleaner to define config.excludes unconditionally 
> > > > then append the Flang-Driver dir if our condition passes.
> > > I _think_ the pattern to follow to exclude tests for something you 
> > > haven't built is to use lit features.
> > > 
> > > So you would add a feature like:
> > > `config.available_features.add("new-driver")`
> > > 
> > > then each test that only works on the new driver would be prefixed with a 
> > > statement:
> > > 
> > > `REQUIRES: new-driver`
> > > 
> > > This means that the tests can go in the test/Driver directory and you 
> > > don't need to create a new directory for these tests or this hack. The 
> > > additional benefit would be that all the existing tests for the throwaway 
> > > driver can be re-used on the new Driver to test it. There are not many of 
> > > those though and we are using a different driver name so they can't be 
> > > shared either. Still think it would be a worthwhile thing to do because 
> > > when looking at the test itself it is clear why it is not being run 
> > > whereas with this hack it is hidden away.
> > > 
> > >  Sorry for not thinking this first time around.
> > I like to have the test at a different folder for now so it is clear that 
> > the tests inside this folder all belongs to the new driver. So I don't need 
> > to open the test to know.
> > I can implement the requires, but in the future will not need the REQUIRES 
> > for the driver test.
> Let me clarify a bit. I assume that `FLANG_BUILD_NEW_DRIVER` is Off.
> 
> SCENARIO 1: In order to make sure that `bin/llvm-lit tools/flang/test/` does 
> not _attempt to run_ the new driver tests, add:
>  `config.excludes.append('Flang-Driver')`
> 
> With this, the new driver tests will neither be run nor summarized (e.g. as 
> `UNSUPPORTED`) in the final output.
> 
> SCENARIO 2: `config.excludes.append('Flang-Driver')` will not affect 
> `bin/llvm-lit tools/flang/test/Flang-Driver` (this time I explicitly specify 
> the `Flang-Driver` directory). We need:
> 
> `REQUIERES: new-flang-driver`
> 
> (or similar) for the new Flang driver tests to be reported as `UNSUPPORTED`.
> 
> I agree with 

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 289111.
awarzynski marked an inline comment as done.
awarzynski added a comment.

Update README.md with instructions for building `flang-new`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

Files:
  clang/include/clang/Driver/Driver.h
  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/Frontend/CreateInvocationFromCommandLine.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/unittests/Driver/SanitizerArgsTest.cpp
  clang/unittests/Driver/ToolChainTest.cpp
  flang/CMakeLists.txt
  flang/README.md
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/CMakeLists.txt
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Flang-Driver/driver-error-cc1.c
  flang/test/Flang-Driver/driver-error-cc1.cpp
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/driver-version.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/CMakeLists.txt
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  llvm/include/llvm/Option/OptTable.h
  llvm/lib/Option/OptTable.cpp

Index: llvm/lib/Option/OptTable.cpp
===
--- llvm/lib/Option/OptTable.cpp
+++ llvm/lib/Option/OptTable.cpp
@@ -610,9 +610,12 @@
   continue;
 
 unsigned Flags = getInfo(Id).Flags;
-if (FlagsToInclude && !(Flags & FlagsToInclude))
+
+unsigned ExplicitelyIncluded = Flags & FlagsToInclude;
+if (FlagsToInclude && !(ExplicitelyIncluded))
   continue;
-if (Flags & FlagsToExclude)
+
+if (!(ExplicitelyIncluded) && (Flags & FlagsToExclude))
   continue;
 
 // If an alias doesn't have a help text, show a help text for the aliased
Index: llvm/include/llvm/Option/OptTable.h
===
--- llvm/include/llvm/Option/OptTable.h
+++ llvm/include/llvm/Option/OptTable.h
@@ -225,7 +225,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/CompilerInstanceTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -0,0 +1,52 @@
+//===- unittests/Frontend/CompilerInstanceTest.cpp - CI 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 "gtest/gtest.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Options.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include 
+using namespace llvm;
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(CompilerInstance, AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
+  // 1. Set-up a basic DiagnosticConsumer
+  std::string diagnosticOutput;
+  llvm::raw_string_ostream diagnosticsOS(diagnosticOutput);
+  auto diagPrinter = std::make_unique(
+  diagnosticsOS, new clang::DiagnosticOptions());
+
+  // 2. Create a CompilerInstance (to manage a DiagnosticEngine)
+  CompilerInstance compInst;
+
+  // 3. Set-up DiagnosticOptions
+  auto diagOpts = new clang::DiagnosticOptions();
+  // Tell the diagnostics engine to emit the diagnostic log to STDERR. This
+  // ensures that a chained 

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi accepted this revision.
sameeranjoshi added a comment.

Thank you for changes.
I was able to build successfully out-of-tree.
Please update the `README.md` with the necessary changes.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski commandeered this revision.
awarzynski added a reviewer: CarolineConcatto.
awarzynski added a comment.

@sameeranjoshi Thanks for testing out-of-tree builds. We were missing some 
minor changes in CMake. I have now uploaded these. Out-of-tree will require 
`-DCLANG_DIR` when configuring build while the new driver depends on Clang.

As for your original issue - I suspect that you forgot to update your in-tree 
directory. This patch needs to be applied both in-tree (in `llvm-project`) and 
out-of-tree (`flang`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 288424.
awarzynski added a comment.

Added support for out-of-tree builds. It requires 
-DCLANG_DIR=/lib/cmake/clang when configuring CMake. That's on 
top of LLVM_DIR and MLIR_DIR.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

Files:
  clang/include/clang/Driver/Driver.h
  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/Frontend/CreateInvocationFromCommandLine.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/unittests/Driver/SanitizerArgsTest.cpp
  clang/unittests/Driver/ToolChainTest.cpp
  flang/CMakeLists.txt
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/CMakeLists.txt
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Flang-Driver/driver-error-cc1.c
  flang/test/Flang-Driver/driver-error-cc1.cpp
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/driver-version.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/CMakeLists.txt
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  llvm/include/llvm/Option/OptTable.h
  llvm/lib/Option/OptTable.cpp

Index: llvm/lib/Option/OptTable.cpp
===
--- llvm/lib/Option/OptTable.cpp
+++ llvm/lib/Option/OptTable.cpp
@@ -610,9 +610,12 @@
   continue;
 
 unsigned Flags = getInfo(Id).Flags;
-if (FlagsToInclude && !(Flags & FlagsToInclude))
+
+unsigned ExplicitelyIncluded = Flags & FlagsToInclude;
+if (FlagsToInclude && !(ExplicitelyIncluded))
   continue;
-if (Flags & FlagsToExclude)
+
+if (!(ExplicitelyIncluded) && (Flags & FlagsToExclude))
   continue;
 
 // If an alias doesn't have a help text, show a help text for the aliased
Index: llvm/include/llvm/Option/OptTable.h
===
--- llvm/include/llvm/Option/OptTable.h
+++ llvm/include/llvm/Option/OptTable.h
@@ -225,7 +225,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/CompilerInstanceTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -0,0 +1,52 @@
+//===- unittests/Frontend/CompilerInstanceTest.cpp - CI 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 "gtest/gtest.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Options.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include 
+using namespace llvm;
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(CompilerInstance, AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
+  // 1. Set-up a basic DiagnosticConsumer
+  std::string diagnosticOutput;
+  llvm::raw_string_ostream diagnosticsOS(diagnosticOutput);
+  auto diagPrinter = std::make_unique(
+  diagnosticsOS, new clang::DiagnosticOptions());
+
+  // 2. Create a CompilerInstance (to manage a DiagnosticEngine)
+  CompilerInstance compInst;
+
+  // 3. Set-up DiagnosticOptions
+  auto diagOpts = new clang::DiagnosticOptions();
+  // Tell the diagnostics engine to emit the diagnostic log to STDERR. This
+  // 

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@andwar is my Phabricator alter-ego. Apologies for the confusion - I had my 
Arcanist misconfigured.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

andwar updated this revision to Diff 288018.
andwar added a comment.

- Canonicalised FLANG_BUILD_NEW_DRIVER in flang/test/CMakeLists.txt and updated 
the LIT config scripts accordingly
- Implemented proper handling of `-emit-obj` (via diagnostics) in 
CompilerInvocation.cpp
- Removed members from InputKind which are not needed (and the related methods)
- Removed/updated comments, added new comments to clarify the design and to 
highlight the future steps
- Simplified the original change in OptTable::PrintHelp (made it more generic)
- Removed unused #includes
- Fine-tuned the tests (I wanted to make sure that they clearly communicate 
what is currently supported and what is not)
- Addressed all the outstanding comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

Files:
  clang/include/clang/Driver/Driver.h
  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/Frontend/CreateInvocationFromCommandLine.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/unittests/Driver/SanitizerArgsTest.cpp
  clang/unittests/Driver/ToolChainTest.cpp
  flang/CMakeLists.txt
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/CMakeLists.txt
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Flang-Driver/driver-error-cc1.c
  flang/test/Flang-Driver/driver-error-cc1.cpp
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/driver-version.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/CMakeLists.txt
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  llvm/include/llvm/Option/OptTable.h
  llvm/lib/Option/OptTable.cpp

Index: llvm/lib/Option/OptTable.cpp
===
--- llvm/lib/Option/OptTable.cpp
+++ llvm/lib/Option/OptTable.cpp
@@ -610,9 +610,12 @@
   continue;
 
 unsigned Flags = getInfo(Id).Flags;
-if (FlagsToInclude && !(Flags & FlagsToInclude))
+
+unsigned ExplicitelyIncluded = Flags & FlagsToInclude;
+if (FlagsToInclude && !(ExplicitelyIncluded))
   continue;
-if (Flags & FlagsToExclude)
+
+if (!(ExplicitelyIncluded) && (Flags & FlagsToExclude))
   continue;
 
 // If an alias doesn't have a help text, show a help text for the aliased
Index: llvm/include/llvm/Option/OptTable.h
===
--- llvm/include/llvm/Option/OptTable.h
+++ llvm/include/llvm/Option/OptTable.h
@@ -225,7 +225,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/CompilerInstanceTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -0,0 +1,52 @@
+//===- unittests/Frontend/CompilerInstanceTest.cpp - CI 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 "gtest/gtest.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Options.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include 
+using namespace llvm;
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(CompilerInstance, 

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@CarolineConcatto - thank you for working on this! @all, thank you for your 
reviews, this is much appreciated!

While Carol is away, I'll try my best to address all the outstanding comments. 
I will also update the patch accordingly. I've identified a few small issues 
myself - that will also be updated in the next patch.




Comment at: clang/lib/Driver/Driver.cpp:1573
+ExcludedFlagsBitmask |= options::CC1Option;
+IncludedFlagsBitmask |= options::FlangOption;
+  } else

sameeranjoshi wrote:
> In `enum ClangFlags` 
> inside File `clang/include/clang/Driver/Options.h`
> there are various other options do they need to be considered?
> If not, how are they handled?
> 
> do they need to be considered?

Not at this stage. This is sufficient to make sure that `flang-new -help` only 
prints options relevant to Flang.

We may want to fine-tune this in the future, but currently it would be a bit 
tricky with only 2 options being  supported ;-)



Comment at: flang/include/flang/Frontend/CompilerInstance.h:11
+
+#include "flang/Frontend/CompilerInvocation.h"
+

CarolineConcatto wrote:
> tskeith wrote:
> > Why is this called "Frontend" rather than "Driver"?
> The way we think is that the driver is split in:
> **the driver** libDriver -> that implements the driver. ATM it is inside 
> clang/lib/Driver. And it can be clang( called inside 
> clang/tools/driver/driver.cpp) or flang-new(called inside 
> flang/tools/driver/driver.cpp) according to the mode the driver is called.
> **the frontend driver**  -> it can be:
>  clang frontend driver: clang -cc1  that calls libclangfrontend 
>  flang frontend driver: flang-new -fc1 that calls libflangfrontend. 
> the -xc1 has its functions/methods implemented inside the driver Frontend 
> 
> So this folder is called Frontend because it belongs to the part that 
> implements the driver frontend of the compiler and we would like to leave 
> this way so it has the same design as clang.
>  
Just to further clarify, I think that it's important to distinguish between:

* the compiler driver, `flang`, implemented in terms of `libclandDriver` (it 
creates and executes jobs/commands, e.g. linker or  frontend jobs)
* the frontend driver, `flang -fc1`, implemented in terms of `libflangFrontend` 
(it creates frontend actions, e.g. preprocessor actions)

"Frontend" in this case means "Frontend Driver".  "Driver" is reserved for the 
compiler driver, i.e. "libclangDriver".  

As Carol mentioned, our terminology is inspired by Clang. Keeping the names 
consistent felt like a good idea. @tskeith, would you prefer "FrontendDriver" 
instead of "Frontend"?

(`libclangDriver` is the Clang compiler driver library that we want to re-use, 
`libflangFrontend` is the Flang frontend driver library, inspired by 
`libclangFrontend`)



Comment at: flang/include/flang/Frontend/FrontendOptions.h:31-32
+  Language lang_;
+  unsigned fmt_ : 3;
+  unsigned preprocessed_ : 1;
+

> Why isn't the type bool?

@tskeith Bitfields are used here for optimization. An instance of `InputKind` 
is created per input file, so this could be a substantial saving (if the 
bitfields happen to be laid out nicely).

Btw,  we won't need `fmt_` or `preprocessed_` (and the corresponding member 
methods) just now. I will delete them.



Comment at: flang/include/flang/Frontend/FrontendOptions.h:65
+  /// Show the -version text.
+  unsigned ShowVersion : 1;
+

CarolineConcatto wrote:
> tskeith wrote:
> > Why aren't the types `bool`?
> Most of these things are going to be set by option::driver. That is the 
> reason it is as it is.
> Why aren't the types bool?

@tskeith See above.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:46
+  InputKind DashX(Language::Unknown);
+  return DashX;
+}

CarolineConcatto wrote:
> tskeith wrote:
> > Why not `return InputKind{Language::Unknown};`?
> Just because the next lines also return DashX.
I agree with @tskeith , `return InputKind{Language::Unknown};` would be 
cleaner. I will update this.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:91
+  InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags);
+  (void)DashX;
+

tskeith wrote:
> What is the purpose of `DashX` here?
Not needed for now, I will delete it. Thanks!



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:20
+
+using namespace flang;
+namespace flang {

CarolineConcatto wrote:
> tskeith wrote:
> > What is this for?
> Because of CompilerInstance *Flang.
It's not needed, good catch, cheers!



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:39
+
+  return true;
+}

sameeranjoshi wrote:
> The comment in header for `ExecuteCompilerInvocation` mentions,
> ```
>   

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX added inline comments.



Comment at: flang/tools/flang-driver/fc1_main.cpp:1
+//===-- fc1_main.cpp - Flang FC1 Compiler Frontend 
===//
+//

This file-name seems odd considering LLVM style. How about just `Flang.cpp` or 
`FlangMain.cpp` ? or other


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Caroline via Phabricator via cfe-commits]

CarolineConcatto marked 17 inline comments as done.
CarolineConcatto added a comment.

Hey Folks,
Fist of all, thank you very much for your review. 
I have answered some questions and update some of the requests. 
There are some that I did not had time to go inside yet.
I have the week of 24th off. IN my return I will tackle all the reviews as:
 out of build tree 
 and some question about the driver




Comment at: flang/include/flang/Frontend/CompilerInstance.h:11
+
+#include "flang/Frontend/CompilerInvocation.h"
+

tskeith wrote:
> Why is this called "Frontend" rather than "Driver"?
The way we think is that the driver is split in:
**the driver** libDriver -> that implements the driver. ATM it is inside 
clang/lib/Driver. And it can be clang( called inside 
clang/tools/driver/driver.cpp) or flang-new(called inside 
flang/tools/driver/driver.cpp) according to the mode the driver is called.
**the frontend driver**  -> it can be:
 clang frontend driver: clang -cc1  that calls libclangfrontend 
 flang frontend driver: flang-new -fc1 that calls libflangfrontend. 
the -xc1 has its functions/methods implemented inside the driver Frontend 

So this folder is called Frontend because it belongs to the part that 
implements the driver frontend of the compiler and we would like to leave this 
way so it has the same design as clang.
 



Comment at: flang/include/flang/Frontend/CompilerInstance.h:21
+  /// The options used in this compiler instance.
+  std::shared_ptr Invocation;
+

tskeith wrote:
> Data members, local variables, etc. should start with lower case.
> Non-public data members should end with underscore.
> 
> Please follow the style in flang/documentation/C++style.md.
I believe I have changed the style of the patch to be flang style.



Comment at: flang/include/flang/Frontend/FrontendOptions.h:31
+  Language Lang;
+  unsigned Fmt : 3;
+  unsigned Preprocessed : 1;

tskeith wrote:
> Why isn't the type `Format`?
This is to not mistake with: enum Format { Source, ModuleMap, Precompiled };



Comment at: flang/include/flang/Frontend/FrontendOptions.h:32
+  unsigned Fmt : 3;
+  unsigned Preprocessed : 1;
+

tskeith wrote:
> Why isn't the type `bool`?
Most of these things are going to be set by option::driver.  That is the reason 
it is as it is.



Comment at: flang/include/flang/Frontend/FrontendOptions.h:65
+  /// Show the -version text.
+  unsigned ShowVersion : 1;
+

tskeith wrote:
> Why aren't the types `bool`?
Most of these things are going to be set by option::driver. That is the reason 
it is as it is.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:46
+  InputKind DashX(Language::Unknown);
+  return DashX;
+}

tskeith wrote:
> Why not `return InputKind{Language::Unknown};`?
Just because the next lines also return DashX.



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:20
+
+using namespace flang;
+namespace flang {

tskeith wrote:
> What is this for?
Because of CompilerInstance *Flang.



Comment at: flang/test/lit.cfg.py:40
+# exclude the tests for flang_new driver while there are two drivers
+if config.include_flang_new_driver_test == "OFF":
+  config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt', 
'Flang-Driver']

richard.barton.arm wrote:
> richard.barton.arm wrote:
> > I think it would be cleaner to define config.excludes unconditionally then 
> > append the Flang-Driver dir if our condition passes.
> I _think_ the pattern to follow to exclude tests for something you haven't 
> built is to use lit features.
> 
> So you would add a feature like:
> `config.available_features.add("new-driver")`
> 
> then each test that only works on the new driver would be prefixed with a 
> statement:
> 
> `REQUIRES: new-driver`
> 
> This means that the tests can go in the test/Driver directory and you don't 
> need to create a new directory for these tests or this hack. The additional 
> benefit would be that all the existing tests for the throwaway driver can be 
> re-used on the new Driver to test it. There are not many of those though and 
> we are using a different driver name so they can't be shared either. Still 
> think it would be a worthwhile thing to do because when looking at the test 
> itself it is clear why it is not being run whereas with this hack it is 
> hidden away.
> 
>  Sorry for not thinking this first time around.
I like to have the test at a different folder for now so it is clear that the 
tests inside this folder all belongs to the new driver. So I don't need to open 
the test to know.
I can implement the requires, but in the future will not need the REQUIRES for 
the driver test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Caroline via Phabricator via cfe-commits]

CarolineConcatto updated this revision to Diff 287270.
CarolineConcatto added a comment.

Address review comments

Replace namespace flang for Fortran::frontend 
and fix style


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

Files:
  clang/include/clang/Driver/Driver.h
  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/Frontend/CreateInvocationFromCommandLine.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/unittests/Driver/SanitizerArgsTest.cpp
  clang/unittests/Driver/ToolChainTest.cpp
  flang/CMakeLists.txt
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/CMakeLists.txt
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Flang-Driver/driver-error-cc1.c
  flang/test/Flang-Driver/driver-error-cc1.cpp
  flang/test/Flang-Driver/driver-error-diagnostic.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/driver-version.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/CMakeLists.txt
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  llvm/lib/Option/OptTable.cpp

Index: llvm/lib/Option/OptTable.cpp
===
--- llvm/lib/Option/OptTable.cpp
+++ llvm/lib/Option/OptTable.cpp
@@ -612,7 +612,19 @@
 unsigned Flags = getInfo(Id).Flags;
 if (FlagsToInclude && !(Flags & FlagsToInclude))
   continue;
-if (Flags & FlagsToExclude)
+// If `Flags` is in both the Exclude set and in the Include set - display
+// it.
+if ((Flags & FlagsToExclude) && !(Flags & FlagsToInclude))
+  continue;
+
+// If `Flags` is empty (i.e. it's an option without any flags) then this is
+// a Clang-only option. If:
+// * we _are not_ in Flang Mode (FlagsToExclude contains FlangMode), then
+//   display it.
+// * we _are_ in Flang mode (FlagsToExclude does not contain FlangMode),
+//  don't display it.
+if (!Flags &&
+(FlagsToExclude & /*clang::driver::options::FlangMode*/ (1 << 14)))
   continue;
 
 // If an alias doesn't have a help text, show a help text for the aliased
Index: flang/unittests/Frontend/CompilerInstanceTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -0,0 +1,52 @@
+//===- unittests/Frontend/CompilerInstanceTest.cpp - CI 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 "gtest/gtest.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Options.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include 
+using namespace llvm;
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(CompilerInstance, AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
+  // 1. Set-up a basic DiagnosticConsumer
+  std::string diagnosticOutput;
+  llvm::raw_string_ostream diagnosticsOS(diagnosticOutput);
+  auto diagPrinter = std::make_unique(
+  diagnosticsOS, new clang::DiagnosticOptions());
+
+  // 2. Create a CompilerInstance (to manage a DiagnosticEngine)
+  CompilerInstance compInst;
+
+  // 3. Set-up DiagnosticOptions
+  auto diagOpts = new clang::DiagnosticOptions();
+  // Tell the diagnostics engine to emit the diagnostic log to STDERR. This
+  // ensures that a chained diagnostic consumer is created so that the test can
+  // exercise the unowned diagnostic consumer in a chained consumer.
+  diagOpts->DiagnosticLogFile = "-";
+
+  // 4. Create a DiagnosticEngine with an unowned consumer
+  IntrusiveRefCntPtr diags =
+  compInst.CreateDiagnostics(diagOpts, diagPrinter.get(),
+  /*ShouldOwnClient=*/false);
+
+  // 5. Report a diagnostic
+  

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi requested changes to this revision.
sameeranjoshi added a comment.

Thanks for working on it.
A few review comments/questions on changes in `flang` part from the patch.




Comment at: flang/include/flang/Frontend/CompilerInstance.h:93
+
+  static clang::IntrusiveRefCntPtr createDiagnostics(
+  clang::DiagnosticOptions *Opts,

The block of comments above make sense for this function and not the currently 
mentioned one.
Please interchange/replace the comments to later declared function.
Wrong comments above could reflect in `doxygen APIs`, misleading the reader of 
code.



Comment at: flang/include/flang/FrontendTool/Utils.h:1
+
+//===--- Utils.h - Misc utilities for the flang front-end *- 
C++-*-===//

`nit:`: blank line.



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:35
+  if (Flang->getFrontendOpts().ShowVersion) {
+llvm::cl::PrintVersionMessage();
+return true;

With 
```
clang --version
clang version 11.0.0 (https://github.com/llvm/llvm-project.git 
1acf129bcf9a1b51e301a9fef151254ec4c7ec43)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/bin
```
whereas with 
```
./bin/flang-new --vesion
Flang experimental driver (flang-new)
```

I see both `clang` & `flang` call
`llvm::cl::PrintVersionMessage()` internally.

Is more information need to be registered in llvm(`llvm::cl`) for flang to give 
more detailed output or will that come later once we start adding more patch 
for driver?



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:39
+
+  return true;
+}

The comment in header for `ExecuteCompilerInvocation` mentions,
```
/// \return - True on success.
bool ExecuteCompilerInvocation(CompilerInstance *Flang);
```

Do we need to have a `false` somewhere here?

I see 2 scenarios when `ExecuteCompilerInvocation` might fail (there could 
definitely be more) and there we need an indication of failure by returning 
`false`,
1. When there is no actual execution of compiler.
2. The compilation in not successful.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added inline comments.



Comment at: flang/test/lit.cfg.py:40
+# exclude the tests for flang_new driver while there are two drivers
+if config.include_flang_new_driver_test == "OFF":
+  config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt', 
'Flang-Driver']

richard.barton.arm wrote:
> I think it would be cleaner to define config.excludes unconditionally then 
> append the Flang-Driver dir if our condition passes.
I _think_ the pattern to follow to exclude tests for something you haven't 
built is to use lit features.

So you would add a feature like:
`config.available_features.add("new-driver")`

then each test that only works on the new driver would be prefixed with a 
statement:

`REQUIRES: new-driver`

This means that the tests can go in the test/Driver directory and you don't 
need to create a new directory for these tests or this hack. The additional 
benefit would be that all the existing tests for the throwaway driver can be 
re-used on the new Driver to test it. There are not many of those though and we 
are using a different driver name so they can't be shared either. Still think 
it would be a worthwhile thing to do because when looking at the test itself it 
is clear why it is not being run whereas with this hack it is hidden away.

 Sorry for not thinking this first time around.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added a comment.

Thanks for the work.

A couple of comments on `clang/` related changes:
An `out-of-tree` build with this patch fails for me:
Here's what I did:
I initially used `ENABLE_PROJECTS="clang;mlir"` to build `llvm-project`, I 
didn't build `flang` during this run.

Then I passed following to the cmake when building `flang` as out of tree.

  cmake -G Ninja \
-DCMAKE_BUILD_TYPE=Release \
-DFLANG_ENABLE_WERROR=On \
-DCMAKE_CXX_STANDARD=17 \
-DLLVM_TARGETS_TO_BUILD=host \
-DLLVM_LIT_ARGS=-v \
-D LLVM_DIR=${LLVM_MLIR_CLANG_BUILD}/lib/cmake/llvm \
-D MLIR_DIR=${LLVM_MLIR_CLANG_BUILD}/lib/cmake/mlir \
-DBUILD_FLANG_NEW_DRIVER=ON \
../
  ninja -j16

where `LLVM_MLIR_CLANG_BUILD` points to initially built `llvm-project` .
I see below error message:

  ../lib/Frontend/CompilerInvocation.cpp: In static member function ‘static 
bool flang::CompilerInvocation::CreateFromArgs(flang::CompilerInvocation&, 
llvm::ArrayRef, clang::DiagnosticsEngine&)’:
  ../lib/Frontend/CompilerInvocation.cpp:85:67: error: ‘FlangOption’ is not a 
member of ‘clang::driver::options’
 clang::driver::options::CC1Option | 
clang::driver::options::FlangOption;
 ^~~
  ../lib/Frontend/CompilerInvocation.cpp:85:67: note: suggested alternative: 
‘LastOption’
 clang::driver::options::CC1Option | 
clang::driver::options::FlangOption;
 ^~~
 LastOption

Do you need to add `-DCLANG_DIR` flag, as there seem to be a dependency for 
this patch on clang as libraries?




Comment at: clang/include/clang/Driver/Options.td:60
+// FlangOption - This is considered a "core" Flang option, available in
+// flang mode
+def FlangOption : OptionFlag;

`nit:` a missing period.
// flang mode -> // flang mode.



Comment at: clang/include/clang/Driver/Options.td:2076
 def headerpad__max__install__names : Joined<["-"], 
"headerpad_max_install_names">;
-def help : Flag<["-", "--"], "help">, Flags<[CC1Option,CC1AsOption]>,
+def help : Flag<["-", "--"], "help">, 
Flags<[CC1Option,CC1AsOption,FlangOption]>,
   HelpText<"Display available options">;

`nit:` - A space before `FlangOption`
CC1AsOption,FlangOption -> CC1AsOption, FlangOption



Comment at: clang/include/clang/Driver/Options.td:3019
 // We give --version different semantics from -version.
-def _version : Flag<["--"], "version">, Flags<[CoreOption, CC1Option]>,
+def _version : Flag<["--"], "version">, Flags<[CoreOption, 
CC1Option,FlangOption]>,
   HelpText<"Print version information">;

Same as above.
nit: - A space before `FlangOption`



Comment at: clang/lib/Driver/Driver.cpp:1569
 
+  if (Mode == DriverMode::FlangMode) {
+ExcludedFlagsBitmask |= options::CLOption;

Can `IsFlangMode()` be used instead?



Comment at: clang/lib/Driver/Driver.cpp:1573
+ExcludedFlagsBitmask |= options::CC1Option;
+IncludedFlagsBitmask |= options::FlangOption;
+  } else

In `enum ClangFlags` 
inside File `clang/include/clang/Driver/Options.h`
there are various other options do they need to be considered?
If not, how are they handled?



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.

A few specific comments to address here as well as the pre-commit linting ones.




Comment at: clang/lib/Driver/Driver.cpp:1584
 void Driver::PrintVersion(const Compilation , raw_ostream ) const {
+  if (IsFlangMode()) {
+OS << "Flang experimental driver (flang-new)" << '\n';

Instead of this early exit, could we instead of calling getClangFullVersion 
below call getClangToolFullVersion with a different string here?


```
if (IsFlangMode())
  OS >> getClangToolFullVersion("flang-new") << '\n';
else
  OS >> getClangFullVersion();
```

That way, we get the full clang version screen already implemented 'for free' 
and the code is nicer too (IMO)
  



Comment at: flang/CMakeLists.txt:20
 option(LINK_WITH_FIR "Link driver with FIR and LLVM" ON)
+option(BUILD_FLANG_NEW_DRIVER "Build the flang driver frontend" OFF)
 

Generally we should make sure all Flang-specific CMake build configuration 
variables start with FLANG. Suggest this is FLANG_BUILD_NEW_DRIVER or similar.



Comment at: flang/CMakeLists.txt:74
 
+
   if(LINK_WITH_FIR)

Remove



Comment at: flang/test/lit.cfg.py:40
+# exclude the tests for flang_new driver while there are two drivers
+if config.include_flang_new_driver_test == "OFF":
+  config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt', 
'Flang-Driver']

I think it would be cleaner to define config.excludes unconditionally then 
append the Flang-Driver dir if our condition passes.



Comment at: flang/test/lit.cfg.py:64
 tools = [
+  ToolSubst('%flang-new', command=FindTool('flang-new'), unresolved='ignore'),
   ToolSubst('%f18', command=FindTool('f18'),

Rather than always trying to add flang-new and ignoring failure, I think it 
would be better to conditionally add this to `tools` iff we are building the 
new driver and so have `config.include_flang_new_driver_test = "ON"`. This way 
if you are building the new driver and for some reason lit fails to resolve the 
tool then you get a nice error before trying to run tests on a binary that is 
not there.



Comment at: flang/test/lit.site.cfg.py.in:13
 
+# controld the regression test for flang-new driver
+config.include_flang_new_driver_test="@BUILD_FLANG_NEW_DRIVER@"

typo "controld"



Comment at: flang/unittests/CMakeLists.txt:27
+if (BUILD_FLANG_NEW_DRIVER)
+add_subdirectory(Frontend)
+endif()

indentation?



Comment at: llvm/lib/Option/OptTable.cpp:621
+// If `Flags` is empty (i.e. it's an option without any flags) then this is
+// a Clang-only option. If:
+// * we _are not_ in Flang Mode (FlagsToExclude contains FlangMode), then

This is not the sort of change I expect to see in an llvm support library given 
that it seems very clang and flang specific. 

I think this needs to be re-written to be more generic, perhaps tweaking the 
interface to be able to express the logic you want to use for flang and clang.

Why is it not sufficient to call this function unchanged from both flang and 
clang but with a different set of FlagsToInclude/FlagsToExclude passed in using 
this logic to set that on the clang/flang side?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Tim Keith via Phabricator via cfe-commits]

tskeith requested changes to this revision.
tskeith added inline comments.
This revision now requires changes to proceed.



Comment at: flang/include/flang/Frontend/CompilerInstance.h:11
+
+#include "flang/Frontend/CompilerInvocation.h"
+

Why is this called "Frontend" rather than "Driver"?



Comment at: flang/include/flang/Frontend/CompilerInstance.h:16
+
+namespace flang {
+

Nothing else is in namespace `flang`. `Fortran::frontend` would be more 
consistent.



Comment at: flang/include/flang/Frontend/CompilerInstance.h:21
+  /// The options used in this compiler instance.
+  std::shared_ptr Invocation;
+

Data members, local variables, etc. should start with lower case.
Non-public data members should end with underscore.

Please follow the style in flang/documentation/C++style.md.



Comment at: flang/include/flang/Frontend/FrontendOptions.h:31
+  Language Lang;
+  unsigned Fmt : 3;
+  unsigned Preprocessed : 1;

Why isn't the type `Format`?



Comment at: flang/include/flang/Frontend/FrontendOptions.h:32
+  unsigned Fmt : 3;
+  unsigned Preprocessed : 1;
+

Why isn't the type `bool`?



Comment at: flang/include/flang/Frontend/FrontendOptions.h:65
+  /// Show the -version text.
+  unsigned ShowVersion : 1;
+

Why aren't the types `bool`?



Comment at: flang/include/flang/FrontendTool/Utils.h:18
+
+#include 
+

Is this needed?



Comment at: flang/lib/Frontend/CompilerInstance.cpp:39
+  } else
+Diags->setClient(new clang::TextDiagnosticPrinter(llvm::errs(), Opts));
+

The "then" and "else" statements should have braces around them.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:46
+  InputKind DashX(Language::Unknown);
+  return DashX;
+}

Why not `return InputKind{Language::Unknown};`?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:91
+  InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags);
+  (void)DashX;
+

What is the purpose of `DashX` here?



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:20
+
+using namespace flang;
+namespace flang {

What is this for?



Comment at: flang/tools/flang-driver/driver.cpp:30
+extern int fc1_main(
+llvm::ArrayRef Argv, const char *Argv0, void *MainAddr);
+

Why isn't this declared in a header?



Comment at: flang/tools/flang-driver/fc1_main.cpp:32
+int fc1_main(
+llvm::ArrayRef Argv, const char *Argv0, void *MainAddr) {
+

MainAddr isn't used.



Comment at: flang/tools/flang-driver/fc1_main.cpp:54
+  // Execute the frontend actions.
+  { Success = ExecuteCompilerInvocation(Flang.get()); }
+

Why is this statement in a block? Is the result of CreateFromArgs supposed to 
affect the return value of this function?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86089/new/

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits