[PATCH] Avoid a "diagnostic already in flight" assertion in diagnoseOdrViolations()

[Richard Smith]

On Mon, Aug 4, 2014 at 9:03 PM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> On Aug 4, 2014, at 5:26 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Mon, Aug 4, 2014 at 3:03 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>
>> Hi Richard,
>>
>> We have several lazy builtin Decls (for ObjC decls, va_list, etc.) that
>> might get filled in when we desugar a type for the ODR diagnostics, and
>> these may deserialize more content from a module when they lookup an
>> IdentifierInfo, leading to trying to emit a diagnostic while a diagnostic
>> is already in flight.
>
>
> Hmm, thinking more about the specific problem here, I wonder if even the
> approaches we've discussed so far are not enough. In particular, suppose we
> hit this diagnostic:
>
> Diag(...) << D->somethingThatTriggersDeserialization()
>
> In this case, we'll again assert when deserialization issues a diagnostic
> with another diagnostic in flight. Some quick grepping was unable to find
> such a case, but I feel uneasy about this possibility; this seems like a
> time bomb. Maybe ASTReader should buffer all of its diagnostics, and emit
> them at some known-safe point in time, such as end of TU. What do you think?
>
>
> It looks like a lot of ASTReader is sort of doing that already by using
> the 1-item buffer from Diags.setDelayedDiagnostic() and just discarding any
> diagnostics after the first one.  The “safe point” in this case is
> immediately when there is no in-flight diag, or whenever the in-flight
> diagnostic is emitted if there is.
>

This is an incorrect approach in the general case: it may emit diagnostics
between an error or warning and its notes. It's fine for fatal errors,
though.

That being said, I don’t know that we necessarily *want* non-fatal errors
> to be emitted if they are encountered while a diagnostic is in flight.
>

I'm certain we do. It doesn't make sense to suppress an error from the
serialization system merely because we first detected it while assembling a
warning message, say. I think there are a few reasonable ways we can
produce diagnostics from the serialization system:

1) Buffer the diagnostic and its notes, and produce it when we're at a
point where we know we're not mid-diagnostic. (This doesn't need to be end
of TU, we could also insert safe points at various locations in the parser,
or immediately before we begin emission of any non-note diagnostic.)

2) Set aside the current diagnostic, if there is one, produce the
serialization diagnostic, then restore the set-aside data once we're done.
(If the diagnostic we set aside is a note, inform the diagnostics system
that it should suppress that and any future notes for this diagnostic.)
This may interleave a serialization diagnostic inside another diagnostic,
so we should probably only consider this for diagnostics that are so
important that they should not be delayed (that is, because we know future
diagnostics will be hopeless), and it should be marked as DefaultFatal so
we suppress those future diagnostics.

3) Use the DelayedDiagnostic system; this has the same restrictions as
option 2 but gives a slightly different (arguably worse) diagnostic order.

Maybe there are other approaches that work out OK, but it's not acceptable
to just drop diagnostics.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140805/3b3e0a6d/attachment.html>