[flang][driver] Add options for -Werror

[arnamoy10]

Summary

Add the following option for the flang-driver

-Werror

With the option given, warnings are treated as error.

A test case is added.

[arnamoy10]

Updating to use available option from Options.td instead of creating a new option for -Werror

[awarzynski]

Thank you for adding this @arnamoy10 !

I think that in order to match the semantics of -Werror from f18, this patch needs to be extended a bit. In the following two cases, f18 will exit immediately, whereas flang-new -fc1 will happily carry-on:

• https://github.com/llvm/llvm-project/blob/main/flang/tools/f18/f18.cpp#L212-L213
• https://github.com/llvm/llvm-project/blob/main/flang/tools/f18/f18.cpp#L241-L242

Could you try implementing similar semantics here?

I've left a few more comments inline.

flang/lib/Frontend/CompilerInvocation.cpp

331–334

1. This will capture almost anything that matches -W<warning-flag>. Could you verify that it's indeed -Werror?
2. This is a diagnostics rather than Sema option. We are likely to introduce much more with time. Could you move it to a separate method?

flang/test/Semantics/dosemantics03.f90

1–3

Rather than adding the 2nd line, could you update the first line to use %flang_fc1 instead of %f18?

[arnamoy10]

flang/test/Semantics/dosemantics03.f90

1–3

I am not modifying this test case, as it would create a dependency on -std=f2018 up-streamed (due to this particular test case having dependency on language standard). I create a new test case, we can come back to it later.

[arnamoy10]

Thanks @awarzynski for the comments.

1. Moving diagnostics options parsing to a separate function
2. Adding a check so that -Wblah does not trigger the treatment of warnings as werrors, only -Werror triggers it
3. Adding a new test case.

[arnamoy10]

rebasing on top of main

[awarzynski]

I see that -Werror changes the behavior of the driver in 5 different places. I would hope to see 5 new tests to verify each case.

flang/lib/Frontend/CompilerInvocation.cpp

337–338

Please update

flang/lib/Frontend/FrontendActions.cpp

67–69 ↗

