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

[Ben Langmuir]

> On Aug 4, 2014, at 4:17 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 
> On Mon, Aug 4, 2014 at 3:03 PM, Ben Langmuir <blangmuir at apple.com <mailto: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.  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?
> 
> Your patch LGTM.
> 
> 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.
> 
> 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.
> 
> 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.


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.

  if (NumCurrentElementsDeserializing == 0) {
//==> move recursion guard here
    diagnoseOdrViolations();

    // We are not in recursive loading, so it's safe to pass the "interesting"
    // decls to the consumer.
    if (Consumer)
      PassInterestingDeclsToConsumer();
  }


I don’t see any reason to allow diagnosing ODR violations while we are passing interesting decls to the consumer, or vice-versa.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140804/71bd61d1/attachment.html>