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

[Richard Smith]

On Tue, Aug 5, 2014 at 2:51 PM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> On Aug 5, 2014, at 1:08 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> 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.
>
>
> Ah, good point.
>
>
> 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.
>
>
> Okay.
>
> 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.)
>
>
> It feels really messy to have to spread this outside the DiagnosticsEngine
> and ASTReader.  That said, this feels like the best idea we have so far.
>

We can probably limit the safe points to just before a non-note diagnostic
and end-of-TU, and avoid touching code outside DiagnosticsEngine and
ASTReader that way.

> 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.
>
>
> Is this safe in general?  We don’t know the state of the partial
> diagnostic we would need to move aside.
>

I think the most risky part would be re-entering the diagnostic consumer
when it's half way through rendering a diagnostic (in the case where
rendering a type is what triggers deserialization).

> 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.
>
>
> A quick-and-dirty fix would be to upgrade the deserialization diagnostic
> seen while a diagnostic is in-flight to fatal and use the existing 1-item
> buffer.  That would be better than asserting for the time being.  I don’t
> really have any strong opinion about
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140805/aa506a9d/attachment.html>