[flang] Handle unsupported warning flags

[elmcdonough]

Summary

This PR makes flang emit a warning when the user passes an unsupported gfortran warning flag in as a CLI arg. This PR also checks each -W argument instead of just looking at the last one passed in.

[jdoerfert]

You need tests for both changes.

[elmcdonough]

Add tests for change as requested by reviewer

[jdoerfert]

Squash your commits into a single one before uploading it to Phabricator. We don't do multi-commit pull requests but one commit changes that contain the entire feature (basically we don't care how we arrived at the final commit).

The test should check for the diagnosis, thus that we emit a warning when we expect it, and none otherwise.

@awarzynski other thoughts?

[awarzynski]

In D143301#4109568, @jdoerfert wrote:

Squash your commits into a single one before uploading it to Phabricator. We don't do multi-commit pull requests but one commit changes that contain the entire feature (basically we don't care how we arrived at the final commit).

The test should check for the diagnosis, thus that we emit a warning when we expect it, and none otherwise.

@awarzynski other thoughts?

Thanks for sending this, @elmcdonough !

I appreciate that in your case you are only concerned with -Wextra, but what are we meant to do with other flags like this (either diagnostic or non-diagnostic option that's used in GFortran, but unavailable in LLVM Flang)? I think that we should agree to treat all unsupported GFortran options like this (for consistency) and take a bit more generic approach. I see at least a couple of options:

• Add another flag in Options.td to clearly document such options (something akin FlangOnlyOption), e.g. GFortranFlag. This way, adding a new "unsupported" option would boil down to decorating it with this new flag in Options.td,
• Create something similar to e.g. LangOptions.def to define such unsupported options.

Also, this is effectively changing the design of the driver (and the user-visible behavior). I would appreciate a note in:

• the release notes,
• driver documentation.

It's fine if you only add -Wextra for now, but I think that it would be good for the long term health of Flang if we had a clear mechanism for such flags that others can use in the future. WDYT?

[elmcdonough]

In D143301#4109632, @awarzynski wrote:

In D143301#4109568, @jdoerfert wrote:

Squash your commits into a single one before uploading it to Phabricator. We don't do multi-commit pull requests but one commit changes that contain the entire feature (basically we don't care how we arrived at the final commit).

The test should check for the diagnosis, thus that we emit a warning when we expect it, and none otherwise.

@awarzynski other thoughts?

Thanks for sending this, @elmcdonough !

I appreciate that in your case you are only concerned with -Wextra, but what are we meant to do with other flags like this (either diagnostic or non-diagnostic option that's used in GFortran, but unavailable in LLVM Flang)? I think that we should agree to treat all unsupported GFortran options like this (for consistency) and take a bit more generic approach. I see at least a couple of options:

• Add another flag in Options.td to clearly document such options (something akin FlangOnlyOption), e.g. GFortranFlag. This way, adding a new "unsupported" option would boil down to decorating it with this new flag in Options.td,
• Create something similar to e.g. LangOptions.def to define such unsupported options.

Also, this is effectively changing the design of the driver (and the user-visible behavior). I would appreciate a note in:

• the release notes,
• driver documentation.

It's fine if you only add -Wextra for now, but I think that it would be good for the long term health of Flang if we had a clear mechanism for such flags that others can use in the future. WDYT?

Thank you for the feedback, @awarzynski and @jdoerfert. This is something I think I can do, but I do have a few questions regarding specifics. I've gone through gfortran's flags and checked their support in flang:

Defined flag in clang driver as unsupported:

• -print-multi-os-directory
• -pass-exit-codes
• -dumpspecs
• -print-multiarch

Not present in Options.td:

• -print-sysroot
• -print-sysroot-headers-suffix

If I'm not mistaken, -pass-exit-codes is the only arguments that used for compilation. I think it would be a good idea to make -pass-exit-codes emit a warning when passed into flang's compiler driver via the GFortranFlag method @awarzynski mentioned, but the rest seem fine throwing errors error in my opinion. Someone wouldn't want a warning instead of an error if they passed -dumpspecs into gfortran because they're looking for specific information instead of compiling a program. The reason I felt -Wextra shouldn't throw an error (even though it isn't supported) is because the option itself would only emit extra warnings if it were implemented. Thoughts?

[awarzynski]

In D143301#4110880, @elmcdonough wrote:

Thank you for the feedback, @awarzynski and @jdoerfert. This is something I think I can do, but I do have a few questions regarding specifics. I've gone through gfortran's flags and checked their support in flang:

Defined flag in clang driver as unsupported:

• -print-multi-os-directory
• -pass-exit-codes
• -dumpspecs
• -print-multiarch

Not present in Options.td:

• -print-sysroot
• -print-sysroot-headers-suffix

