<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 4, 2014, at 4:17 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">On Mon, Aug 4, 2014 at 3:03 PM, Ben Langmuir <span dir="ltr" class=""><<a href="mailto:blangmuir@apple.com" target="_blank" class="">blangmuir@apple.com</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Richard,<br class="">
<br class="">
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. My patch fixes the specific issue I ran into with the ODR diagnostics, but it seems like there may be a more general problem that filling in these lazy builtins may not be safe during diagnostic printing. Any thoughts?</blockquote>
<div class=""><br class=""></div><div class="">Your patch LGTM.</div><div class=""><br class=""></div><div class="">I don't think this is specific to filling in lazy builtins: any AST deserialization triggered by diagnoseOdrViolations seems to hit this issue. This used to be protected from this particular issue by occurring within a 'Deserializing' RAII region; I moved it out of that region because it depends indirectly on AST invariants being maintained, so it seemed better to keep it outside the 'danger zone' of finishPendingActions.</div>
<div class=""><br class=""></div><div class="">We could perhaps make this more robust by adding a flag indicating whether we're already doing diagnoseOdrViolations, and skipping it if it reentrantly triggers itself, but your change looks like it would have the same effect in practice.</div></div></div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">
<div class=""><br class=""></div><div class="">FWIW, the PassInterestingDeclsToConsumer call in FinishedDeserializing has a similar, but worse, issue: it calls back into CodeGen, and may be triggered by CodeGen inspecting the AST.</div></div></div>
</div>
</div></blockquote></div><div class=""><br class=""></div><div class="">Would it make more sense for me to just hoist+rename the guard for PassInterestingDeclsToConsumer up and protect every function that we call here? I.e.</div><div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(79, 129, 135);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #bb2ca2" class="">if</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> (</span>NumCurrentElementsDeserializing<span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> == </span><span style="font-variant-ligatures: no-common-ligatures; color: #272ad8" class="">0</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class="">) {</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(79, 129, 135);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class="">//==> move recursion guard here</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(49, 89, 93);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span>diagnoseOdrViolations<span style="font-variant-ligatures: no-common-ligatures; color: #000000" class="">();</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo; min-height: 15px;" class=""><br class=""></div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span>// We are not in recursive loading, so it's safe to pass the "interesting"</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span>// decls to the consumer.</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(79, 129, 135);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #bb2ca2" class="">if</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> (</span>Consumer<span style="font-variant-ligatures: no-common-ligatures; color: #000000" class="">)</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(49, 89, 93);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span>PassInterestingDeclsToConsumer<span style="font-variant-ligatures: no-common-ligatures; color: #000000" class="">();</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> }</div></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""><br class=""></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""><br class=""></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class="">I don’t see any reason to allow diagnosing ODR violations while we are passing interesting decls to the consumer, or vice-versa.</div></body></html>