[flang][Driver] Refine _when_ driver diagnostics are formatted

[peixin]

Summary

This patch refines when driver diagnostics are formatted so that
flang-new and flang-new -fc1 behave consistently with clang and
clang -cc1, respectively. This change only applies to driver diagnostics.
Scanning, parsing and semantic diagnostics are separate and not covered here.

NEW BEHAVIOUR
To illustrate the new behaviour, consider the following input file:

! file.f90
program m
  integer :: i = k
end

In the following invocations, "error: Semantic errors in file.f90" _will be_
formatted:

$ flang-new file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k
$ flang-new -fc1 -fcolor-diagnostics file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k

However, in the following invocations, "error: Semantic errors in file.f90"
_will not be_ formatted:

$ flang-new -fno-color-diagnostics file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k
$ flang-new -fc1 file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k

Before this change, none of the above would be formatted. Note also that the
default behaviour in flang-new is different to flang-new -fc1 (this is
consistent with Clang).

NOTES ON IMPLEMENTATION
Note that the diagnostic options are parsed in createAndPopulateDiagOpts in
driver.cpp. That's where the driver's DiagnosticEngine options are set. Like
most command-line compiler driver options, these flags are "claimed" in
Flang.cpp (i.e. when creating a frontend driver invocation) by calling
getLastArg rather than in driver.cpp.