(On Diff #332997)

Please add a test for this change.

102–104 ↗

(On Diff #332997)

Please add a test for this change.

116–118 ↗

(On Diff #332997)

Please add a test for this change.

273–275 ↗

(On Diff #332997)

Please add a test for this change.

flang/test/Driver/werror.f90

11–12 ↗

(On Diff #332997)

Does this work for f18?

[arnamoy10]

Modifying the test cases to:

1. Make it work for f18 (when flang-new is not installed)
2. Adding more options and one test case to check correct functionality with PrescanAction and PrescanAndSemaAction

[awarzynski]

I really like how you split your tests into two files:

• werror_scan.f captures warning generated by the prescanner
• werror.f captures warnings from the semantic analysis

In every case you added multiple RUN lines to make sure that the behavior is consistent across multiple actions. I think that that's very useful. Ideally, we'd have one central switch for turning warnings into errors and this would be unnecessary. But we're not there yet. In the meantime, could you add a comment explaining why multiple RUN lines are used?

I have 2 more suggestions:

• add yet another file for warnings from the parser
• rename werror.f as `werror_sema.f

These are non-blockers for me.

flang/lib/Frontend/CompilerInvocation.cpp

344

AFAIK, no other option is allowed in -W<option>. Perhaps throw a diagnostic here?

flang/test/Driver/werror.f90

2 ↗

(On Diff #333843)

[nit] This comment is not directly related to what's in this file (i.e. it doesn't really help understand it, does it)? I would remove it. Same in werror_scan.f.

4–6 ↗

(On Diff #333843)

DELETEME

flang/test/Driver/werror_scan.f

5–7 ↗

(On Diff #333843)

DELETEME

9–11 ↗

(On Diff #333843)

This is going to be either flang-new -fc1 or f18. I think that we can start skipping this part of the comment to make it more generic.

[arnamoy10]

1. Separated test cases to check prescan, parse and sema differently.
2. Diagnostics is thrown when anything other that error is given for -W. This behavior is reflected in f18 as well.
3. Minor changes to address other comments.

[arnamoy10]

Adding newline at the end of a test case.

[awarzynski]

LGTM, thank you for addressing my comments!

[crownyanguan]

How about -Wl option, it will cause link error like " Only -Werror is supported currently"

[arnamoy10]

In D98657#2701673, @crownyanguan wrote:

How about -Wl option, it will cause link error like " Only -Werror is supported currently"

Thanks for pointing it out. The issue was due to restricting -W` options in f18 as well, this has been fixed with 4299ab6c5daf

clang/include/clang/Driver/Options.td

 def flogical_abbreviations : Flag<["-"], "flogical-abbreviations">, Group<f_Group>,
   HelpText<"Enable logical abbreviations">;
 def fno_logical_abbreviations : Flag<["-"], "fno-logical-abbreviations">, Group<f_Group>;
 def fimplicit_none : Flag<["-"], "fimplicit-none">, Group<f_Group>,
   HelpText<"No implicit typing allowed unless overridden by IMPLICIT statements">;
 def fno_implicit_none : Flag<["-"], "fno-implicit-none">, Group<f_Group>;
 def falternative_parameter_statement : Flag<["-"], "falternative-parameter-statement">, Group<f_Group>,
   HelpText<"Enable the old style PARAMETER statement">;
+def werror : Flag<["-"], "Werror">, Group<f_Group>,
+  HelpText<"Make all warnings into errors">;
 }
 
 //===----------------------------------------------------------------------===//
 // FC1 Options
 //===----------------------------------------------------------------------===//
 let Flags = [FC1Option, FlangOnlyOption] in {
 
 def test_io : Flag<["-"], "test-io">, Group<Action_Group>,

clang/lib/Driver/ToolChains/Flang.cpp

 
 void Flang::AddPreprocessingOptions(const ArgList &Args,
                                     ArgStringList &CmdArgs) const {
   Args.AddAllArgs(CmdArgs, {options::OPT_D, options::OPT_U, options::OPT_I});
 }
 
 void Flang::AddOtherOptions(const ArgList &Args, ArgStringList &CmdArgs) const {
   Args.AddAllArgs(CmdArgs,
-                  {options::OPT_module_dir, options::OPT_fdebug_module_writer});
+                  {options::OPT_module_dir, options::OPT_fdebug_module_writer,
+                   options::OPT_werror});
 }
 
 void Flang::ConstructJob(Compilation &C, const JobAction &JA,
                          const InputInfo &Output, const InputInfoList &Inputs,
                          const ArgList &Args, const char *LinkingOutput) const {
   const auto &TC = getToolChain();
   // TODO: Once code-generation is available, this will need to be commented
   // out.

flang/include/flang/Frontend/CompilerInvocation.h

 //===- CompilerInvocation.h - Compiler Invocation Helper Data ---*- C -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_FLANG_FRONTEND_COMPILERINVOCATION_H
 #define LLVM_FLANG_FRONTEND_COMPILERINVOCATION_H
 
 #include "flang/Frontend/FrontendOptions.h"
 #include "flang/Frontend/PreprocessorOptions.h"
 #include "flang/Parser/parsing.h"
 #include "flang/Semantics/semantics.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "llvm/Option/ArgList.h"
 #include <memory>
 
class CompilerInvocation : public CompilerInvocationBase {
 
   /// Semantic options
   // TODO: Merge with or translate to frontendOpts_. We shouldn't need two sets
   // of options.
   std::string moduleDir_ = ".";
 
   bool debugModuleDir_ = false;
 
+  bool warnAsErr_ = false;
+
   // Fortran Dialect options
   Fortran::common::IntrinsicTypeDefaultKinds defaultKinds_;
 
 public:
   CompilerInvocation() = default;
 
   FrontendOptions &frontendOpts() { return frontendOpts_; }
   const FrontendOptions &frontendOpts() const { return frontendOpts_; }
public:
   }
 
   std::string &moduleDir() { return moduleDir_; }
   const std::string &moduleDir() const { return moduleDir_; }
 
   bool &debugModuleDir() { return debugModuleDir_; }
   const bool &debugModuleDir() const { return debugModuleDir_; }
 
+  bool &warnAsErr() { return warnAsErr_; }
+  const bool &warnAsErr() const { return warnAsErr_; }
+
   Fortran::common::IntrinsicTypeDefaultKinds &defaultKinds() {
     return defaultKinds_;
   }
   const Fortran::common::IntrinsicTypeDefaultKinds &defaultKinds() const {
     return defaultKinds_;
   }
 
   /// Create a compiler invocation from a list of input options.
   /// \returns true on success.
   /// \returns false if an error was encountered while parsing the arguments
   /// \param [out] res - The resulting invocation.
   static bool CreateFromArgs(CompilerInvocation &res,
       llvm::ArrayRef<const char *> commandLineArgs,
       clang::DiagnosticsEngine &diags);
 
   /// Useful setters
   void SetModuleDir(std::string &moduleDir) { moduleDir_ = moduleDir; }
 
   void SetDebugModuleDir(bool flag) { debugModuleDir_ = flag; }
 
+  void SetWarnAsErr(bool flag) { warnAsErr_ = flag; }
+
   /// Set the Fortran options to predifined defaults. These defaults are
   /// consistend with f18/f18.cpp.
   // TODO: We should map frontendOpts_ to parserOpts_ instead. For that, we
   // need to extend frontendOpts_ first. Next, we need to add the corresponding
   // compiler driver options in libclangDriver.
   void SetDefaultFortranOpts();
 
   /// Set the default predefinitions.

flang/lib/Frontend/CompilerInvocation.cpp

 //===- CompilerInvocation.cpp ---------------------------------------------===//
 //
 // 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/CompilerInvocation.h"
 #include "flang/Common/Fortran-features.h"
 #include "flang/Frontend/PreprocessorOptions.h"
 #include "flang/Semantics/semantics.h"
 #include "flang/Version.inc"
 #include "clang/Basic/AllDiagnostics.h"
 #include "clang/Basic/DiagnosticDriver.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Driver/DriverDiagnostic.h"
static void parseSemaArgs(CompilerInvocation &res, llvm::opt::ArgList &args,
   }
   if (moduleDirList.size() == 1)
     res.SetModuleDir(moduleDirList[0]);
 
   // -fdebug-module-writer option
   if (args.hasArg(clang::driver::options::OPT_fdebug_module_writer)) {
     res.SetDebugModuleDir(true);
   }
+
+  // -Werror option
+  if (args.hasArg(clang::driver::options::OPT_werror)) {
+    res.SetWarnAsErr(true);
+  }
 }
 
 /// Parses all Dialect related arguments and populates the variables
 /// options accordingly.
 static void parseDialectArgs(CompilerInvocation &res, llvm::opt::ArgList &args,
     clang::DiagnosticsEngine &diags) {
 
   // -fdefault* family
   if (args.hasArg(clang::driver::options::OPT_fdefault_real_8)) {
     res.defaultKinds().set_defaultRealKind(8);
     res.defaultKinds().set_doublePrecisionKind(16);
   }
   if (args.hasArg(clang::driver::options::OPT_fdefault_integer_8)) {
     res.defaultKinds().set_defaultIntegerKind(8);
     res.defaultKinds().set_subscriptIntegerKind(8);
     res.defaultKinds().set_sizeIntegerKind(8);
   }
   if (args.hasArg(clang::driver::options::OPT_fdefault_double_8)) {

 void CompilerInvocation::setSemanticsOpts(
     Fortran::parser::AllCookedSources &allCookedSources) {
   const auto &fortranOptions = fortranOpts();
 
   semanticsContext_ = std::make_unique<semantics::SemanticsContext>(
       defaultKinds(), fortranOptions.features, allCookedSources);
 
   semanticsContext_->set_moduleDirectory(moduleDir())
-      .set_searchDirectories(fortranOptions.searchDirectories);
+      .set_searchDirectories(fortranOptions.searchDirectories)
+      .set_warningsAreErrors(warnAsErr());
 }

flang/test/Semantics/dosemantics03.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -Mstandard -Werror
+! RUN: %S/test_errors.sh %s %t %flang_fc1 -Werror
 
 ! Issue 458 -- semantic checks for a normal DO loop.  The DO variable
 ! and the initial, final, and step expressions must be INTEGER if the
 ! options for standard conformance and turning warnings into errors
 ! are both in effect.  This test turns on the options for standards
 ! conformance and turning warnings into errors.  This produces error
 ! messages for the cases where REAL and DOUBLE PRECISION variables
 ! and expressions are used in the DO controls.