-ferror-limit omits part of last error message

Consider the following program:

   class Alpha {};

When compiled it will give the following reply:

<output>
   error-limit.cpp:2:7: error: redefinition of 'Alpha'
   class Alpha {};
         ^
   error-limit.cpp:1:7: note: previous definition is here
   class Alpha {};
         ^
   1 error generated.
</output>

The first messages tells us the error, and the second message contains
additional information about the error.

However, when compiled with -ferror-limit=1 we get:

<output>
   error-limit.cpp:2:7: error: redefinition of 'Alpha'
   class Alpha {};
         ^
   fatal error: too many errors emitted, stopping now [-ferror-limit=]

   2 errors generated.
</output>

The additional information about the (first) error is gone.

Please file a bug and/or submit a patch. The -ferror-limit logic is clipping notes when it shouldn't.

  - Doug

Clang emits a fatal error (fatal_too_many_errors) when the error limit is reached and this clips all subsequent diagnostics. Here’s a patch that allows notes to be emitted even after the fatal error. The question is what kind of diagnostics does error-limit filter, everything, only errors, warnings + errors? If it only filters errors then we should also allow warnings to be emitted.

error-limit_clipping_notes.patch (546 Bytes)

@@ -703,7 +703,7 @@

// If a fatal error has already been emitted, silence all subsequent
// diagnostics.

  • if (Diag.FatalErrorOccurred) {
  • if (Diag.FatalErrorOccurred && DiagLevel != DiagnosticIDs::Note) {
    if (DiagLevel >= DiagnosticIDs::Error &&
    Diag.Client->IncludeInDiagnosticCounts()) {
    ++Diag.NumErrors;

This doesn’t look right… it looks like it’ll allow all notes to be printed, even if the warning/error preceding that note was suppressed. What we really want is for the notes corresponding to the last displayed error to be displayed, but subsequent notes should still be suppressed.

  • Doug

You’re right. It seems that this logic is already in place but it doesn’t work and here’s why: The error is emitted and delayed diagnostic is set (fatal error). So when ReportDelayed() is called it emits the fatal error but it still has delayed diagnostic set (itself) which causes another ReportDelayed to be called, and this one defeats the logic.

The fix would be to reset DelayedDiagID before reporting it, like this:

// store the object returned by Report in order to make its destructor run after DelayedDiagID is set to zero

void Diagnostic::ReportDelayed() {
DiagnosticBuilder diagBuilder = Report(DelayedDiagID);
diagBuilder << DelayedDiagArg1 << DelayedDiagArg2;
DelayedDiagID = 0;
}

// reset DelayedDiagID before calling Report, but create the temporary to save the value

void Diagnostic::ReportDelayed() {
int temp = DelayedDiagID;
DelayedDiagID = 0;
Report(temp) << DelayedDiagArg1 << DelayedDiagArg2;
}

// pass DelayedDiagID as the argument (this is passing its own member as an argument)

void Diagnostic::ReportDelayed(int id) {
DelayedDiagID = 0;
Report(id) << DelayedDiagArg1 << DelayedDiagArg2;
}

The first one is the most reasonable one for me. What do you think?

The first one seems reasonable. Can you update your patch with this fix + test cases?

  • Doug

Here is a patch that fixes the problem. I’ll send one more with test cases after the weekend. Is there a place where I can find similar test case?

error-limit_clipping_notes.patch (511 Bytes)

Here is a patch that fixes the problem. I’ll send one more with test cases after the weekend. Is there a place where I can find similar test case?

I don’t see any existing tests for this functionality… I guess that’s how this bug slipped through :frowning:

You should be able to use FileCheck to test this appropriately.

  • Doug

Here’s a test (I’m not sending a patch because I don’t know where to put this file). The output now looks like this:

error-limit.c:4:8: error: redefinition of ‘s1’

struct s1{};

^

fatal error: too many errors emitted, stopping now

error-limit.c:3:8: note: previous definition is here

struct s1{};

^

2 errors generated.

Note comes after the fatal error, but I don’t see an easy way around this. Is this good enough?

I’ve also noticed a difference in how ferror-limit accepts its parameter depending on how clang is invoked (driver or the front-end):

clang -ferrror-limit=1
clang -cc1 -ferror-limit 1 (doesn’t accept ferror-limit= syntax)
Is this intentional?

error-limit.c (424 Bytes)

Here’s a test (I’m not sending a patch because I don’t know where to put this file). The output now looks like this:

error-limit.c:4:8: error: redefinition of ‘s1’

struct s1{};

^

fatal error: too many errors emitted, stopping now

error-limit.c:3:8: note: previous definition is here

struct s1{};

^

2 errors generated.

Note comes after the fatal error, but I don’t see an easy way around this. Is this good enough?

IMO, no. We should get it right. One way to do so would be to emit the fatal error rather than the first error that would exceed the threshold, rather than following the last error that is permitted within the threshold. That’s probably easier in general.

I’ve also noticed a difference in how ferror-limit accepts its parameter depending on how clang is invoked (driver or the front-end):

clang -ferrror-limit=1
clang -cc1 -ferror-limit 1 (doesn’t accept ferror-limit= syntax)
Is this intentional?

The -cc1 interface is not a public interface, so the difference between the two command-line interfaces doesn’t matter.

  • Doug

So lets say our limit is 1 and there is only one error in our translation unit. In this case we wouldn’t get the fatal error?

IMO, no. We should get it right. One way to do so would be to emit the
fatal error rather than the first error that would exceed the threshold,
rather than following the last error that is permitted within the threshold.
That's probably easier in general.

So lets say our limit is 1 and there is only one error in our translation
unit. In this case we wouldn't get the fatal error?

No, I think what he's saying is rather than emitting the fatal error
when you see the <limit> error message (when you see the first error
when limit is 1, in your example) - wait until you see the <limit>+1
error and emit the fatal error then.

That seems more correct anyway - if you asked for an error limit of 3
and you only produced 3 errors you shouldn't get the fatal message
because there was no problem:

Given this:

$ clang++ foo.cpp -ferror-limit=2
foo.cpp:5:15: error: no member named 'stuff' in
      '__gnu_cxx::__normal_iterator<char *, std::basic_string<char> >'
  foo.begin().stuff();
  ~~~~~~~~~~~ ^
1 error generated.

This seems unnecessary:

$ clang++ foo.cpp -ferror-limit=1
foo.cpp:5:15: error: no member named 'stuff' in
      '__gnu_cxx::__normal_iterator<char *, std::basic_string<char> >'
  foo.begin().stuff();
  ~~~~~~~~~~~ ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
2 errors generated.

Too many errors were not emitted - one error was emitted so there's no
problem. If you implement -ferror-limit by waiting for the N+1th error
& outputting the fatal error instead of the N+1th error then you'll
fix this note problem & the above weirdness won't occur. I believe
that's what Doug was suggesting.

- David

IMO, no. We should get it right. One way to do so would be to emit the
fatal error rather than the first error that would exceed the threshold,
rather than following the last error that is permitted within the threshold.
That’s probably easier in general.

So lets say our limit is 1 and there is only one error in our translation
unit. In this case we wouldn’t get the fatal error?

No, I think what he’s saying is rather than emitting the fatal error
when you see the error message (when you see the first error
when limit is 1, in your example) - wait until you see the +1
error and emit the fatal error then.

That seems more correct anyway - if you asked for an error limit of 3
and you only produced 3 errors you shouldn’t get the fatal message
because there was no problem:

Given this:

$ clang++ foo.cpp -ferror-limit=2
foo.cpp:5:15: error: no member named ‘stuff’ in
‘__gnu_cxx::__normal_iterator<char *, std::basic_string >’
foo.begin().stuff();

1 error generated.

This seems unnecessary:

$ clang++ foo.cpp -ferror-limit=1
foo.cpp:5:15: error: no member named 'stuff' in
'__gnu_cxx::__normal_iterator<char *, std::basic_string<char> >'
foo.begin().stuff();
~~~~~~~~~~~ ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
2 errors generated.


Too many errors were not emitted - one error was emitted so there's no
problem. If you implement -ferror-limit by waiting for the N+1th error
& outputting the fatal error instead of the N+1th error then you'll
fix this note problem & the above weirdness won't occur. I believe
that's what Doug was suggesting.

What about errors that produce more than one note (like c++ function overload suggestions)?

What about errors that produce more than one note (like c++ function
overload suggestions)?

I'm not sure why they would be a problem - they currently don't trip
up -ferror-limit (which I assume is counting only error diagnostics
(not notes), but then throwing out all diagnostics once it reaches the
limit)

eg:
$ cat foo.cpp
void foo(int);
void foo(double);
void foo(int, int);

int main() {
  foo();
}
$ clang++ foo.cpp -ferror-limit=2
foo.cpp:6:3: error: no matching function for call to 'foo'
  foo();
  ^~~
foo.cpp:3:6: note: candidate function not viable: requires 2 arguments, but 0
      were provided
void foo(int, int);
     ^
foo.cpp:2:6: note: candidate function not viable: requires 1 argument, but 0
      were provided
void foo(double);
     ^
foo.cpp:1:6: note: candidate function not viable: requires 1 argument, but 0
      were provided
void foo(int);
     ^

doesn't trip up -ferror-limit. It correctly doesn't count the notes as errors.

IMO, no. We should get it right. One way to do so would be to emit the fatal error rather than the first error that would exceed the threshold, rather than following the last error that is permitted within the threshold. That’s probably easier in general.

So lets say our limit is 1 and there is only one error in our translation unit. In this case we wouldn’t get the fatal error?

Correct.

One way to do so would be to emit the fatal
error rather than the first error that would exceed the threshold, rather
than following the last error that is permitted within the threshold. That's
probably easier in general.

Like this? (patch attached, no test cases yet) - I don't know if I
should be doing something better than SetDelayedDiagnostic now that we
don't really need to delay it at all.

Examples:

foo.cpp contains one overload error
foo2.cpp contains two overload errors

Old behavior:

$ clang++ foo.cpp -ferror-limit=1
foo.cpp:4:3: error: no matching function for call to 'foo'
  foo();
  ^~~
fatal error: too many errors emitted, stopping now [-ferror-limit=]
2 errors generated.
$ clang++ foo2.cpp -ferror-limit=1
foo2.cpp:4:3: error: no matching function for call to 'foo'
  foo();
  ^~~
fatal error: too many errors emitted, stopping now [-ferror-limit=]
2 errors generated.

New behavior:

$ clang++ foo.cpp -ferror-limit=1
foo.cpp:4:3: error: no matching function for call to 'foo'
  foo();
  ^~~
foo.cpp:1:6: note: candidate function not viable: requires 1 argument,
but 0 were provided
void foo(int);
     ^
1 error generated.
$ clang++ foo2.cpp -ferror-limit=1
foo2.cpp:4:3: error: no matching function for call to 'foo'
  foo();
  ^~~
foo2.cpp:1:6: note: candidate function not viable: requires 1
argument, but 0 were provided
void foo(int);
     ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
2 errors generated.

error-limit.patch (876 Bytes)

Your patch does the trick (the question about SetDelayedDiagnostic still remains). Here are the test cases:

The first one checks if both error and note are emitted before the fatal error, and that nothing is emitted afterwards
The second one checks that multiple notes are emitted correctly before the fatal error, and that nothing is emitted afterwards

Should I include one more that checks that no fatal error is emitted when the limit is 1 and there is only one error?

There is also a piece of code that allows notes to be attached to the fatal error. I wanted to write a test for this but I’m not sure that this is used anywhere (the only fatal error I can see is fatal_too_many_errors and there are no notes being attached)?

error-limit.c (463 Bytes)

error-limit-multiple-notes.cpp (784 Bytes)

Like this? (patch attached, no test cases yet) - I don’t know if I
should be doing something better than SetDelayedDiagnostic now that we
don’t really need to delay it at all.

Your patch does the trick (the question about SetDelayedDiagnostic still remains). Here are the test cases:

SetDelayedDiagnostic is a reasonable way to deal with this.

The first one checks if both error and note are emitted before the fatal error, and that nothing is emitted afterwards
The second one checks that multiple notes are emitted correctly before the fatal error, and that nothing is emitted afterwards

Should I include one more that checks that no fatal error is emitted when the limit is 1 and there is only one error?

There is also a piece of code that allows notes to be attached to the fatal error. I wanted to write a test for this but I’m not sure that this is used anywhere (the only fatal error I can see is fatal_too_many_errors and there are no notes being attached)?

These tests look great to me; I’ve committed them, along with David’s patch, as r137851.

  • Doug