This is an archive of the discontinued LLVM Phabricator instance.

[flang] Handle unsupported warning flags
ClosedPublic

Authored by elmcdonough on Feb 3 2023, 3:28 PM.

Details

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.

Diff Detail

Event Timeline

elmcdonough created this revision.Feb 3 2023, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 3:28 PM
elmcdonough requested review of this revision.Feb 3 2023, 3:28 PM

You need tests for both changes.

Add tests for change as requested by reviewer

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?

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:

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?

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:

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 added a comment.EditedFeb 7 2023, 11:45 AM

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).

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, ...).

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 retitled this revision from Emit warning for `-Wextra` instead of error to Emit warning for unsupported gfortran flags.

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

Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2023, 5:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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
607–624

Probably

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 :)

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)?

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.

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?)

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!

elmcdonough retitled this revision from Emit warning for unsupported gfortran flags to [flang] Handle unsupported warning flags.
elmcdonough edited the summary of this revision. (Show Details)

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.

Change multiclass name for the sake of clarity.

@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 marked an inline comment as done.

@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.

Clang format + documentation update

awarzynski added inline comments.Feb 21 2023, 9:17 AM
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 marked 3 inline comments as done.

Comment edit + exhuastive testing

awarzynski accepted this revision.Feb 21 2023, 1:50 PM

LGTM, thanks for seeing this through! :)

This revision is now accepted and ready to land.Feb 21 2023, 1:50 PM
This revision was landed with ongoing or failed builds.Feb 21 2023, 2:14 PM
This revision was automatically updated to reflect the committed changes.

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?

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?

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, I think vzakhari's most recent commit fixes this.

Thanks, @ elmcdonough, and @vzakhari!