If I'm not mistaken, -pass-exit-codes is the only arguments that used for compilation. I think it would be a good idea to make -pass-exit-codes emit a warning when passed into flang's compiler driver via the GFortranFlag method @awarzynski mentioned, but the rest seem fine throwing errors error in my opinion. Someone wouldn't want a warning instead of an error if they passed -dumpspecs into gfortran because they're looking for specific information instead of compiling a program. The reason I felt -Wextra shouldn't throw an error (even though it isn't supported) is because the option itself would only emit extra warnings if it were implemented. Thoughts?

Thanks for this overview!

I feel that we should prioritize consistency and avoid treating one unsupported flag differently to other unsupported flags (that are available in GFortan). But it would also be good to hear what others think and for that we may need to bring this up on Discourse :) If we do decide that some options should lead to warnings and others to errors, then there should be a very clear mechanism for this. I would really like to avoid adding special cases to "CompilerInvocation.cpp" (which is already quite complex).

[jdoerfert]

Let's do (= @elmcdonough, please do) two things:

1. Fix the tests and get this in (assuming it's reasonable)
2. Start a discourse thread with the different options we discussed so far. My vote is to do what clang does. If that is not properly defined we can do categories of ignored options for other compilers and emit warnings under a common flag (-Wgfortran-only, -Wifx-only, ...).

[elmcdonough]

In D143301#4110914, @jdoerfert wrote:

Let's do (= @elmcdonough, please do) two things:

1. Fix the tests and get this in (assuming it's reasonable)
2. Start a discourse thread with the different options we discussed so far. My vote is to do what clang does. If that is not properly defined we can do categories of ignored options for other compilers and emit warnings under a common flag (-Wgfortran-only, -Wifx-only, ...).

Sounds good. I've started a discussion at https://discourse.llvm.org/t/flang-driver-behavior-of-unsupported-gfortran-flags-when-fed-into-flang/68289

[elmcdonough]

This update adds other unsupported gfortran options to Options.td.

[jdoerfert]

Split this into two patches/reviews. I think the -W stuff can go in, it has tests and is reasonable. The other stuff needs tests too.

flang/lib/Frontend/CompilerInvocation.cpp

602–619

Probably

[awarzynski]

In D143301#4126682, @jdoerfert wrote:

Split this into two patches/reviews.

+1

I think the -W stuff can go in, it has tests and is reasonable.

I'd like for us to rely on a flag from Options.td for this instead. Something similar to clang_ignored_f_Group. I would probably call it flang_ignored_w_Group :)

[jdoerfert]

In D143301#4126712, @awarzynski wrote:

I think the -W stuff can go in, it has tests and is reasonable.

I'd like for us to rely on a flag from Options.td for this instead. Something similar to clang_ignored_f_Group. I would probably call it flang_ignored_w_Group :)

For the -W stuff? You want to remove the explicit warning then (which is generally fine too)?

[elmcdonough]

Thanks for the feedback everyone. This is my current understanding on what I should do: I am to rename gfortran_unsupported_Group to flang_ignored_w_Group and move the non W group gfortran options into another patch. I have these changes locally and am currently building + testing. Please let me know if I'm misinterpreting these instructions.

[awarzynski]

In D143301#4126855, @jdoerfert wrote:

In D143301#4126712, @awarzynski wrote:

I think the -W stuff can go in, it has tests and is reasonable.

I'd like for us to rely on a flag from Options.td for this instead. Something similar to clang_ignored_f_Group. I would probably call it flang_ignored_w_Group :)

For the -W stuff? You want to remove the explicit warning then (which is generally fine too)?

I had something like this in mind: https://github.com/llvm/llvm-project/blob/7301a7ce196e217c077b2b68f58366be48664223/clang/lib/Driver/ToolChains/Clang.cpp#L7448. Also, the whole logic could be moved to the compiler driver (i.e. Flang.cpp) 🤔 . Emitting a warning makes sense, but do we care about the frontend driver (i.e. "CompilerInvocation.cpp")? (which is intended for developers familiar with the implementation?)

[awarzynski]

In D143301#4126884, @elmcdonough wrote:

Thanks for the feedback everyone. This is my current understanding on what I should do: I am to rename gfortran_unsupported_Group to flang_ignored_w_Group and move the non W group gfortran options into another patch. I have these changes locally and am currently building + testing. Please let me know if I'm misinterpreting these instructions.

Makes sense and sorry for confusion. Just to clarify, "w" in flang_ignored_w_Group stands for "warning" :) I think that having two separate patches will simplify the discussion too ;-) And, most importantly, thanks for doing this!

[awarzynski]

Thanks for the update! One thing that's not clear to me - how come "-Wextra" is not treated as an error and "-Wblah" is? That's not clear from the code.

I'm also realising that I incorrectly assumed that -Wextra was defined in Options.td. Instead, it's defined in DiagnosticGroup.td. I've not worked with that file and it's not clear to me how it works. IIUC, that file encapsulates all the user-facing logic controlling diagnostics in Clang. I am mentioning this as in your approach you are effectively re-defining "-Wextra" in Options.td. That's unfortunate (it's duplication), but IMO the approach taken here is fine - all tests pass and tweaking Clang's DiagnosticGroup.td is beyond the scope. I just want understand what "makes this work" :)