In Clang's Options.td, defm color_diagnostics is replaced with two separate
definitions: def fcolor_diagnostics and def fno_color_diagnostics`. That's
because originally color_diagnostics derived from OptInCC1FFlag, which is a
multiclass for opt-in options in CC1. In order to preserve the current
behaviour in clang -cc1 (i.e. to keep -fno-color-diagnostics unavailable in
clang -cc1) and to implement similar behaviour in flang-new -fc1, we can't
re-use OptInCC1FFlag.

Formatting is only available in consoles that support it and will normally mean that
the message is printed in bold + color.

Co-authored-by: Andrzej Warzynski <[e-mail]>

[peixin]

Please check the following test cases after applying this patch. This highlight the color of the keyword "error" before "Could not parse case.f90" and "Semantic errors in case.f90".

$ cat case.f90 
program 
end
$ flang-new -fc1 case.f90 
error: Could not parse case.f90
./case.f90:1:11: error: expected '('
  program !m
            ^
./case.f90:1:1: in the context: statement function definition
  program !m
  ^
./case.f90:1:1: in the context: declaration construct
  program !m
  ^
./case.f90:1:1: in the context: specification part
  program !m
  ^
./case.f90:1:1: in the context: main program
  program !m
  ^
$ cat case.f90 
program m
  integer :: i = k
end
$ flang-new -fc1 case.f90 
error: Semantic errors in case.f90
./case.f90:2:18: error: Must be a constant value
    integer :: i = k
                   ^

[peixin]

This comment was removed by peixin.

[peixin]

ping. Do we have plans to support the colorful keyword for the diag messages for LLVM Flang?

[awarzynski]

Hi @peixin, thanks for sending this!

I think that formatting diagnostics is very useful, but we should avoid having the formatting "ON" at all times. We should also allow users to turn the formatting "OFF". More comments below and inline.

Enable the color set by default for diag messages.

1. Is the new behavior consistent with Clang? In general, we should follow the principle of least surprise and make flang-new as consistent with clang as possible (within reason). And if we decide to diverge, then we should document the rationale.
2. Does it affect the driver (flang-new & flang-new -fc1) and/or the frontend diagnostics? Note that unlike in Clang, driver and frontend diagnostics in Flang are separate. It would be good to document more specifically what is being updated here (from what I can see, just the driver).

[awarzynski]

flang/lib/Frontend/CompilerInstance.cpp

189 ↗

(On Diff #431226)

This should be controllable by -f{no}-color-diagnostics.

flang/lib/Frontend/TextDiagnosticPrinter.cpp

51–54

I'm not a fan of including semantic details in variable names. I've also not encountered it before in LLVM, have you?

diagsEng (or diagsEngine) would remain valid even if people decide to change & to *. Why not use that instead?

flang/test/Driver/driver-help-hidden.f90

75 ↗

(On Diff #431226)

In tests, we only need the error message - the actual formatting is irrelevant. In fact, it complicates testing because it forces you to add {{.*}}.

My point being - the diagnostics in tests should be displayed without any formatting. If you want the formatting to be enabled by default, then you will have to use -fno-color-diagnostics in tests (alternatively, update the %flang and %flang_fc1 LIT variables - that would probably be cleaner). I can't think of any other solution at the moment 🤔 . Clang's equivalent of -verify would be much better, but sadly that's not available.

[peixin]

Thanks @awarzynski for these suggestions.

In D126164#3547331, @awarzynski wrote:

Hi @peixin, thanks for sending this!

I think that formatting diagnostics is very useful, but we should avoid having the formatting "ON" at all times. We should also allow users to turn the formatting "OFF". More comments below and inline.

Enable the color set by default for diag messages.

1. Is the new behavior consistent with Clang? In general, we should follow the principle of least surprise and make flang-new as consistent with clang as possible (within reason). And if we decide to diverge, then we should document the rationale.
2. Does it affect the driver (flang-new & flang-new -fc1) and/or the frontend diagnostics? Note that unlike in Clang, driver and frontend diagnostics in Flang are separate. It would be good to document more specifically what is being updated here (from what I can see, just the driver).

I checked with clang, and it has the option you mentioned. I think clang enable it by default. I can see colorful diag messages on my terminal by default.

$ clang --help  | grep color
  -fcolor-diagnostics     Enable colors in diagnostics
  -fno-color-diagnostics  Disable colors in diagnostics

flang/lib/Frontend/CompilerInstance.cpp

189 ↗

(On Diff #431226)

Agree. Clang does it, too.

flang/lib/Frontend/TextDiagnosticPrinter.cpp

51–54

Good point. Will fix this.

flang/test/Driver/driver-help-hidden.f90

75 ↗

(On Diff #431226)

Agree with you. I didn't figure out how clang use -verify with expected-error{{message}} and expected-error{{warning}}. But I think it's OK to use the current test tools for flang. With the option, it is easy to silence the colorful diagnostics.

[awarzynski]

In D126164#3547399, @peixin wrote:

I checked with clang, and it has the option you mentioned. I think clang enable it by default. I can see colorful diag messages on my terminal by default.

$ clang --help  | grep color
  -fcolor-diagnostics     Enable colors in diagnostics
  -fno-color-diagnostics  Disable colors in diagnostics

Great! I think that it would be helpful to mention in the summary/commit message that:

• with this change, driver diagnostics will be formatted by default,
• this can be controlled with -f{no-}color-diagnosics,
• the new behavior is consistent with clang.

Enable the color set by default for diag messages.

Note that it's not only about the colors - the formatting includes making the font "bold".

flang/test/Driver/driver-help-hidden.f90

75 ↗

(On Diff #431226)

-verify is a Clang option that configures Clang's DiagnosticsEngine. Although we do use it for driver diagnostics, language diagnostics in Flang (e.g. scanning, parsing and sema diagnostics) are completely separate. And -verify is most useful/powerful when it comes to language diagnostics.

But I think it's OK to use the current test tools for flang. With the option, it is easy to silence the colorful diagnostics.

+1

[peixin]

Address the comments from @awarzynski .

[awarzynski]

Thanks, this is much shorter now :) So, with this revision I get no formatting at all. Do you? I suspect that DiagnosticsEngine is misconfigured. Note that in Clang, diagnostics are formatted by default in clang, but the formatting is disabled in clang -cc1. We need to make sure that that's the case here as well.

In particular, see how DiagnosticsOptions is created for clang: here. Compare this with the set-up for clang -cc1 here. I believe that the default value of DefaultDiagColor is key: here.

I might have missed something, but please make sure that:

• flang-new and clang behave consistently
• flang-new -fc1 and clang -cc1 also behave consistently

clang/include/clang/Driver/Options.td

1358–1359

These are GCC spellings, which are aliases for the Clang spellings (-f{no-}color-diagnostics) and which are already enabled for flang-new. Is this change required at all?

clang/lib/Driver/ToolChains/Flang.cpp

58–59

Would you mind moving this to a dedicated method? For example, something akin Clang's RenderDiagnosticsOptions. I know that it's only for 2 options, but there could be many more in the future and I think that it's nice to categorise them like this.

flang/lib/Frontend/CompilerInvocation.cpp

500–501

This is inconsistent with Clang: https://github.com/llvm/llvm-project/blob/5fee1799f4d8da59c251e2d04172fc2f387cbe54/clang/lib/Frontend/CompilerInvocation.cpp#L4453-L4454.

It would be good to update this comment too.

[peixin]

Thanks @awarzynski for the explanations. I misunderstood flang-new -fc1. Will fix this patch.

So, with this revision I get no formatting at all. Do you?

What do you mean by this? With thie revision, I got color bold for both flang-new and flang-new -fc1.

$ cat case.f90 
program m
  integer :: i = k
end
$ flang-new -fc1 case.f90 
error: Semantic errors in case.f90
./case.f90:2:18: error: Must be a constant value
    integer :: i = k
                   ^

But please notice that this revision is to set the color and bold for the "error" before "Semantic errors in case.f90", not the "error" before "Must be a constant value", which is planned in D126166.

clang/lib/Driver/ToolChains/Flang.cpp

58–59

Agree. I also saw a number of diagnostic options in Options.td. Will fix this.

[awarzynski]

In D126164#3560178, @peixin wrote:

So, with this revision I get no formatting at all. Do you?

What do you mean by this? With thie revision, I got color bold for both flang-new and flang-new -fc1.

Ignore that comment - one of these days! ;)

[peixin]

Address the comments from @awarzynski .

Please notice -fno-color-diagnostics is not supported in clang -cc1. This option is supported in flang-new -fc1 even though no-color is enabled by default for flang-new -fc1 since I don't see any reason to disallow it.

clang/include/clang/Driver/Options.td

1358–1359

When I disable one of them, I cannot get the color and bold messages.

flang/lib/Frontend/CompilerInvocation.cpp

500–501

Fixed.

[awarzynski]

$ flang-new a.f90 ! enable color and bold
$ flang-new -fc1 a.f90 ! disable color and bold
$ flang-new -fcolor-diagnostics a.f90 ! enable color and bold
$ flang-new -fcolor-diagnostics -fc1 a.f90 ! enable color and bold
$ flang-new -fno-color-diagnostics a.f90 ! disable color and bold
$ flang-new -fno-color-diagnostics -fc1 a.f90 ! disable color and bold

1. -fc1 needs to be the very first option, so flang-new -fno-color-diagnostics -fc1 doesn't make much sense

2. enable color and bold -> where?
   • In compiler driver diagnostics?
   • In frontend driver diagnostics?
   • In frontend (i.e. sema, parsing) diagnostics?

3. $ flang-new a.f90 ! enable color and bold -> I get no formatting with this revision.

Re 3, I believe that that's down to your changes in "TextDiagnosticPrinter.cpp". Since you are no longer using DiagnosticOptions directly, you will have to make sure that options in DiagnosticsEngine are set-up correctly. More specifically, there's ShowColors member variable in two places:

• DiagnosticsEngine
• DiagnosticOptions

These are separate and independent member variables. Before this change, AFAIK, Flang used ShowColors from DiagnosticOptions. Your patch changes that - is this required?

To me, the most natural approach would be to follow Clang's example:

1. flang-new should have the formatting enabled by default
2. flang-new -fc1 should have the formatting disabled by default
3. flang-new should forward it's settings to flang-new -fc1 when constructing a frontend driver job.

Item 3 above should translate to (skipping irrelevant frontend driver options):

• flang-new file.f90 --> flang-new -fc1 -fcolor-diagnostics file.f90
• flang-new -fno-color-diagnostics file.f90 --> flang-new -fc1 -fno-color-diagnostics file.f90 (or flang-new -fc1 file.f90)

There is a lot in here - I hope that this is not discouraging! Unfortunately, the diagnostics API that the driver library is using is very complex and convoluted. And additionally, we want different default behavior in every driver, which makes all this even more confusing. I'm hoping that this is helpful.

flang/include/flang/Frontend/FrontendOptions.h

217

This is out of order:

flang/include/flang/Frontend/FrontendOptions.h:217:9: error: field 'needProvenanceRangeToCharBlockMappings' will be initialized after field 'showColors' [-Werror,-Wreorder-ctor]
        needProvenanceRangeToCharBlockMappings(false), showColors(false) {}
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~~~
        showColors(false)                              needProvenanceRangeToCharBlockMappings(false)

[awarzynski]

clang/include/clang/Driver/Options.td

1358–1359

I would like to avoid any changes in Options.td unless ... unavoidable :)

When I disable one of them, I cannot get the color and bold messages.

Could you explain why?

[peixin]

@awarzynski Thanks a lot for the detailed explanations. I think doing two things in one patch gets me into trouble. I had thought flang-new -fc1 works as part of flang-new. Apparently, they have different philosophy. Now I make it clear. Add in the summary to describle what were missed before and what are changed in this patch and why.

clang/include/clang/Driver/Options.td

1358–1359

I made something wrong for this. Now fixed.

flang/include/flang/Frontend/FrontendOptions.h

217

Thanks for the notice. Fixed.

[awarzynski]

@peixin Thanks for the updates!

After parsing the option, it cannot be added to CmdArgs in Flang.cpp since the -fc1 is added by default for the compiler driver and the option -fno-color-diagnostics conflicts with -fc1. Instead, the parsing result is stored in diags options and push -fcolor-diagnostics to CmdArgs only if the ShowColors label of diags options is true.

I don't believe that's true :) How about this:

For the compiler driver, flang-new, the diagnostic options are parsed in createAndPopulateDiagOpts in driver.cpp. That's where the driver's DiagnosticEngine options are set. Like most command-line compiler driver options, these flags are "claimed" in Flang.cpp (i.e. when creating a frontend driver invocation) by calling getLastArg rather than in driver.cpp.

clang/include/clang/Driver/Options.td

1356–1357

This change makes sense to me. Basically, you are making sure that both invocations below will behave consistently:

$ clang -cc1 -fno-color-diagnostics file.c
error: unknown argument: '-fno-color-diagnostics'
$ flang-new -fc1 -fno-color-diagnostics file.f90
error: unknown argument: '-fno-color-diagnostics'

In particular, you avoid using OptInCC1FFlag for flang-new -fc1 (which, IMO, would be counterintuitive). But it would be great to double-check this with somebody from the Clang side of the universe :)

@jansvoboda11 @MaskRay Is this change in Options.td OK with you? Thanks!

clang/lib/Driver/ToolChains/Flang.cpp

110

IIRC, the diagnostic options are parsed here. So when we get here, these options should ideally be already "claimed". Could you expand the comment, please? The reason behind "argument unused" is unclear.

[MaskRay]

clang/include/clang/Driver/Options.td

1356–1357

I confirm that the behavior for Clang is still desired: both -f[no-]color-diagnostics are usable for driver while only the positive option exists for cc1.

But I am puzzled by this def fcolor_diagnostics change. Why can't flang-new use its own Options.td file?

[awarzynski]

clang/include/clang/Driver/Options.td

1356–1357

I confirm that the behavior for Clang is still desired

Thanks!

But I am puzzled by this def fcolor_diagnostics change. Why can't flang-new use its own Options.td file?

How would this help (i.e. without a major refactor)? IIUC, clangDriver creates one global instance of OptTable . So regardless whether there's a dedicated Options.td for flang-new or not, we'd be including the one for Clang that already defines these options, i.e.:

defm color_diagnostics : OptInCC1FFlag<"color-diagnostics", "Enable", "Disable", " colors in diagnostics",
  [CoreOption, FlangOption]>;

Separately, we'd need to define something similar for flang-new, e.g.:

defm flang_color_diagnostics : OptInFC1FFlag<"color-diagnostics", "Enable", "Disable", " colors in diagnostics",
  [CoreOption, FlangOption]>;

But how would the clangDriver parse -fcolor-diagnotiscs? As OPT_color_diagnostics or as OPT_flang_color_diagnostics?

I might be missing something obvious here. Until recently there have only been a handful of flang-new options and we've not investigated splitting Options.td too much. From my initial impression, that would require a fair bit of re-design work (e.g. shouldn't we extract a dedicated *.td file for clang -cc1 as well?).

I think that that's something outside the scope of this patch, but I would definitely support that. Is that what you had in mind?

[MaskRay]

clang/include/clang/Driver/Options.td

1356–1357

My question is why do these flang frontend need the Clang Options.td. It is like: rust/swift are not supposed to use Clang Options.td. They define their own options.

Does flang intend to support most Clang options so that it reuses Clang Options.td?

[peixin]

In D126164#3578702, @awarzynski wrote:

@peixin Thanks for the updates!

After parsing the option, it cannot be added to CmdArgs in Flang.cpp since the -fc1 is added by default for the compiler driver and the option -fno-color-diagnostics conflicts with -fc1. Instead, the parsing result is stored in diags options and push -fcolor-diagnostics to CmdArgs only if the ShowColors label of diags options is true.

I don't believe that's true :) How about this:

For the compiler driver, flang-new, the diagnostic options are parsed in createAndPopulateDiagOpts in driver.cpp. That's where the driver's DiagnosticEngine options are set. Like most command-line compiler driver options, these flags are "claimed" in Flang.cpp (i.e. when creating a frontend driver invocation) by calling getLastArg rather than in driver.cpp.

Thanks for this. Your desciptions are better.

Considering @awarzynski contributes a lot to this patch, add as co-author.

clang/lib/Driver/ToolChains/Flang.cpp

110

Borrow the comment from Clang.cpp.

[awarzynski]

clang/include/clang/Driver/Options.td

1356–1357

My question is why do these flang frontend need the Clang Options.td. It is like: rust/swift are not supposed to use Clang Options.td. They define their own options.

Flang is an in-tree project that builds its compiler driver in terms of clangDriver (CMake rule). Neither Rust nor Swift are in-tree (or use clangDriver, AFAIK).

Does flang intend to support most Clang options so that it reuses Clang Options.td?

No. But Options.td is already being shared between clang and clang -cc1, and these drivers don't share all of their options. Also, Options.td contained various Fortran flags even before LLVM Flang was merged into LLVM. My point being - Options.td is quite a mix regardless of whether LLVM Flang is there or not.

Btw, here's Flang driver documentation, and here's the original RFC in which it was proposed for LLVM Flang to re-use Clang's clangDriver. Just in case you were interested in the wider context (perhaps you're already familiar with this).

[awarzynski]

@peixin Thanks again for improving this! All in all it makes sense to me. However, given that I am now a co-author here (thanks for that!), we should make sure that at least one more reviewer approves this change :)

One other thing: given how fiddly and nuanced this change is, I would appreciate a bit more refined summary. Would be open to updating it? How about the following:

UPDATED SUMMARY BELOW

[flang][Driver] Refine _when_ driver diagnostics are formatted

This patch refines when driver diagnostics are formatted so that
flang-new and flang-new -fc1 behave consistently with clang and
clang -cc1, respectively. This change only applies to driver diagnostics
Scanning, parsing and semantic diagnostics are separate and not covered here.

NEW BEHAVIOUR
To illustrate the new behaviour, consider the following input file:

! file.f90
program m
  integer :: i = k
end

In the following invocations, "error: Semantic errors in file.f90" _will be_
formatted:

$ flang-new file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k
$ flang-new -fc1 -fcolor-diagnostics file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k

However, in the following invocations, "error: Semantic errors in file.f90"
_will not be_ formatted:

$ flang-new -fno-color-diagnostics file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k
$ flang-new -fc1 file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k

Before this change, none of the above would be formatted. Note also that the
default behaviour in flang-new is different to flang-new -fc1 (this is
consistent with Clang).

NOTES ON IMPLEMENTATION
Note that the diagnostic options are parsed in createAndPopulateDiagOpts in
driver.cpp. That's where the driver's DiagnosticEngine options are set. Like
most command-line compiler driver options, these flags are "claimed" in
Flang.cpp (i.e. when creating a frontend driver invocation) by calling
getLastArg rather than in driver.cpp.

In Clang's Options.td, defm color_diagnostics is replaced with two separate
definitions: def fcolor_diagnostics and def fno_color_diagnostics`. That's
because originally color_diagnostics derived from OptInCC1FFlag, which is a
multiclass for opt-in options in CC1. In order to preserve the current
behaviour in clang -cc1 (i.e. to keep -fno-color-diagnostics unavailable in
clang -cc1) and to implement similar behaviour in flang-new -fc1, we can't
re-use OptInCC1FFlag.

