This is an archive of the discontinued LLVM Phabricator instance.

[flang] Make the frontend driver error out when requesting multiple actions
ClosedPublic

Authored by awarzynski on Oct 14 2021, 1:13 AM.

Details

Summary

With this change, the following invocations are treated as errors
(multiple actions are specified):

$ flang-new -fc1 -E -fsyntax-only file.95
$ flang-new -fc1 -fsyntax-only -fsyntax-only file.95

Diff Detail

Event Timeline

awarzynski created this revision.Oct 14 2021, 1:13 AM
Herald added a project: Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Oct 14 2021, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2021, 1:13 AM
awarzynski added a comment.EditedOct 14 2021, 1:17 AM
NOTE: : Please see the discussion on flang-dev for more context.

Is this the right approach? Shall we ping cfe-dev and see whether they would be interested in something similar?

Make sure that all tests pass

  • Update RUN lines that specified multiple frontend actions (with this patch, only one action can be used)
  • Refine hasMultipleArgs. Previously it returned true for any number of action options !=1. This should be >=1 instead.
PeteSteinfeld accepted this revision.Oct 14 2021, 1:26 PM

This generally looks good. Another possibility would be to allow the specification of the default option.

This revision is now accepted and ready to land.Oct 14 2021, 1:26 PM
awarzynski added a comment.EditedNov 4 2021, 2:03 AM

I've just added reviewers that recently touched llvm/include/llvm/Option/ArgList.h. For a bit of context, this patch addresses some concerns raised here:

I'm not suggesting that a similar change is made in clang -cc1, but I definitely wouldn't mind extending this patch so that it covers both flang-new -cc1 and clang -cc1.

Thank you for taking a look!

I've just added reviewers that recently touched llvm/include/llvm/Option/ArgList.h. For a bit of context, this patch addresses some concerns raised here:

Both of those links point to the cfe-dev mailing list.

I'm not suggesting that a similar change is made in clang -cc1, but I definitely wouldn't mind extending this patch so that it covers both flang-new -cc1 and clang -cc1.

Personally, I don't think this is something we'd want for clang -cc1. The end users only interact with the driver interface (which will never generate multiple actions) and never see/modify the -cc1 invocation.
On the other hand, when hacking on Clang, I often grab a -cc1 invocation and append -Eonly to only run the preprocessor. If that resulted in an error and forced me to go through the whole command-line and remove -c, I'd be very frustrated.

If the Flang community is okay with this change, I don't have any objections.

Both of those links point to the cfe-dev mailing list.

Sorry for that, updated in the original comment!

Personally, I don't think this is something we'd want for clang -cc1.

Sure thing. And it's helpful to know that people are happy with the "status quo" (for good reasons, as you note below).

On the other hand, when hacking on Clang, I often grab a -cc1 invocation and append -Eonly to only run the preprocessor. If that resulted in an error and forced me to go through the whole command-line and remove -c, I'd be very frustrated.

That's a very interesting perspective. We don't hit this use-case just yet, because our feature list is still very limited. But we *are* heading in the similar direction (in terms of our workflows).

Rebase on top of main

This change will mostly affect Flang compiler developers who use flang-new -fc1 for their work or end-users who want to play with the internals. I know that @PeteSteinfeld is in favor and we have also discussed this on the mailing list and in Flang community calls. AFAIK, people are OK with this change. It does mean that flang-new -fc1 and clang -cc1 will behave a bit differently here, but it's nothing major. If there are no further comments, I will merge this tomorrow.

This revision was landed with ongoing or failed builds.Dec 17 2021, 2:10 AM
This revision was automatically updated to reflect the committed changes.