clang/include/clang/Driver/Options.td

4881 ↗

(On Diff #497405)

[nit] These are diag options.

[elmcdonough]

Change multiclass name for the sake of clarity.

[awarzynski]

@elmcdonough , let me rephrase this (should've been clearer before, sorry):

One thing that's not clear to me - how come "-Wextra" is not treated as an error and "-Wblah" is?

Where's the logic that makes sure that -Wextra (and other flags that you redefine here) is reported as unused? That's not clear from this definition:

multiclass FlangIgnoredDiagOpt<string name> {
  def unsupported_warning_w#NAME : Flag<["-", "--"], "W"#name>, Group<flang_ignored_w_Group>;
}

In particular, I don't see anything that would check whether a particular option is in this group: flang_ignored_w_Group.

[elmcdonough]

In D143301#4135301, @awarzynski wrote:

@elmcdonough , let me rephrase this (should've been clearer before, sorry):

One thing that's not clear to me - how come "-Wextra" is not treated as an error and "-Wblah" is?

Where's the logic that makes sure that -Wextra (and other flags that you redefine here) is reported as unused? That's not clear from this definition:

multiclass FlangIgnoredDiagOpt<string name> {
  def unsupported_warning_w#NAME : Flag<["-", "--"], "W"#name>, Group<flang_ignored_w_Group>;
}

In particular, I don't see anything that would check whether a particular option is in this group: flang_ignored_w_Group.

I think I understand now. I initially didn't add any manual handling to the PR because I found that defining the options but leaving them as Ignored caused an "argument unused during compilation" whenever they were used in my local tests. This revision should give an explicit warning about the warning options not being supported.

[elmcdonough]

Clang format + documentation update

[awarzynski]

clang/include/clang/Driver/Options.td

5045 ↗

(On Diff #498944)

The name of the sub-project is "Flang", the sub-dir is "flang" and the driver name is "flang-new". What is "flang" referring to in this comment?

Also, "-W<arg> options" would be IMHO a bit more descriptive.

5047 ↗

(On Diff #498944)

How about a test for all the options that follow -Wextra?

[elmcdonough]

Comment edit + exhuastive testing

[awarzynski]

LGTM, thanks for seeing this through! :)

[vzakhari]

The new tests fail to link:

flang-new: warning: The warning option '-Wextra' is not supported
/usr/bin/ld: cannot find -lFortran_main
/usr/bin/ld: cannot find -lFortranRuntime
/usr/bin/ld: cannot find -lFortranDecimal

Is the linking really necessary?

[PeteSteinfeld]

In D143301#4143108, @vzakhari wrote:

The new tests fail to link:

That is, these new tests cause check-flang to fail. @elmcdonough, can you please fix things so that check-flang no longer fails?

[elmcdonough]

In D143301#4143241, @PeteSteinfeld wrote:

In D143301#4143108, @vzakhari wrote:

The new tests fail to link:

That is, these new tests cause check-flang to fail. @elmcdonough, can you please fix things so that check-flang no longer fails?

Thank you for pointing this out, @vzakhari.

@PeteSteinfeld, I think vzakhari's most recent commit fixes this.

[PeteSteinfeld]

@PeteSteinfeld, I think vzakhari's most recent commit fixes this.

Thanks, @ elmcdonough, and @vzakhari!

flang/lib/Frontend/CompilerInvocation.cpp

 static bool parseDiagArgs(CompilerInvocation &res, llvm::opt::ArgList &args,
                           clang::DiagnosticsEngine &diags) {
   unsigned numErrorsBefore = diags.getNumErrors();
 
   // -Werror option
   // TODO: Currently throws a Diagnostic for anything other than -W<error>,
   // this has to change when other -W<opt>'s are supported.
   if (args.hasArg(clang::driver::options::OPT_W_Joined)) {
-    if (args.getLastArgValue(clang::driver::options::OPT_W_Joined)
-            .equals("error")) {
+    const auto wArgs =
+        args.getAllArgValues(clang::driver::options::OPT_W_Joined);
+    for (const auto &wArg : wArgs) {
+      if (wArg == "error") {
         res.setWarnAsErr(true);
+      } else if (wArg == "extra") {
+        const unsigned id = diags.getCustomDiagID(
+            clang::DiagnosticsEngine::Warning,
+            "Argument `-Wextra` was ignored during compilation. Only `-Werror` "
+            "is supported currently.");
+        diags.Report(id);
       } else {
-      const unsigned diagID =
+        const unsigned id =
             diags.getCustomDiagID(clang::DiagnosticsEngine::Error,
                                   "Only `-Werror` is supported currently.");
-      diags.Report(diagID);
+        diags.Report(id);
+        break;
+      }
     }
   }
 
   // Default to off for `flang-new -fc1`.
   res.getFrontendOpts().showColors =
       parseShowColorsArgs(args, /*defaultDiagColor=*/false);
 
   return diags.getNumErrors() == numErrorsBefore;