This is an archive of the discontinued LLVM Phabricator instance.

[flang] Introduce DiagnosticConsumer classes in libflangFrontend
ClosedPublic

Authored by awarzynski on Sep 16 2020, 9:12 AM.

Details

Summary

Currently Flang uses TextDiagnostic, TextDiagnosticPrinter &
TestDiagnosticBuffer from Clang (more specifically, from
libclangFrontend). This patch introduces simplified versions of these
classes in Flang (i.e. it removes the depdendency on libclangFrontend).

Note that currently, Flang needs these diagnostics classes only for the
compiler driver diagnostics. This is unlike in Clang in which similar
diagnostic classes are used for (among others) Lexing/Parsing/Sema
diagnostics. For this reason the implementations introduced here are
rather basic.

This patch also enhances how the diagnostics are printed. In particular,
this is what you'd get _before_ (no text formatting):

$ bin/flang-new
error: no input files

This is what you get _after_ (in terminals that support it,
the text is formatted - bold + red):

$ bin/flang-new
flang-new: error: no input files

Tests are updated accordingly and options related to enabling/disabling
color diagnostics are flagged as supported by Flang.

Diff Detail

Event Timeline

awarzynski created this revision.Sep 16 2020, 9:12 AM
Herald added a project: Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Sep 16 2020, 9:12 AM

I had a quick look through this patch. Have a few comments (mostly nits and questions) inline.

flang/include/flang/Frontend/CompilerInvocation.h
18

Nit: Opts -> opts?

20

Nit: Diags -> diags? Or is that convention?

flang/include/flang/Frontend/TextDiagnostic.h
10

An -> A

42

Nit: diagonstic -> diagnostic

45

Nit: missed doxygen comments? Or is it because it is a helper function?

57

Nit: OS->os?

flang/include/flang/Frontend/TextDiagnosticPrinter.h
15

Nit: Nos->NOS?

flang/include/flang/Frontend/TextDiagnosticPrinter.h
11

Where it prints depends on what stream is passed to the constructor of this class right? So is this standard error usage correct here?

flang/lib/Frontend/CompilerInvocation.cpp
75

Is diags not needed now?

flang/lib/Frontend/TextDiagnostic.cpp
23–24

Is the plan to make all these colors configurable?

70–75

Tempted to suggest saving a break here by reordering. :)

flang/lib/Frontend/TextDiagnosticBuffer.cpp
29

Is that a different smallstring from the one in LLVM?

flang/lib/Frontend/TextDiagnosticPrinter.cpp
38

100? Will this contain path names by any chance?

43

Is there an assumption that the prefix will not be empty?

flang/test/Flang-Driver/driver-help.f90
10

Should there be tests for gcc style as well?

flang/tools/flang-driver/driver.cpp
45

Nit: Inore->Ignore

105

Not clear why the stem needs to be taken here. Is this for windows compatibility?

flang/tools/flang-driver/fc1_main.cpp
39

Anyreason why this was moved up here (compared to the previous version which was close to its use)?

40

#JustAsking: Should this be deleted somewhere?

Thank you @awarzynski for this patch.
Very good that you added color feature for Fortran.
It is just a shame that we do not test. Is there a way to add a regression test for it?
I don't see any inconsistency with what we have on clang side, so it is good.
My comments are in the code, but two things that need an overall look:
1- style is not yet according to Flang, mainly functions and variables in the classes
2 -I believe that the name space is Fortran::frontend to be consistent with the other files at Frontend.

clang/include/clang/Driver/Options.td
874

Is it possible to also add when using FC1Option?

