[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

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

flang/lib/Frontend/TextDiagnosticPrinter.cpp

51

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

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

Agree. Clang does it, too.

flang/lib/Frontend/TextDiagnosticPrinter.cpp

51

Good point. Will fix this.

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

75

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

-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 ↗

(On Diff #434324)

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

51–52 ↗

(On Diff #434324)

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 ↗

(On Diff #434324)

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

51–52 ↗

(On Diff #434324)

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 ↗

(On Diff #434324)

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

flang/lib/Frontend/CompilerInvocation.cpp

500–501 ↗

(On Diff #434324)

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 ↗

(On Diff #435025)

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 ↗

(On Diff #434324)

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 ↗

(On Diff #434324)

I made something wrong for this. Now fixed.

flang/include/flang/Frontend/FrontendOptions.h

217 ↗

(On Diff #435025)

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–1360 ↗

(On Diff #435405)

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

104 ↗

(On Diff #435405)

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–1360 ↗

(On Diff #435405)

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–1360 ↗

(On Diff #435405)

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–1360 ↗

(On Diff #435405)

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

104 ↗

(On Diff #435405)

Borrow the comment from Clang.cpp.

[awarzynski]

clang/include/clang/Driver/Options.td

1356–1360 ↗

(On Diff #435405)

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–1360 ↗

(On Diff #435405)

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–1360 ↗

(On Diff #435405)

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.

flang/lib/Frontend/CompilerInstance.cpp

CompilerInstance::createDiagnostics(clang::DiagnosticOptions *opts,
 
   // Create the diagnostic client for reporting errors or for
   // implementing -verify.
   if (client) {
     diags->setClient(client, shouldOwnClient);
   } else {
     diags->setClient(new TextDiagnosticPrinter(llvm::errs(), opts));
   }
+  diags->setShowColors(true);
   return diags;
 }

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 &diagsRef =
+      const_cast<clang::DiagnosticsEngine &>(*info.getDiags());
+  Fortran::frontend::TextDiagnostic::printDiagnosticLevel(
+      os, level, diagsRef.getShowColors());
   Fortran::frontend::TextDiagnostic::printDiagnosticMessage(
       os,
       /*IsSupplemental=*/level == clang::DiagnosticsEngine::Note,
-      diagMessageStream.str(), diagOpts->ShowColors);
+      diagMessageStream.str(), diagsRef.getShowColors());
 
   os.flush();
   return;
 }

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

 ! CHECK-NEXT: -U <macro>             Undefine macro <macro>
 ! CHECK-NEXT: --version Print version information
 ! CHECK-NEXT: -W<warning>            Enable the specified warning
 ! CHECK-NEXT: -Xflang <arg>          Pass <arg> to the flang compiler
 
 !-------------------------------------------------------------
 ! EXPECTED OUTPUT FOR FLANG DRIVER (flang-new)
 !-------------------------------------------------------------
-! ERROR-FLANG: error: unknown argument '-help-hidden'; did you mean '--help-hidden'?
+! ERROR-FLANG: error: {{.*}}unknown argument '-help-hidden'; did you mean '--help-hidden'?
 
 !-------------------------------------------------------------
 ! EXPECTED OUTPUT FOR FLANG FRONTEND DRIVER (flang-new -fc1)
 !-------------------------------------------------------------
 ! Frontend driver -help-hidden is not supported
-! ERROR-FLANG-FC1: error: unknown argument: '{{.*}}'
+! ERROR-FLANG-FC1: error: {{.*}}unknown argument: '{{.*}}'
 

flang/test/Driver/driver-help.f90

 ! HELP-FC1-NEXT: -triple <value>        Specify target triple (e.g. i686-apple-darwin9)
 ! HELP-FC1-NEXT: -U <macro>             Undefine macro <macro>
 ! HELP-FC1-NEXT: -version               Print the compiler version
 ! HELP-FC1-NEXT: -W<warning>            Enable the specified warning
 
 !---------------
 ! EXPECTED ERROR
 !---------------
-! ERROR: error: unknown argument '-helps'; did you mean '-help'
+! ERROR: error: {{.*}}unknown argument '-helps'; did you mean '-help'

flang/test/Driver/driver-version.f90

 ! VERSION-NEXT: Target:
 ! VERSION-NEXT: Thread model:
 ! VERSION-NEXT: InstalledDir:
 
 ! ERROR: flang-new: error: unsupported option '--versions'; did you mean '--version'?
 
 ! VERSION-FC1: LLVM version
 
-! ERROR-FC1: error: unknown argument '--version'; did you mean '-version'?
+! ERROR-FC1: error: {{.*}}unknown argument '--version'; did you mean '-version'?

flang/test/Driver/dump-all-bad.f90

 ! Verify that -fdebug-dump-all dumps both symbols and the parse tree, even when semantic errors are present
 
 !----------
 ! RUN lines
 !----------
 ! RUN: not %flang_fc1 -fdebug-dump-all %s 2>&1 | FileCheck %s
 
 !----------------
 ! EXPECTED OUTPUT
 !----------------
-! CHECK: error: Semantic errors in
+! CHECK: error: {{.*}}Semantic errors in
 ! CHECK: Flang: parse tree dump
 ! CHECK: Flang: symbols dump
 
 !-------
 ! INPUT
 !-------
 program bad
   real,pointer :: x
   x = null()      ! Error - must be pointer assignment
 end

flang/test/Driver/get-definition.f90

 ! RUN: not %flang_fc1 -fget-definition a 1 c %s 2>&1 | FileCheck --check-prefix=ERROR-ac %s
 
 !-----------------
 ! EXPECTED OUTPUT
 !-----------------
 ! OK: String range: >m<
 ! OK-NOT: error
 
-! ERROR-a: error: invalid value 'a' in 'fget-definition'
+! ERROR-a: error: {{.*}}invalid value 'a' in 'fget-definition'
 ! ERROR-a-NOT: String range: >m<
 
-! ERROR-b: error: invalid value 'b' in 'fget-definition'
+! ERROR-b: error: {{.*}}invalid value 'b' in 'fget-definition'
 ! ERROR-b-NOT: String range: >m<
 
-! ERROR-c: error: invalid value 'c' in 'fget-definition'
+! ERROR-c: error: {{.*}}invalid value 'c' in 'fget-definition'
 ! ERROR-c-NOT: String range: >m<
 
-! ERROR-ab: error: invalid value 'a' in 'fget-definition'
+! ERROR-ab: error: {{.*}}invalid value 'a' in 'fget-definition'
 ! ERROR-ab-NOT: String range: >m<
 
-! ERROR-ac: error: invalid value 'a' in 'fget-definition'
+! ERROR-ac: error: {{.*}}invalid value 'a' in 'fget-definition'
 ! ERROR-ac-NOT: String range: >m<
 
-! ERROR-bc: error: invalid value 'b' in 'fget-definition'
+! ERROR-bc: error: {{.*}}invalid value 'b' in 'fget-definition'
 ! ERROR-bc-NOT: String range: >m<
 
-! ERROR-abc: error: invalid value 'a' in 'fget-definition'
+! ERROR-abc: error: {{.*}}invalid value 'a' in 'fget-definition'
 ! ERROR-abc-NOT: String range: >m<
 
 !-------
 ! INPUT
 !-------
 module m
 end module

flang/test/Driver/init-only.f90

 ! Verify that -init-only flag generates a diagnostic as expected
 
 ! RUN: %flang_fc1 -init-only 2>&1 | FileCheck %s
 
-! CHECK: warning: Use `-init-only` for testing purposes only
+! CHECK: warning: {{.*}}Use `-init-only` for testing purposes only

flang/test/Driver/missing-arg.f90

 ! RUN: not %flang_fc1 -E %s -J 2>&1 | FileCheck %s
 ! RUN: not %flang_fc1 -E %s -module-dir 2>&1 | FileCheck %s
 ! RUN: not %flang_fc1 -E %s -module-suffix 2>&1 | FileCheck %s
 ! RUN: not %flang_fc1 -E %s -fintrinsic-modules-path 2>&1 | FileCheck %s
 
 !-----------------------
 ! EXPECTED OUTPUT
 !-----------------------
-! CHECK: error: argument to '-{{.*}}' is missing (expected 1 value)
+! CHECK: error: {{.*}}argument to '-{{.*}}' is missing (expected 1 value)

flang/test/Driver/missing-triple.ll

 ; Verify that the module triple is overridden by the driver - even when the
 ; module triple is missing.
 ; NOTE: At the time of writing, the tested behaviour was consistent with Clang
 
 ;-------------
 ; RUN COMMANDS
 ;-------------
 ; RUN: %flang_fc1 -S %s -o - 2>&1 | FileCheck %s
 ; RUN: %flang -S  %s -o - 2>&1 | FileCheck %s
 
 ;----------------
 ; EXPECTED OUTPUT
 ;----------------
-; CHECK: warning: overriding the module target triple with {{.*}}
+; CHECK: warning: {{.*}}overriding the module target triple with {{.*}}
 
 ;------
 ; INPUT
 ;------
 define void @foo() {
   ret void
 }

flang/test/Driver/multiple-actions-error.f95

 ! Verify that the frontend driver error-out if multiple actions are specified
 
 ! RUN: not %flang_fc1 -E -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=ERROR
 ! RUN: not %flang_fc1 -fsyntax-only -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=ERROR
 
-! ERROR: error: Only one action option is allowed
+! ERROR: error: {{.*}}Only one action option is allowed
 
 end progream

flang/test/Driver/output-paths.f90

 ! Test the diagnostic for cases when the output file cannot be generated
 
 !--------------------------
 ! RUN lines
 !--------------------------
 ! RUN: not %flang_fc1 -E -o %t.doesnotexist/somename %s 2> %t
 ! RUN: FileCheck -check-prefix=OUTPUTFAIL -DMSG=%errc_ENOENT -input-file=%t %s
 
 !-----------------------
 ! EXPECTED OUTPUT
 !-----------------------
-! OUTPUTFAIL: error: unable to open output file '{{.*}}doesnotexist{{.}}somename': '[[MSG]]'
+! OUTPUTFAIL: error: {{.*}}unable to open output file '{{.*}}doesnotexist{{.}}somename': '[[MSG]]'

flang/test/Driver/override-triple.ll

 ; Verify that the module triple is overridden by the driver - even in the presence
 ; of a module triple.
 ; NOTE: At the time of writing, the tested behaviour was consistent with Clang
 
 ;-------------
 ; RUN COMMANDS
 ;-------------
 ; RUN: %flang_fc1 -S %s -o - 2>&1 | FileCheck %s
 ; RUN: %flang -S  %s -o - 2>&1 | FileCheck %s
 
 ;----------------
 ; EXPECTED OUTPUT
 ;----------------
-; CHECK: warning: overriding the module target triple with {{.*}}
+; CHECK: warning: {{.*}}overriding the module target triple with {{.*}}
 
 ;------
 ; INPUT
 ;------
 ; For the triple to be overridden by the driver, it needs to be different to the host triple.
 ; Use a random string to guarantee that.
 target triple = "invalid-triple"
 
 define void @foo() {
   ret void
 }

flang/test/Driver/plugin-invalid-name.f90

 ! Check the correct error diagnostic is reported when a plugin name isn't found
 
 ! REQUIRES: plugins, shell
 
 ! RUN: not %flang_fc1 -plugin -wrong-name %s 2>&1 | FileCheck %s --check-prefix=ERROR
 
-! ERROR: error: unable to find plugin '-wrong-name'
+! ERROR: error: {{.*}}unable to find plugin '-wrong-name'

flang/test/Driver/syntax-only.f90

 ! RUN: rm -rf %t/non-existent-dir/
 ! RUN: not %flang -c %s -o %t/non-existent-dir/syntax-only.o 2>&1 | FileCheck %s --check-prefix=NO_FSYNTAX_ONLY
 ! RUN: not %flang_fc1 -emit-obj %s -o %t/non-existent-dir/syntax-only.o 2>&1 | FileCheck %s --check-prefix=NO_FSYNTAX_ONLY
 
 !-----------------
 ! EXPECTED OUTPUT
 !-----------------
 ! CHECK-NOT: error
-! NO_FSYNTAX_ONLY: error: failed to create the output file
+! NO_FSYNTAX_ONLY: error: {{.*}}failed to create the output file
 
 !-------
 ! INPUT
 !-------
 end program

flang/test/Driver/use-module-error.f90

 !-----------------------------------------
 ! RUN: not %flang_fc1 -fsyntax-only -J %S/Inputs/module-dir -J %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
 ! RUN: not %flang_fc1 -fsyntax-only -J %S/Inputs/module-dir -module-dir %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
 ! RUN: not %flang_fc1 -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
 
 !-----------------------------------------
 ! EXPECTED OUTPUT FOR MISSING MODULE FILE
 !-----------------------------------------
-! DOUBLEINCLUDE:error: Only one '-module-dir/-J' option allowed
+! DOUBLEINCLUDE: error: {{.*}}Only one '-module-dir/-J' option allowed
 
 program too_many_module_dirs
 end