Formatting is only available in consoles that support it and will normally mean that
the message is printed in bold + color.

[peixin]

One other thing: given how fiddly and nuanced this change is, I would appreciate a bit more refined summary. Would be open to updating it? How about the following:

@awarzynski Thanks a lot for the summary. I think it's much better and more user-friendly. The patch supporting one feature of driver really should give the direction of how to use it in the commit message (summary). Updated it as suggested.

[peixin]

clang/include/clang/Driver/Options.td

1356–1357

My question is why do these flang frontend need the Clang Options.td. It is like: rust/swift are not supposed to use Clang Options.td. They define their own options.

Flang is an in-tree project that builds its compiler driver in terms of clangDriver (CMake rule). Neither Rust nor Swift are in-tree (or use clangDriver, AFAIK).

Does flang intend to support most Clang options so that it reuses Clang Options.td?

No. But Options.td is already being shared between clang and clang -cc1, and these drivers don't share all of their options. Also, Options.td contained various Fortran flags even before LLVM Flang was merged into LLVM. My point being - Options.td is quite a mix regardless of whether LLVM Flang is there or not.

Btw, here's Flang driver documentation, and here's the original RFC in which it was proposed for LLVM Flang to re-use Clang's clangDriver. Just in case you were interested in the wider context (perhaps you're already familiar with this).

