<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 4, 2014 at 4:46 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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 4:17 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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. 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><br></div><div>Your patch LGTM.</div><div><br></div><div>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><br></div><div>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"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>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><br></div></div></div><div>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><br>
</div><div><div style="margin:0px;font-size:13px;font-family:Menlo;color:rgb(79,129,135)"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(187,44,162)">if</span><span style="color:rgb(0,0,0)"> (</span>NumCurrentElementsDeserializing<span style="color:rgb(0,0,0)"> == </span><span style="color:rgb(39,42,216)">0</span><span style="color:rgb(0,0,0)">) {</span></div>
<div style="margin:0px;font-size:13px;font-family:Menlo;color:rgb(79,129,135)"><span style="color:rgb(0,0,0)">//==> move recursion guard here</span></div><div style="margin:0px;font-size:13px;font-family:Menlo;color:rgb(49,89,93)">
<span style="color:rgb(0,0,0)"> </span>diagnoseOdrViolations<span style="color:rgb(0,0,0)">();</span></div><div style="margin:0px;font-size:13px;font-family:Menlo;min-height:15px"><br></div><div style="margin:0px;font-size:13px;font-family:Menlo;color:rgb(0,132,0)">
<span style="color:rgb(0,0,0)"> </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)"><span style="color:rgb(0,0,0)"> </span>// decls to the consumer.</div>
<div style="margin:0px;font-size:13px;font-family:Menlo;color:rgb(79,129,135)"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(187,44,162)">if</span><span style="color:rgb(0,0,0)"> (</span>Consumer<span style="color:rgb(0,0,0)">)</span></div>
<div style="margin:0px;font-size:13px;font-family:Menlo;color:rgb(49,89,93)"><span style="color:rgb(0,0,0)"> </span>PassInterestingDeclsToConsumer<span style="color:rgb(0,0,0)">();</span></div><div style="margin:0px;font-size:13px;font-family:Menlo">
}</div></div><div style="margin:0px;font-size:13px;font-family:Menlo"><br></div><div style="margin:0px;font-size:13px;font-family:Menlo"><br></div><div style="margin:0px;font-size:13px;font-family:Menlo">I don’t see any reason to allow diagnosing ODR violations while we are passing interesting decls to the consumer, or vice-versa.</div>
</div></blockquote></div><br></div><div class="gmail_extra">Sure, that seems like a good general safety measure. However, it doesn't completely fix the problem for PassInterestingDeclsToConsumer. We can have:</div><div class="gmail_extra">
<br></div><div class="gmail_extra"> <some call to CodeGen from outside ASTReader></div><div class="gmail_extra"> => <CodeGen emitting something></div><div class="gmail_extra"> => <triggers deserialization></div>
<div class="gmail_extra"> => ASTReader::FinishedDeserializing<br></div><div class="gmail_extra"><div class="gmail_extra"> => PassInterestingDeclsToConsumer</div><div class="gmail_extra">
=> <CodeGen re-entered to emit something else><br></div><div><br></div><div>It seems that either CodeGen needs to protect itself from being re-entered like this, or needs to deal with it happening.</div>
<div><br></div><div>One option would be to never lazily pass interesting decls to the consumer, and instead rely on the consumer asking us if it's reached the end of the TU and not seen a definition of something it wants, but for now it actually seems more straightforward to make CodeGen's ModuleBuilder be reentrant, so that's the path I'm following. I'm happy to accept arguments that we should handle this a different way.</div>
<div><br></div><div>FWIW, this seems to be a general problem with ASTConsumer / ExternalASTSource interactions: ASTConsumer can trigger ExternalASTSource to produce more stuff, and ExternalASTSource can hand more stuff to ASTConsumer, so both sides need to be reentrant.</div>
</div></div>