Page MenuHomePhabricator

Make diagnostic reporting more robust in presence of corrupt PCH data.
Needs ReviewPublic

Authored by neilmacintosh on Feb 18 2020, 2:35 PM.

Details

Summary

When running clangd, I found that occasionally, accessing a
precompiled header would cause an LLVM assertion to fire.

This in turn would cause a diagnostic to try and be emitted,
when the diagnostics machinery would reach back to find
information from the PCH, it would hit the same problem
that caused the initial assertion. Another assertion would fire and
another delayed diagnostic would be setup to report, and so on.

This recursive sequence would continue until stack overflow, which
would take down clangd.

This small patch works around that possibility by making a delayed
diagnostic more explicit (and therefore testable to avoid recursion).

I tested this against the files in our codebase that hit the
LLVM assertion with clangd and now the assertion clearly reports
and clangd is no longer exhausted by a stack overflow.

Tracking down the underlying problem (hitting the assertion)
is a separate task.

I'm not sure how (or if it's possible) to add a test that would
demonstrate the before/after here. Happy to take advice!

Event Timeline

neilmacintosh created this revision.Feb 18 2020, 2:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2020, 2:35 PM

Thanks for working on this! I have a couple of comments inline.

clang/include/clang/Basic/Diagnostic.h
918–927

CurDelayedDiagID has a different kind of "in-flight" lifetime than CurDiagID. I think the comment here should be more explicit about what precisely the variable is doing, and perhaps it should have another name. Do we actually need the ID kept as state? If not, this would be simpler as:

bool IsReportingDelayedDiag = false;

Also, a couple of nit-picks:

  • Periods at the end of sentences in comments.
  • You seem to have dropped out of doxygen comments (// instead of ///).
clang/lib/Basic/Diagnostic.cpp
160–161

If you take my suggestion, then you wouldn't need to change this line, just add:

IsReportingDelayedDiag = true;

but would these assertions be valid after your change?

assert(DelayedDiagID && "Called without a delayed diagnostic?");
assert(!IsReportingDelayedDiag &&
       "Called while reporting another delayed diagnostic?");
bruno added a subscriber: bruno.Feb 19 2020, 3:21 PM

Thanks for working on this.

I'm not sure how (or if it's possible) to add a test that would demonstrate the before/after here. Happy to take advice!

Yea, those are some times hard. Did you try to write a unittest that hits the assertion and trigger this case? Perhaps clang/unittests/Frontend/PCHPreambleTest.cpp would be a good example to start off.