This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by awarzynski on Mar 25 2021, 10:24 AM.

Details

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

Diff Detail

Event Timeline

awarzynski created this revision.Mar 25 2021, 10:24 AM
awarzynski requested review of this revision.Mar 25 2021, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2021, 10:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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!

awarzynski added a project: Restricted Project.

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.

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

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 updated this revision to Diff 335270.Apr 5 2021, 9:00 AM

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 updated this revision to Diff 335275.Apr 5 2021, 9:21 AM

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

awarzynski added inline comments.Apr 5 2021, 9:22 AM
clang/lib/Driver/Driver.cpp
294–297

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

ashermancinelli accepted this revision.Apr 6 2021, 10:22 AM
This revision is now accepted and ready to land.Apr 6 2021, 10:22 AM

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 added a comment.EditedApr 7 2021, 8:53 AM

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 updated this revision to Diff 336075.Apr 8 2021, 6:07 AM

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

tstellar added a subscriber: tstellar.

Any chance that we get this into llvm/13?

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!

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

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

Let me add @rsmith as a reviewer.

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.

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 abandoned this revision.Sep 21 2023, 2:15 AM

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

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2023, 2:15 AM
Herald added a subscriber: MaskRay. · View Herald Transcript