clang/tools/driver/driver.cpp
281 ↗(On Diff #292248)

undo

flang/include/flang/Frontend/TextDiagnostic.h
19

Do we need clang/Frontend here?

61

According to Flang style It should be PrintDiagnosticMessage, no?

flang/include/flang/Frontend/TextDiagnosticBuffer.h
24

It is inside Frontend, why you are not using Fortran::frontend?

43

a given

flang/include/flang/Frontend/TextDiagnosticPrinter.h
49

style

flang/lib/Frontend/CompilerInvocation.cpp
75

Why do you have clang::DiagnosticsEngine *diags

flang/lib/Frontend/TextDiagnostic.cpp
33

Why do static is commented?

flang/lib/Frontend/TextDiagnosticPrinter.cpp
53

Why when it is clang::DiagnosticsEngine::Note it is Supplemental?

flang/tools/flang-driver/driver.cpp
48

This is an odd part of the code, because missingArgIndex, missingArgCount are not set or even used in this part of the code.

52

can you do only diagOpts.showcolor= parseShowColorsArgs(args, defaultDiagColor)?
this function is not doing much now and it only adds another layer to understand the code.

Thanks for working on it.
Few comments inline:

  1. For an out-of-tree build, I see check-flang target failing with
/unittests/Frontend/CompilerInstanceTest.cpp:17:10: fatal error: filesystem: No such file or directory
 #include <filesystem>
          ^~~~~~~~~~~~

I used gcc/g++ 7.5 version.
I haven't checked in-tree still, and others/bots might have checked it.

  1. Either the documentation comments are wrong or code.

README mentions DBUILD_FLANG_NEW_DRIVER where as cmake ignores the flag for me.
Whereas, CMakeLists.txt mentions FLANG_BUILD_NEW_DRIVER.

flang/include/flang/Frontend/TextDiagnosticBuffer.h
37

How about simplifying this with using keyword?

flang/include/flang/Frontend/TextDiagnosticPrinter.h
38

Where is this used? I don't see any reference.

flang/lib/Frontend/TextDiagnostic.cpp
13

Is this header used ?

27

Unless Flang is not changing the color scheme w.r.t clang, can't the code be shared between both projects?

flang/lib/Frontend/TextDiagnosticPrinter.cpp
25

A silly question from what I see usually in Flang coding style.
Why isn't it defined in header file?

38

Can we use at places where LLVM data structures are used explicit llvm:: so an unknown user can easily identify where they come from?

awarzynski marked 17 inline comments as done.

Move code from Fortran to Fortran::frontend namespace, address PR comments

Thank you for reviewing! I think that I've addressed all your comments.. Please see the updated patch.

clang/include/clang/Driver/Options.td
874

I skipped it intentionally. In fc1_main.cpp we use TextDiagnosticBuffer instead of TextDiagnosticPrinter. The former will just print the diagnostics without pretty-formatting them, so this option wouldn't make much sense.

We could use TextDiagnosticPrinter in flang-new -fc1 (frontend driver) as we do for flang-new (compiler driver), but at this stage I would prefer to keep the design consistent with clang (i.e. this is the main reason why we use TextDiagnosticBuffer instead of TextDiagnosticPrinter in the frontend driver).

Having said all that, your comment makes me realise that I should improve the documentation, thanks!

flang/include/flang/Frontend/CompilerInvocation.h
20

Copy and paste error, thanks!

flang/include/flang/Frontend/TextDiagnostic.h
19

This should be replaced with #include "clang/Basic/Diagnostic.h". Thanks for pointing out!

61

Correct, thanks for catching this!

flang/include/flang/Frontend/TextDiagnosticBuffer.h
24

I've just copied the design from Clang - that's a rather disappointing explanation. But I agree that Fortran::frontend would be better. Updated.

flang/include/flang/Frontend/TextDiagnosticPrinter.h
11

Good catch. I've updated this and also the description of TextDiagnosticBuffer. Hopefully that will make the distinction clearer.

38

We won't be needing this :) Thanks for catching this!

49

I think that this is correct as is:

Accessor member functions are named with the non-public data member's name, less the trailing underscore.

https://github.com/llvm/llvm-project/blob/master/flang/docs/C%2B%2Bstyle.md#naming
3rd point

flang/lib/Frontend/CompilerInvocation.cpp
75

I missed that one. Thank you @CarolineConcatto and @kiranchandramohan !

flang/lib/Frontend/TextDiagnostic.cpp
13

Nope, thanks for catching this!

23–24
27

That would be preferable, I agree. However, keep in mind that in Clang these enums are defined in llvm-project/clang/lib/Frontend/TextDiagnostic.cpp , i.e. in libclangFrontend. As suggested in [1], we shouldn't be re-using anything from libclangFrontend.

We could move these enums out of libclangFrontend to some other library. That's not difficult and not that much work, but IMO that's a separate change that should happen on top of this patch (i.e. once this patch is approved). Also - where do we move them to? Currently there is no good candidate, but once the refactoring proposed in [1] is complete, there should be a dedicated library with infrastructure for the drivers to share.

I suggest that for now we just add a comment that recommends sharing these enums with Clang in the future. This could happen once we start refactoring Clang. In the meantime I'd like to keep the changes in Clang to the required minimum.

How does this sound? And does it make sense?

[1] http://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html

33

https://en.cppreference.com/w/cpp/language/static

The static keyword is only used with the declaration of a static member, inside the class definition, but not with the definition of that static member
70–75

Do suggest it and paste an example - my C++ foo is failing me here :)

flang/lib/Frontend/TextDiagnosticBuffer.cpp
29

It's the same SmallString. I'm using a forward declaration from clang/Basic/LLVM.h, which I do include, but I shouldn't. Fixed!

flang/lib/Frontend/TextDiagnosticPrinter.cpp
25

No such thing as silly questions! :)

AFAIK, there are no rules in the coding guidelines with regard to

  • where things should be defined, and
  • where things should be declared.

