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

[Richard Smith]

On Mon, Aug 4, 2014 at 4:46 PM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> 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> 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.
>

Sure, that seems like a good general safety measure. However, it doesn't
completely fix the problem for PassInterestingDeclsToConsumer. We can have:

  <some call to CodeGen from outside ASTReader>
     => <CodeGen emitting something>
          => <triggers deserialization>
               => ASTReader::FinishedDeserializing
                    => PassInterestingDeclsToConsumer
                         => <CodeGen re-entered to emit something else>

It seems that either CodeGen needs to protect itself from being re-entered
like this, or needs to deal with it happening.

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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140804/3e353d34/attachment.html>