Notes in Clang diagnostics and LLVM DiagnosticInfo

Hi,

I just saw the LLVM DiagnosticInfo being integrated into clang in
r200931 by Quentin and noticed interesting behaviour when using Notes.

Specifically, I was using the interface in Polly to emit informative messages to the user (similar to icc's -vec-report). To emit this information I used the Severity 'Note' as these are neither warnings nor errors, just pieces of information that help the user understand what happens.

However, when running clang I realized my notes are not emitted as expected. I tracked it down to this piece of code:

// If the client doesn't care about this message, don't issue it.
// If this is a note and the last real diagnostic was ignored, ignore
// it too.
   if (DiagLevel == DiagnosticIDs::Ignored ||
       (DiagLevel == DiagnosticIDs::Note &&
        Diag.LastDiagLevel == DiagnosticIDs::Ignored))
     return false;

as a piece of additional information for 'real diagnostic'. Now I wonder what is the right way to emit independent informative messages. I see two approaches:

1) Make it possible to create first-class notes, that do not depend on previous diagnostics and that are not ignored.

2) Introduce an additional severity level (e.g. Info) that is a first class Diagnostic.

2) allows us to also provide 'notes' for our info-diagnostics.

E.g. something like this:

/tmp/test.c:1:6: info: Polly detected an optimizable code region
/tmp/test.c:3:9: note: End of optimizable code region

Does anybody have some ideas/opinions regarding this issue?

Cheers,
Tobias

Hi Tobias,
I got the problem.
I use the DiagnostigBuilder::setForceEmit() for this.
An instance of DiagnostigBuilder will be returned if you use the
report-function.

Hope this helps.

Best,
Robert

Good point. The following patch makes my messages appear again:

--- a/lib/CodeGen/CodeGenAction.cpp
+++ b/lib/CodeGen/CodeGenAction.cpp
@@ -384,7 +384,9 @@ void BackendConsumer::DiagnosticHandlerImpl(const DiagnosticInfo &DI) {

    // Report the backend message using the usual diagnostic mechanism.
    FullSourceLoc Loc;
- Diags.Report(Loc, DiagID).AddString(MsgStorage);
+ DiagnosticBuilder DB = Diags.Report(Loc, DiagID);
+ DB.setForceEmit();
+ DB.AddString(MsgStorage);

We could use this to either force printing of all backend diagnostics or only the notes. However, I am afraid that would break existing flags to suppress diagnostics.

Another option is to only set this for plugin diagnostics. However, this would not solve the problem for the vectorizers which may need similar reporting capabilities.

Tobias

Hi Tobias,

Thanks for looking into this.

Hi Tobias,
I got the problem.
I use the DiagnostigBuilder::setForceEmit() for this.
An instance of DiagnostigBuilder will be returned if you use the
report-function.

Good point. The following patch makes my messages appear again:

--- a/lib/CodeGen/CodeGenAction.cpp
+++ b/lib/CodeGen/CodeGenAction.cpp
@@ -384,7 +384,9 @@ void BackendConsumer::DiagnosticHandlerImpl(const DiagnosticInfo &DI) {

  // Report the backend message using the usual diagnostic mechanism.
  FullSourceLoc Loc;
- Diags.Report(Loc, DiagID).AddString(MsgStorage);
+ DiagnosticBuilder DB = Diags.Report(Loc, DiagID);
+ DB.setForceEmit();
+ DB.AddString(MsgStorage);

We could use this to either force printing of all backend diagnostics or only the notes. However, I am afraid that would break existing flags to suppress diagnostics.

Would it be hard to check what is the actual behavior?
That would indeed be unfortunate!

-Quentin

Hi Tobias,
I got the problem.
I use the DiagnostigBuilder::setForceEmit() for this.
An instance of DiagnostigBuilder will be returned if you use the
report-function.

Good point. The following patch makes my messages appear again:

--- a/lib/CodeGen/CodeGenAction.cpp
+++ b/lib/CodeGen/CodeGenAction.cpp
@@ -384,7 +384,9 @@ void BackendConsumer::DiagnosticHandlerImpl(const
DiagnosticInfo &DI) {

   // Report the backend message using the usual diagnostic mechanism.
   FullSourceLoc Loc;
- Diags.Report(Loc, DiagID).AddString(MsgStorage);
+ DiagnosticBuilder DB = Diags.Report(Loc, DiagID);
+ DB.setForceEmit();
+ DB.AddString(MsgStorage);

We could use this to either force printing of all backend diagnostics or
only the notes. However, I am afraid that would break existing flags to
suppress diagnostics.

Please don't do this. Notes are designed to attach additional information
to the prior non-note diagnostic, and this assumption is pervasive within
the diagnostics mechanism inside clang, as well as in the model we present
to our users and in our API.

Adding an 'info' or 'remark' level for diagnostics seems much more in line
with our existing design. It may be of somewhat limited utility, but the
cost of having this support seems rather low. (In the long term, I assume
that end users of polly will not care about these diagnostics, and that
they're primarily to aid people developing or testing polly -- and
certainly this does not match how other optimization passes in LLVM report
debug or progress information to their users.) FWIW, some other compilers
have a 'remark' diagnostic level, for messages that don't rise to the level
of 'warning'.

Another option is to only set this for plugin diagnostics. However, this

Hi Tobias,
I got the problem.
I use the DiagnostigBuilder::setForceEmit() for this.
An instance of DiagnostigBuilder will be returned if you use the
report-function.

Good point. The following patch makes my messages appear again:

--- a/lib/CodeGen/CodeGenAction.cpp
+++ b/lib/CodeGen/CodeGenAction.cpp
@@ -384,7 +384,9 @@ void BackendConsumer::DiagnosticHandlerImpl(const
DiagnosticInfo &DI) {

    // Report the backend message using the usual diagnostic mechanism.
    FullSourceLoc Loc;
- Diags.Report(Loc, DiagID).AddString(MsgStorage);
+ DiagnosticBuilder DB = Diags.Report(Loc, DiagID);
+ DB.setForceEmit();
+ DB.AddString(MsgStorage);

We could use this to either force printing of all backend diagnostics or
only the notes. However, I am afraid that would break existing flags to
suppress diagnostics.

Please don't do this. Notes are designed to attach additional information
to the prior non-note diagnostic, and this assumption is pervasive within
the diagnostics mechanism inside clang, as well as in the model we present
to our users and in our API.

That's what I assumed. Thanks for verifying.

Adding an 'info' or 'remark' level for diagnostics seems much more in line
with our existing design. It may be of somewhat limited utility, but the
cost of having this support seems rather low. (In the long term, I assume
that end users of polly will not care about these diagnostics, and that
they're primarily to aid people developing or testing polly -- and
certainly this does not match how other optimization passes in LLVM report
debug or progress information to their users.) FWIW, some other compilers
have a 'remark' diagnostic level, for messages that don't rise to the level
of 'warning'.

The intention is to support something similar to -vec-report in icc. Information about loops that have been vectorized, transformed, ...

To my knowledge, at the moment LLVM is not really providing such kind of diagnostics to the users. The majority of the users probably would not look at them, but people working on performance or trying to understand what happens will be interested in such kind of diagnostics.

I submitted two patches to LLVM and clang that introduce new 'remark' level diagnostics.

Cheers,
Tobias

As Richard confirmed, using 'note' to provide information not attached to another warning or error is not intended. I submitted two patches
to introduce a new 'remark' type that can be used for exactly this purpose.

Tobias

How about calling these "informative" diagnostics, and use "info:" for them? That seems more descriptive than remark.

FWIW, I agree with Richard, notes are always supposed to be the continuation of an existing diagnostic.

-Chris

Seems like a good idea. :slight_smile:

-eric