[driver] Make `clang` warn rather then error on `flang` options

[awarzynski]

Summary

This patch adds extra logic in Driver::ParseArgStrings so that clang
(Clang's compiler driver) generates a warning when a Flang-only option
is used. Previously it would throw an error:

• error: unknown argument: <option>

and exit immediately. That was flagged as problematic in [1].

The new behaviour is consistent with GCC:

gcc -c  -ffree-form  file.c
cc1: warning: command line option ‘-ffree-form’ is valid for Fortran but not for C

It allows users to use clang as e.g. a linker driver, without worrying
about passing Fortran specific flags to clang. These flags are often
embedded in build systems and are hard to extract just for linking.
An issue like this raised in [1]. This patch is an attempt to address
this.

The current approach is a bit verbose. Ideally we should extend the
OptTable API to make accommodating for situations like this easier. In
particular, I'm not sure whether we can add similar logic for Flang.
In this patch we are relying on the FlangOnlyOption flag to
identify problematic options. There's no equivalent flag for Clang-only
options. I propose that we address this later, when this becomes a
problem for Flang.

[1] https://reviews.llvm.org/D95460

[awarzynski]

I've not had a chance to add tests yet. I will add them shortly.

@protze.joachim , hopefully this addresses the issues that you raised in https://reviews.llvm.org/D95460. Please, could you verify before I update this? I want to make sure that this is the right approach. Thank you!

[protze.joachim]

Thank you for working on this! It works for me.
As you mentioned in D95460, this makes the behavior more similar to gcc.

I tested with -Werror:

$ flang -fopenmp test-f77.f -ffree-form -c
$ clang -fopenmp test-f77.o -ffree-form -lgfortran -Werror && echo $?
clang-13: warning: command line option ‘-ffree-form’ is only valid in Flang mode (i.e. for Fortran input)
clang-13: error: argument unused during compilation: '-ffree-form' [-Werror,-Wunused-command-line-argument]

Since -Werror only raises the second warning, -Wno-error=unused-command-line-argument allows to successfully compile with -Werror.

$ clang -fopenmp test-f77.o -ffree-form -lgfortran -Werror -Wno-error=unused-command-line-argument && echo $?
clang-13: warning: command line option ‘-ffree-form’ is only valid in Flang mode (i.e. for Fortran input)
clang-13: warning: argument unused during compilation: '-ffree-form' [-Wunused-command-line-argument]
0

I also tested with -ffixed-form and -ffixed-line-length-132. The latter doesn't work, while -ffixed-line-length=132 works.

[awarzynski]

Hi @protze.joachim , thank you for testing this so thoroughly!

In D99353#2656215, @protze.joachim wrote:

I tested with -Werror:

$ flang -fopenmp test-f77.f -ffree-form -c
$ clang -fopenmp test-f77.o -ffree-form -lgfortran -Werror && echo $?
clang-13: warning: command line option ‘-ffree-form’ is only valid in Flang mode (i.e. for Fortran input)
clang-13: error: argument unused during compilation: '-ffree-form' [-Werror,-Wunused-command-line-argument]

Since -Werror only raises the second warning, -Wno-error=unused-command-line-argument allows to successfully compile with -Werror.

I think that -Werror should also elevate the first warning to an error. This can be fixed by creating a definition for the diagnostic in clang/include/clang/Basic/DiagnosticDriverKinds.td.

I also tested with -ffixed-form and -ffixed-line-length-132. The latter doesn't work, while -ffixed-line-length=132 works.

Tl;Dr I haven't figured yet how to make this work for options that are aliases.
Longer version:
This is caused by the fact that ffixed-line-length-132 is an alias for ffixed-line-length=132. When parsing options, the OptTable API will skip unknown options (e.g. options marked with FlangOnlyOption when in Clang mode) and represent them internally as OPT_UNKNOWN. This happens in Optable.cpp. Options that are "unknown" contain no information, so it's impossible to check their aliases (or whether they alias some other option). This is problematic as options/args are converted to their unaliased forms when created (see here).

Btw, how important are these aliases for you?

[awarzynski]

Refine the behaviour when using diag options and add tests

• -Werror will now elevate the warning generated here to an error. This is consistent with gcc. To this end I had to create a new diagnostic in DiagnosticDriverKinds.td.
• Added tests for the logic implemented here.

[awarzynski]

Add more reviewers and remove unrelated changes (apologies for the noise!)

[awarzynski]

clang/lib/Driver/Driver.cpp

294–297

This is how we could deal with aliased options, but currently A->getAlias() always returns nullptr.

[protze.joachim]

In D99353#2669046, @awarzynski wrote:

Btw, how important are these aliases for you?

I can work with -ffixed-line-length=132 where needed. It's just not obvious from flang --help that this is an alias for -ffixed-line-length-132 (or the other way around). I only learned that by looking at LLVM source.

[awarzynski]

In D99353#2672503, @protze.joachim wrote:

In D99353#2669046, @awarzynski wrote:

Btw, how important are these aliases for you?

It's just not obvious from flang --help that this is an alias for -ffixed-line-length-132 (or the other way around). I only learned that by looking at LLVM source.

It's the other way round :) -ffixed-line-length-132 is an alias for -ffixed-line-length=132. If you run flang-new --help, then you will only see non-aliases. Hopefully that will help navigating this.

I don't have a solution for aliases, I'd need more time for this. Long term we should definitely aim for something more general, but for now I'd like to focus on making sure that:

• you are no longer blocked, and
• we are not introducing anything that could block/disable other important use cases.

Btw, I hope that you meant flang-new --help (the new driver, based on libClangDriver) rather than flang --help (the "throwaway" driver that we want to replace in the near future).

[awarzynski]

Refine comments, remove code for aliases (which didn't work anyway)

[awarzynski]

I've just send an RFC for this change to cfe-dev: https://lists.llvm.org/pipermail/cfe-dev/2021-April/067981.html.

[protze.joachim]

Any chance that we get this into llvm/13?

[awarzynski]

In D99353#2918629, @protze.joachim wrote:

Any chance that we get this into llvm/13?

Given that Release-13 branch has already been created (https://lists.llvm.org/pipermail/llvm-dev/2021-July/151956.html), this is very unlikely. IMHO, this should be approved by somebody from cfe-dev before we merge it. But from the conversation that we started on the mailing list, I feel that there's little appetite for this change.

@protze.joachim , if this is still required for your workflow, could you either reply to the thread on cfe-dev or create a bugzilla ticket? Otherwise people might not realise that this is an issue for some Clang users. Thank you!

[tstellar]

This is something we could merge, just need a code owner ack.

[awarzynski]

In D99353#2919774, @tstellar wrote:

This is something we could merge, just need a code owner ack.

Let me add @rsmith as a reviewer.

[protze.joachim]

The use case, where I see the current behavior as an issue is linking multiple object files compiled from different languages.
Many build systems will pass CFLAGS as well as FFLAGS to the linker command in such case. To automatically link the necessary language-specific runtime libraries, I see using the compiler for linking as common practice:

$(FC) -c fortran-code.F90 $(FFLAGS)
$(CXX) -c cpp-code.cpp $(CFLAGS)
$(CC) -c c-code.c $(CFLAGS)
$(CXX) fortran-code.o cpp-code.o c-code.o $(FFLAGS) $(CFLAGS) -l$(FORTRAN_RUNTIME)

To support such workflow, the compiler should not error out, at least when only used for linking.

This works with CC=gcc, CXX=g++, FC=gfortran, and FORTRAN_RUNTIME=gfortran.
This used to work with CC=clang, CXX=clang++, FC=gfortran, and FORTRAN_RUNTIME=gfortran.

[protze.joachim]

Since I see this as a regression over clang-12, I posted https://bugs.llvm.org/show_bug.cgi?id=51339 as a release blocking issue.

[awarzynski]

With the imminent switch to GitHub and no new comments in over 2 years, I feel that it's time to abandon this change. Feel free to re-use this in the future :)

clang/lib/Driver/Driver.cpp

InputArgList Driver::ParseArgStrings(ArrayRef<const char *> ArgStrings,
   // TODO: Make sure that Clang-only options don't pollute Flang output
   if (!IsFlangMode())
     ExcludedFlagsBitmask |= options::FlangOnlyOption;
 
   unsigned MissingArgIndex, MissingArgCount;
   InputArgList Args =
       getOpts().ParseArgs(ArgStrings, MissingArgIndex, MissingArgCount,
                           IncludedFlagsBitmask, ExcludedFlagsBitmask);
-
   // Check for missing argument error.
   if (MissingArgCount) {
     Diag(diag::err_drv_missing_argument)
         << Args.getArgString(MissingArgIndex) << MissingArgCount;
     ContainsError |=
         Diags.getDiagnosticLevel(diag::err_drv_missing_argument,
                                  SourceLocation()) > DiagnosticsEngine::Warning;
   }
if (A->getOption().matches(options::OPT_mcpu_EQ) && A->containsValue("")) {
       Diag(diag::warn_drv_empty_joined_argument) << A->getAsString(Args);
       ContainsError |= Diags.getDiagnosticLevel(
                            diag::warn_drv_empty_joined_argument,
                            SourceLocation()) > DiagnosticsEngine::Warning;
     }
   }
 
   for (const Arg *A : Args.filtered(options::OPT_UNKNOWN)) {
-    unsigned DiagID;
+    // The default diagnostic when the option is unknow
+    unsigned DiagID = diag::err_drv_unknown_argument;
+
+    // Try to find a good hint for the user
     auto ArgString = A->getAsString(Args);
     std::string Nearest;
-    if (getOpts().findNearest(
-          ArgString, Nearest, IncludedFlagsBitmask, ExcludedFlagsBitmask) > 1) {
-      DiagID = IsCLMode() ? diag::warn_drv_unknown_argument_clang_cl
-                          : diag::err_drv_unknown_argument;
-      Diags.Report(DiagID) << ArgString;
-    } else {
+    unsigned Distance = getOpts().findNearest(
+        ArgString, Nearest, IncludedFlagsBitmask, ExcludedFlagsBitmask);
+    assert(Distance != 0 && "This option should not be 'unknown'");
+
+    if (Distance == 1) {
+      // Found a good suggestion - propose that in the diagnostic
       DiagID = IsCLMode()
                    ? diag::warn_drv_unknown_argument_clang_cl_with_suggestion
                    : diag::err_drv_unknown_argument_with_suggestion;
       Diags.Report(DiagID) << ArgString << Nearest;
+    } else {
+      // No good suggestion was found - the diagnostic depends on the mode in
+      // which the driver operates
+      if (IsCLMode())
+        // In CL mode just warn the user (MSVC consumes everything anyway)
+        DiagID = diag::warn_drv_unknown_argument_clang_cl;
+      else if (!IsFlangMode()) {
+        // In non-Flang mode, check whether this is a Flang flag and
+        // communicate accordingly.
+        ExcludedFlagsBitmask &= ~options::FlangOnlyOption;
+        if (getOpts().findNearest(ArgString, Nearest, IncludedFlagsBitmask,
+                                  ExcludedFlagsBitmask) == 0)
+          DiagID =
+              Diags.getCustomDiagID(clang::DiagnosticsEngine::Warning,
+                                    "command line option ‘%0’ is only valid "
+                                    "in Flang mode (i.e. for Fortran input)");
+      } // TODO: Check whether this is a C-mode flag and report accordingly.
+
+      Diags.Report(DiagID) << ArgString;
     }
+
     ContainsError |= Diags.getDiagnosticLevel(DiagID, SourceLocation()) >
                      DiagnosticsEngine::Warning;
   }
 
   return Args;
 }
 
 // Determine which compilation mode we are in. We look for options which