I use this rather generic rule of thumb: declare in headers, define in source files. In this particular case - I followed what was already in Clang. It made sense to me.

Do you think that it would better to define this in a header file?

38

If we do that, the code is likely to be bloated with llvm::, which IMO could be detrimental to readability in the long term.

In Clang you will find a header file with all the forward declarations to avoid this: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/LLVM.h.

I definitely want to make reading this code easier to users, but I also think that instead of llvm:: one could use code-navigation tools (e.g. clangd). And forward declarations are fairly standard AFAIK.

However, if you still think that llvm:: would be better, I'm happy to update :)

38

It shouldn't. Currently these diagnostics are only used to report errors from the command line, so no paths are involved.

In Clang, similar class is used both for the driver diagnostics (i..e command line errors - no source location) and frontend diagnostics (source code errors - with source locations). However, AFAIK, this string is not used for the source location (e.g. path).

If we do use this class for more Flang diagnostics, we shouldn't use outStr for paths. This is just the message.

43

The prefix is optional. With prefix:

$ bin/flang-new
flang-new: error: no input files

Without prefix:

$ bin/flang-new
error: no input files

Both are valid.

53

I'm not aware of any documentation for this, so here's my understanding :)

A clang::DiagnosticsEngine::Note is an additional piece of information for the user. It's neither a warning nor an error. Hence it's something _supplemental_ (i.e. additional info that could otherwise be skipped) and should not be pretty-printed. And that's the purpose of this argument.

flang/test/Flang-Driver/driver-help.f90
10

That's a good point,thanks! However, I'd like to defer this decision till later.

Currently we have enough options to support what is implemented in this driver. At some point we will need to have a discussion with regard to what options should be supported and displayed. This is not obvious to me just now. For example, Clang supports GCC style flags for diagnostic colors, but doesn't display them when running clang --help.

And once we do start discussing this in the context of Flang, any changes will very likely affect Clang too. I would prefer to focus on the required minimum before offering a complete set of supported option variants :)

flang/tools/flang-driver/driver.cpp
48

They are required when calling ParseArgs (that's also where they are set). As mentioned in the comment above, we ignore them as any errors that these variables would indicate, will be captured elsewhere anyway.

52

That's a good point, but then I have to move the definition of parseShowColorsArgs into this file. This will bloat this file with stuff that should be abstracted away.

105

That too. In general, to canonicalise the path.

For example, we want:

$ bin/flang-new
flang-new: error: no input files

instead of:

$ bin/flang-new
<some-very-long-path>/bin/flang-new: error: no input files
flang/tools/flang-driver/fc1_main.cpp
39

Sorry, this is unrelated.

I wanted to better highlight that we are using TextDiagnosticBuffer (instead of e.g. TextDiagnosticPrinter) and to separate the creation of CompilerInvocation from this.

40

DiagnosticsConsumer (in this case, diagsBuffer), is required to construct diags, i.e. an instance of DiagnosticsEngine.

DiagnosticsEngine::DiagnosticsEngine(
    IntrusiveRefCntPtr<DiagnosticIDs> diags,
    IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts, DiagnosticConsumer *client,
    bool ShouldOwnClient)
    : Diags(std::move(diags)), DiagOpts(std::move(DiagOpts)) {
  setClient(client, ShouldOwnClient);
  ArgToStringFn = DummyArgToStringFn;

  Reset();
}
explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags,
                           IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts,
                           DiagnosticConsumer *client = nullptr,
                           bool ShouldOwnClient = true);

Note that ShouldOwnClient defaults to true. So this should be fine #fingers-crossed :)

awarzynski added a comment.EditedSep 24 2020, 2:23 AM

@sameeranjoshi Apologies, I missed some of your comments.

Thanks for working on it.
Few comments inline:

  1. For an out-of-tree build, I see check-flang target failing with
/unittests/Frontend/CompilerInstanceTest.cpp:17:10: fatal error: filesystem: No such file or directory
 #include <filesystem>
          ^~~~~~~~~~~~

I used gcc/g++ 7.5 version.
I haven't checked in-tree still, and others/bots might have checked it.

I haven't been able to reproduce it, but note that filesystem is a C++17 header. AFAIK, in GCC-7 you have to use <experimental/filesystem> instead of <filesystem>. This suggests README.md should be updated: https://github.com/llvm/llvm-project/blob/master/flang/README.md#supported-c-compilers (we probably need an RFC for this). Since this #include was introduced elsewhere, I suggest that we move this discussion either to Slack or flang-dev.

  1. Either the documentation comments are wrong or code.

README mentions DBUILD_FLANG_NEW_DRIVER where as cmake ignores the flag for me.
Whereas, CMakeLists.txt mentions FLANG_BUILD_NEW_DRIVER.