1356–1357

ping @MaskRay

[rovka]

Hi Peixin,

I think this needs a few more tests. You should at least check that -fcolor-diagnostics is correctly forwarded to flang-new -fc1 (maybe add to this test), and also check that -fcolor-diagnostics and -fno-color-diagnostics are accepted or rejected by flang-new and flang-new -fc1. It would also be nice to check that -fcolor-diagnostics actually does something, if you can find an example where flang uses colors. You can find some inspiration in various clang or lld tests:
https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/color-diagnostics.c
https://github.com/llvm/llvm-project/blob/main/clang/test/Misc/diag-template-diffing-color.cpp
https://github.com/llvm/llvm-project/blob/main/clang/test/AST/ast-dump-color.cpp
https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/color-diagnostics.test

Hope that helps.

[peixin]

Add test cases as suggested. @rovka Thanks for the provided info. It really helps.

[awarzynski]

Thanks for the tests @peixin - they will be very helpful! I've left a few suggestions to make them a bit stricter. WDYT?

flang/test/Driver/color-diagnostics.f90

1 ↗

(On Diff #438606)

[nit] You are also testing the behavior of -f{no-}color-diagnostics here :)

2 ↗

(On Diff #438606)

Can you explain why? I'm guessing it's the same reason as https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/color-diagnostics.test#L1.

10–11 ↗

(On Diff #438606)

Could you also test:

! RUN: not %flang_fc1 %s 2>&1

?

17 ↗

(On Diff #438606)

This is identical to COMPILER_DRIVER_DC - why not use one LIT variable for this?

flang/test/Driver/frontend-forwarding.f90

21–39 ↗

(On Diff #438606)

1. I'd move it to a dedicated file. In the RUN line above, everything is just unconditionally forwarded for flang-new -fc1. In here, there's quite a lot of extra logic involved that's specific to -f{no-}color-diagnosstics. Alternatively, could you add comments to explain what this new block is meant to test?
2. What's CHECK-DC? Perhaps you meant CHECK-CD instead (i.e. CHECK-{C}OLOR_{D}IAGNOSTICS)?
3. ! CHECK-DC: "-fcolor-diagnostics" - could you be a bit more stricter here and replace with ! CHECK-DC: "-fc1"{{.*}} "-fcolor-diagnostics" ?

[peixin]

Format the test cases as @awarzynski suggested.

[awarzynski]

Thanks! LGTM :)

To me, Fangrui is OK with this change provided that it does not alter the behavior in Clang (that's indeed the case here). So, I would go ahead and merge it regardless of whether there's an explicit LGTM from a Clang developer or not. Unless there's more feedback!

Please, wait for Diana to confirm that she is also OK with the updates before merging.

Thanks for all the effort and for addressing all my comments - that's much appreciated!

[peixin]

OK. Thanks, @awarzynski .

[rovka]

Yep, LGTM, thanks for the tests!
(Disclaimer: I don't consider myself a Clang developer, so maybe wait a day or 2 before committing in case someone else has comments)

[peixin]

In D126164#3598847, @rovka wrote:

Yep, LGTM, thanks for the tests!
(Disclaimer: I don't consider myself a Clang developer, so maybe wait a day or 2 before committing in case someone else has comments)

Sure. Will wait for one more day before committing.

clang/include/clang/Driver/Options.td

def fbuiltin_module_map : Flag <["-"], "fbuiltin-module-map">, Group<f_Group>,
   Flags<[NoXarchOption]>, HelpText<"Load the clang builtins module map file.">;
 defm caret_diagnostics : BoolFOption<"caret-diagnostics",
   DiagnosticOpts<"ShowCarets">, DefaultTrue,
   NegFlag<SetFalse, [CC1Option]>, PosFlag<SetTrue>>;
 def fclang_abi_compat_EQ : Joined<["-"], "fclang-abi-compat=">, Group<f_clang_Group>,
   Flags<[CC1Option]>, MetaVarName<"<version>">, Values<"<major>.<minor>,latest">,
   HelpText<"Attempt to match the ABI of Clang <version>">;
 def fclasspath_EQ : Joined<["-"], "fclasspath=">, Group<f_Group>;
 defm color_diagnostics : OptInCC1FFlag<"color-diagnostics", "Enable", "Disable", " colors in diagnostics",
-  [CoreOption, FlangOption]>;
-def : Flag<["-"], "fdiagnostics-color">, Group<f_Group>, Flags<[CoreOption]>, Alias<fcolor_diagnostics>;
-def : Flag<["-"], "fno-diagnostics-color">, Group<f_Group>, Flags<[CoreOption]>, Alias<fno_color_diagnostics>;
+  [CoreOption, FlangOption, FC1Option]>;
+def : Flag<["-"], "fdiagnostics-color">, Group<f_Group>, Flags<[CoreOption, FlangOption, FC1Option]>, Alias<fcolor_diagnostics>;
+def : Flag<["-"], "fno-diagnostics-color">, Group<f_Group>, Flags<[CoreOption, FlangOption, FC1Option]>, Alias<fno_color_diagnostics>;
 def fdiagnostics_color_EQ : Joined<["-"], "fdiagnostics-color=">, Group<f_Group>;
 def fansi_escape_codes : Flag<["-"], "fansi-escape-codes">, Group<f_Group>,
   Flags<[CoreOption, CC1Option]>, HelpText<"Use ANSI escape codes for diagnostics">,
   MarshallingInfoFlag<DiagnosticOpts<"UseANSIEscapeCodes">>;
 def fcomment_block_commands : CommaJoined<["-"], "fcomment-block-commands=">, Group<f_clang_Group>, Flags<[CC1Option]>,
   HelpText<"Treat each comma separated argument in <arg> as a documentation comment block command">,
   MetaVarName<"<arg>">, MarshallingInfoStringVector<LangOpts<"CommentOpts.BlockCommandNames">>;
 def fparse_all_comments : Flag<["-"], "fparse-all-comments">, Group<f_clang_Group>, Flags<[CC1Option]>,

clang/lib/Driver/ToolChains/Flang.h

private:
 
   /// Extract preprocessing options from the driver arguments and add them to
   /// the preprocessor command arguments.
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
   void AddPreprocessingOptions(const llvm::opt::ArgList &Args,
                                llvm::opt::ArgStringList &CmdArgs) const;
+
+  /// Extract diagnostics options from the driver arguments and add them to the
+  /// command arguments.
+  ///
+  /// \param [in] Args The list of input driver arguments
+  /// \param [out] CmdArgs The list of output command arguments
+  void AddDiagnosticsOptions(const llvm::opt::ArgList &Args,
+                             llvm::opt::ArgStringList &CmdArgs) const;
+
   /// Extract other compilation options from the driver arguments and add them
   /// to the command arguments.
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
   void AddOtherOptions(const llvm::opt::ArgList &Args,
                        llvm::opt::ArgStringList &CmdArgs) const;
 

clang/lib/Driver/ToolChains/Flang.cpp

 
 void Flang::AddPreprocessingOptions(const ArgList &Args,
                                     ArgStringList &CmdArgs) const {
   Args.AddAllArgs(CmdArgs,
                   {options::OPT_P, options::OPT_D, options::OPT_U,
                    options::OPT_I, options::OPT_cpp, options::OPT_nocpp});
 }
 
+void Flang::AddDiagnosticsOptions(const ArgList &Args,
+                                  ArgStringList &CmdArgs) const {
+  Args.AddAllArgs(CmdArgs,
+                  {options::OPT_fcolor_diagnostics,
+                   options::OPT_fno_color_diagnostics});
+}
+
 void Flang::AddOtherOptions(const ArgList &Args, ArgStringList &CmdArgs) const {
   Args.AddAllArgs(CmdArgs,
                   {options::OPT_module_dir, options::OPT_fdebug_module_writer,
                    options::OPT_fintrinsic_modules_path, options::OPT_pedantic,
                    options::OPT_std_EQ, options::OPT_W_Joined});
 }
 
 void Flang::ConstructJob(Compilation &C, const JobAction &JA,
                          const InputInfo &Output, const InputInfoList &Inputs,
                          const ArgList &Args, const char *LinkingOutput) const {
   const auto &TC = getToolChain();
   const llvm::Triple &Triple = TC.getEffectiveTriple();
   const std::string &TripleStr = Triple.getTriple();
 
   ArgStringList CmdArgs;
void Flang::ConstructJob(Compilation &C, const JobAction &JA,
 
   // Add preprocessing options like -I, -D, etc. if we are using the
   // preprocessor (i.e. skip when dealing with e.g. binary files).
   if (types::getPreprocessedType(InputType) != types::TY_INVALID)
     AddPreprocessingOptions(Args, CmdArgs);
 
   AddFortranDialectOptions(Args, CmdArgs);
 
+  AddDiagnosticsOptions(Args, CmdArgs);
+
   // Add other compile options
   AddOtherOptions(Args, CmdArgs);
 
   // Forward -Xflang arguments to -fc1
   Args.AddAllArgValues(CmdArgs, options::OPT_Xflang);
 
   // Forward -mllvm options to the LLVM option parser. In practice, this means
   // forwarding to `-fc1` as that's where the LLVM parser is run.

flang/include/flang/Frontend/CompilerInvocation.h

 
 namespace Fortran::frontend {
 
 /// Fill out Opts based on the options given in Args.
 ///
 /// When errors are encountered, return false and, if Diags is non-null,
 /// report the error(s).
 bool parseDiagnosticArgs(clang::DiagnosticOptions &opts,
-                         llvm::opt::ArgList &args,
-                         bool defaultDiagColor = true);
+                         llvm::opt::ArgList &args);
 
 class CompilerInvocationBase {
 public:
   /// Options controlling the diagnostic engine.
   llvm::IntrusiveRefCntPtr<clang::DiagnosticOptions> diagnosticOpts;
   /// Options for the preprocessor.
   std::shared_ptr<Fortran::frontend::PreprocessorOptions> preprocessorOpts;
 

flang/include/flang/Frontend/FrontendOptions.h

const llvm::MemoryBuffer *getBuffer() const {
     return buffer;
   }
 };
 
 /// FrontendOptions - Options for controlling the behavior of the frontend.
 struct FrontendOptions {
   FrontendOptions()
       : showHelp(false), showVersion(false), instrumentedParse(false),
-        needProvenanceRangeToCharBlockMappings(false) {}
+        needProvenanceRangeToCharBlockMappings(false), showColors(false) {}
 
   /// Show the -help text.
   unsigned showHelp : 1;
 
   /// Show the -version text.
   unsigned showVersion : 1;
 
   /// Instrument the parse to get a more verbose log
   unsigned instrumentedParse : 1;
 
+  /// Enable color diagnostics.
+  unsigned showColors : 1;
+
   /// Enable Provenance to character-stream mapping. Allows e.g. IDEs to find
   /// symbols based on source-code location. This is not needed in regular
   /// compilation.
   unsigned needProvenanceRangeToCharBlockMappings : 1;
 
   /// Input values from `-fget-definition`
   struct GetDefinitionVals {
     unsigned line;

flang/lib/Frontend/CompilerInvocation.cpp

: diagnosticOpts(new clang::DiagnosticOptions(x.getDiagnosticOpts())),
       preprocessorOpts(new PreprocessorOptions(x.getPreprocessorOpts())) {}
 
 CompilerInvocationBase::~CompilerInvocationBase() = default;
 
 //===----------------------------------------------------------------------===//
 // Deserialization (from args)
 //===----------------------------------------------------------------------===//
 static bool parseShowColorsArgs(
-    const llvm::opt::ArgList &args, bool defaultColor) {
+    const llvm::opt::ArgList &args, bool defaultColor = true) {
   // Color diagnostics default to auto ("on" if terminal supports) in the driver
-  // but default to off in cc1, needing an explicit OPT_fdiagnostics_color.
+  // `flang-new` but default to off in `flang-new -fc1`, needing an explicit
+  // OPT_fdiagnostics_color.
   // Support both clang's -f[no-]color-diagnostics and gcc's
   // -f[no-]diagnostics-colors[=never|always|auto].
   enum {
     Colors_On,
     Colors_Off,
     Colors_Auto
   } showColors = defaultColor ? Colors_Auto : Colors_Off;
 
static bool parseShowColorsArgs(
   }
 
   return showColors == Colors_On ||
          (showColors == Colors_Auto &&
           llvm::sys::Process::StandardErrHasColors());
 }
 
 bool Fortran::frontend::parseDiagnosticArgs(clang::DiagnosticOptions &opts,
-                                            llvm::opt::ArgList &args,
-                                            bool defaultDiagColor) {
-  opts.ShowColors = parseShowColorsArgs(args, defaultDiagColor);
+                                            llvm::opt::ArgList &args) {
+  opts.ShowColors = parseShowColorsArgs(args);
 
   return true;
 }
 
 /// Parses all target input arguments and populates the target
 /// options accordingly.
 ///
 /// \param [in] opts The target options instance to update
if (args.hasArg(clang::driver::options::OPT_W_Joined)) {
     } else {
       const unsigned diagID =
           diags.getCustomDiagID(clang::DiagnosticsEngine::Error,
               "Only `-Werror` is supported currently.");
       diags.Report(diagID);
     }
   }
 
+  // Default to off for `flang-new -fc1`.
+  res.getFrontendOpts().showColors =
+      parseShowColorsArgs(args, /*defaultDiagColor=*/false);
+
   return diags.getNumErrors() == numErrorsBefore;
 }
 
 /// Parses all Dialect related arguments and populates the variables
 /// options accordingly. Returns false if new errors are generated.
 static bool parseDialectArgs(CompilerInvocation &res, llvm::opt::ArgList &args,
     clang::DiagnosticsEngine &diags) {
   unsigned numErrorsBefore = diags.getNumErrors();

flang/lib/Frontend/TextDiagnosticPrinter.cpp

void TextDiagnosticPrinter::HandleDiagnostic(
 
   llvm::raw_svector_ostream diagMessageStream(outStr);
 
   if (!prefix.empty())
     os << prefix << ": ";
 
   // We only emit diagnostics in contexts that lack valid source locations.
   assert(!info.getLocation().isValid() &&
          "Diagnostics with valid source location are not supported");
 
-  Fortran::frontend::TextDiagnostic::printDiagnosticLevel(os, level,
-                                                          diagOpts->ShowColors);
+  clang::DiagnosticsEngine &diagsEngine =
+      const_cast<clang::DiagnosticsEngine &>(*info.getDiags());
+  Fortran::frontend::TextDiagnostic::printDiagnosticLevel(
+      os, level, diagsEngine.getShowColors());
   Fortran::frontend::TextDiagnostic::printDiagnosticMessage(
       os,
       /*IsSupplemental=*/level == clang::DiagnosticsEngine::Note,
-      diagMessageStream.str(), diagOpts->ShowColors);
+      diagMessageStream.str(), diagsEngine.getShowColors());
 
   os.flush();
   return;
 }

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

if (!flang->getFrontendOpts().mlirArgs.empty()) {
     llvm::cl::ParseCommandLineOptions(numArgs + 1, args.get());
   }
 
   // If there were errors in processing arguments, don't do anything else.
   if (flang->getDiagnostics().hasErrorOccurred()) {
     return false;
   }
 
+  // Honor color diagnostics.
+  if (flang->getFrontendOpts().showColors) {
+    flang->getDiagnostics().setShowColors(true);
+  }
+
   // Create and execute the frontend action.
   std::unique_ptr<FrontendAction> act(createFrontendAction(*flang));
   if (!act)
     return false;
 
   bool success = flang->executeAction(*act);
   return success;
 }
 
 } // namespace Fortran::frontend

flang/test/Driver/driver-help.f90

 ! HELP-FC1-NEXT: -emit-llvm-bc          Build ASTs then convert to LLVM, emit .bc file
 ! HELP-FC1-NEXT: -emit-llvm Use the LLVM representation for assembler and object files
 ! HELP-FC1-NEXT: -emit-mlir Build the parse tree, then lower it to MLIR
 ! HELP-FC1-NEXT: -emit-obj Emit native object files
 ! HELP-FC1-NEXT: -E                     Only run the preprocessor
 ! HELP-FC1-NEXT: -falternative-parameter-statement
 ! HELP-FC1-NEXT: Enable the old style PARAMETER statement
 ! HELP-FC1-NEXT: -fbackslash            Specify that backslash in string introduces an escape character
+! HELP-FC1-NEXT: -fcolor-diagnostics     Enable colors in diagnostics
 ! HELP-FC1-NEXT: -fdebug-dump-all       Dump symbols and the parse tree after the semantic checks
 ! HELP-FC1-NEXT: -fdebug-dump-parse-tree-no-sema
 ! HELP-FC1-NEXT:                        Dump the parse tree (skips the semantic checks)
 ! HELP-FC1-NEXT: -fdebug-dump-parse-tree Dump the parse tree
 ! HELP-FC1-NEXT: -fdebug-dump-parsing-log
 ! HELP-FC1-NEXT:                   Run instrumented parse and dump the parsing log
 ! HELP-FC1-NEXT: -fdebug-dump-pft        Dump the pre-fir parse tree
 ! HELP-FC1-NEXT: -fdebug-dump-provenance Dump provenance

 ! HELP-FC1-NEXT: -finput-charset=<value> Specify the default character set for source files
 ! HELP-FC1-NEXT: -fintrinsic-modules-path <dir>
 ! HELP-FC1-NEXT:                        Specify where to find the compiled intrinsic modules
 ! HELP-FC1-NEXT: -flarge-sizes          Use INTEGER(KIND=8) for the result type in size-related intrinsics
 ! HELP-FC1-NEXT: -flogical-abbreviations Enable logical abbreviations
 ! HELP-FC1-NEXT: -fno-analyzed-objects-for-unparse
 ! HELP-FC1-NEXT:                        Do not use the analyzed objects when unparsing
 ! HELP-FC1-NEXT: -fno-automatic         Implies the SAVE attribute for non-automatic local objects in subprograms unless RECURSIVE
+! HELP-FC1-NEXT: -fno-color-diagnostics  Disable colors in diagnostics
 ! HELP-FC1-NEXT: -fno-reformat          Dump the cooked character stream in -E mode
 ! HELP-FC1-NEXT: -fopenacc              Enable OpenACC
 ! HELP-FC1-NEXT: -fopenmp               Parse OpenMP pragmas and generate parallel code.
 ! HELP-FC1-NEXT: -fxor-operator         Enable .XOR. as a synonym of .NEQV.
 ! HELP-FC1-NEXT: -help                  Display available options
 ! HELP-FC1-NEXT: -init-only             Only execute frontend initialization
 ! HELP-FC1-NEXT: -I <dir>               Add directory to the end of the list of include search paths
 ! HELP-FC1-NEXT: -load <dsopath>        Load the named plugin (dynamic shared object)