<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Aug 4, 2014 at 9:03 PM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="h5"><br><div><blockquote type="cite"><div>On Aug 4, 2014, at 5:26 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div>
<br><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Aug 4, 2014 at 3:03 PM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Richard,<br>
<br>
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.</blockquote>

<div><br></div><div>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:</div><div><br></div><div>Diag(...) << D->somethingThatTriggersDeserialization()</div>

<div><br></div><div>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?</div>

</div></div></div>
</div></blockquote></div><br></div></div><div>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.</div>
</div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>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.</div></div></blockquote><div><br></div>
<div>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:</div>
<div><br></div><div>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.)</div>
<div><br></div><div>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.</div>
<div><br></div><div>3) Use the DelayedDiagnostic system; this has the same restrictions as option 2 but gives a slightly different (arguably worse) diagnostic order.</div><div><br></div><div>Maybe there are other approaches that work out OK, but it's not acceptable to just drop diagnostics.</div>
</div></div></div>