Sorry, that's an unfortunate typo. Fixed here: https://github.com/llvm/llvm-project/commit/27da2875070ac00f6cd9f8040c6f994aca915406

flang/lib/Frontend/TextDiagnosticPrinter.cpp
25

I think in flang the style is to declare the class in the source file if it is only used in that file. If it is used elsewhere then put it in the header. If it is used in another directory then move the header to include directory.

@sameeranjoshi Regarding <filesystem>, please see https://reviews.llvm.org/D88219. Sadly we missed that when adding that test, sorry!

Rebase on top of master

sameeranjoshi added inline comments.Sep 24 2020, 11:23 AM
flang/lib/Frontend/TextDiagnostic.cpp
27

Agreed.

@sameeranjoshi Apologies, I missed some of your comments.

Thanks for working on it.
Few comments inline:

  1. For an out-of-tree build, I see check-flang target failing with
/unittests/Frontend/CompilerInstanceTest.cpp:17:10: fatal error: filesystem: No such file or directory
 #include <filesystem>
          ^~~~~~~~~~~~

I used gcc/g++ 7.5 version.
I haven't checked in-tree still, and others/bots might have checked it.

I haven't been able to reproduce it, but note that filesystem is a C++17 header. AFAIK, in GCC-7 you have to use <experimental/filesystem> instead of <filesystem>. This suggests README.md should be updated: https://github.com/llvm/llvm-project/blob/master/flang/README.md#supported-c-compilers (we probably need an RFC for this). Since this #include was introduced elsewhere, I suggest that we move this discussion either to Slack or flang-dev.

Do you know if there are any bots configured to handle out-of-tree changes?
That might be helpful to avoid configuration differences and test OFT patches.

sameeranjoshi added inline comments.Sep 24 2020, 11:31 AM
flang/lib/Frontend/TextDiagnosticPrinter.cpp
38

I think it's better to follow what FIR(fir-dev) and llvm-project/flang does.
They generally try to fully-qualify name of library and avoid using forward declarations.

Thank you @awarzynski for updating the patch.
It looks good to me. I've build it locally and played a little bit.
It works fine with gcc9.3. I did not try to build out-of-tree. But I don't see any reason it should not work.
I have two minor comments. But besides that the patch looks good to me.
If you fix them it is all fine for me.

flang/lib/Frontend/CompilerInvocation.cpp
75

Am I missing something or this is still here?

flang/tools/flang-driver/driver.cpp
14

Why do we need invocation here?

awarzynski marked 4 inline comments as done.Sep 28 2020, 10:04 AM

Do you know if there are any bots configured to handle out-of-tree changes?
That might be helpful to avoid configuration differences and test OFT patches.

I'm not aware of a buildbot that would test out-of-tree builds. I also don't know how hard it would be to set one up (AFAIK, there's no precedence of out-of-tree LLVM buildbots). Defending every sub-project/feature that people care about with a buildbot is important. Sadly I won't have the bandwidth to set one up for out-of-tree builds any time soon. This my gentle call for volunteers :)

flang/lib/Frontend/TextDiagnosticPrinter.cpp
25

This class is used in multiple files:

  • flang/lib/Frontend/CompilerInstance.{cpp|h}
  • flang/tools/flang-driver/driver.cpp
  • flang/unittests/Frontend/CompilerInstanceTest.cpp

IIUC, this is now resolved.

flang/tools/flang-driver/driver.cpp
14

Required for ParseDiagnosticArgs.

awarzynski marked an inline comment as done.

Address the remaining PR comments, fix test

Do you know if there are any bots configured to handle out-of-tree changes?
That might be helpful to avoid configuration differences and test OFT patches.

I'm not aware of a buildbot that would test out-of-tree builds. I also don't know how hard it would be to set one up (AFAIK, there's no precedence of out-of-tree LLVM buildbots). Defending every sub-project/feature that people care about with a buildbot is important. Sadly I won't have the bandwidth to set one up for out-of-tree builds any time soon. This my gentle call for volunteers :)

https://reviews.llvm.org/D87085. Thanks @rovka.

CarolineConcatto added a comment.EditedSep 28 2020, 11:58 PM

Hi Folks,
@awarzynski and @sameeranjoshi, Diana was working in an out-of-tree build.

This is the patch:
https://reviews.llvm.org/D87085

CarolineConcatto accepted this revision.Oct 5 2020, 2:27 AM

Thank you @awarzynski for working on this.
It is good because now we do not depend on clang frontend to build flang frontend.
LGTM

This revision is now accepted and ready to land.Oct 5 2020, 2:27 AM
sameeranjoshi accepted this revision.Oct 5 2020, 7:25 AM

Thanks for patch.
Looks good.

This revision was landed with ongoing or failed builds.Oct 5 2020, 9:51 AM
This revision was automatically updated to reflect the committed changes.