[C++20][Modules] Prevent premature calls to PassInterestingDeclsToConsumer() within FinishedDeserializing(). by mpark · Pull Request #129982 · llvm/llvm-project (original) (raw)

Alright folks, I've finally figured this out! I'll describe the high-level problem, what's happening specifically in the test case, and the solution.

High-Level Problem

ASTReader::FinishedDeserializing uses NumCurrentElementsDeserializing to keep track of nested Deserializing RAII actions. The FinishedDeserializing only performs actions if it is the top-level Deserializing layer. This works fine in general, but there is a problematic edge case.

If a call to redecls() in FinishedDeserializing performs deserialization, we re-enter FinishedDeserializing while in the middle of the previous FinishedDeserializing call.

It looks something like:

      +-- ... 2 ... --+
      |               |
   +--+       1       +--+     +-----+
   |  ^SD2         FD2^  |     |     |
|--+          0          +-----+-----+-------|
   ^SD1               FD1^     ^SD3  ^FD3 (FD1 still in progress)

The known problematic part of this is that this inner FinishedDeserializing (FD3 in the diagram) can go all the way to PassInterestingDeclsToConsumer, which operates on PotentiallyInterestingDecls data structure which contain decls that should be handled by the FD1 stage.

The other shared data structures are also somewhat concerning at a high-level in that the inner FinishedDeserializing would be handling pending actions that are not "within its scope", but this part is not known to be problematic.

Specifics

We perform a look-up on f for the f(0) call where f is an overloaded function. One of which is void f(const N::S &) {}, where S = BS<Empty<char>>. In finishPendingActions, we get to RD->addedMember(MD); where RD is the ClassTemplateSpecializationDecl of Empty<char> and MD is the destructor of Empty<char>.

Without #121245, the ClassTemplateSpecializationDecl of BS<Empty<char>> and Empty<char> are left in the PendingIncompleteDeclChains data structure as we exit finishPendingActions. We get through the rest of FinishedDeserializing, and gets to PassInterestingDeclsToConsumer. void f(const N::S &) (i.e. void f(const N::__1::BS<Empty<char>> &)) is identified as an interesting decl and gets name mangled. During name-mangling, we call redecls() on N, __1, BS, and Empty. The redecls() on BS<Empty<char>> specifically doesn't do anything, and instead gets marked incomplete later, and happens to be loaded.

With #121245, the ClassTemplateSpecializationDecl of BS<Empty<char>> and Empty<char> get marked incomplete as we exit finishPendingActions. In the second part of FinishedDeserializing, the call to Update.second->redecls() is made where Update.second is the ClassTemplateSpecializationDecl of Empty<char>. This time, since Empty<char> is marked incomplete, redecl chain completion logic is triggered.

Since we're in the NumCurrentElementsDeserializing == 0 part of FinishedDeserializing, we do not merely add to PendingIncompleteDeclChains, but rather get to the DC->lookup(Name) call in ASTReader::CompleteRedeclChain with DC = __1 and Name = "Empty".

The implementation of DC->lookup ensures that the DC is up-to-date by invoking getMostRecentDecl() on DC. This happens recursively for DC = __1, Name = "Empty", DC = N, Name = "__1" and DC = TranslationUnit, Name = "N". So now, FindExternalVisibleDeclsByName is called from here with DC = TranslationUnit and Name = "N".

Note that at this point, we haven't gotten to actually performing FindExternalVisibleDeclsByName with DC = N, Name = "__1" which will populate new declarations/definitions to __1.

There is an instance of Deserializing inside FindExternalVisibleDeclsByName here, end of which of course is an invocation to FinishedDeserializing. This call gets to PassInterestingDeclsToConsumer with void f(const N::S &) inside of it from before, and tries to mangle the name. During name-mangling, we call redecls() on N, __1, BS, and Empty, the same as without #121245. However, this time the redecls() on BS<Empty<char>> actually tries to do stuff since BS<Empty<char>> was marked incomplete coming out of finishPendingActions. What happens next is that we try to complete the redecl chain for BS<Empty<char>> at this point without having completed __1. So BS<Empty<char>> gets its lazy pointer generation updated with incorrect information.

The claim in this fix is that the real problem is the invocation of PassInterestingDeclsToConsumer deep within this recursion. The PotentiallyInterestingDecls data structure ends up being shared down the recursion and it ends up passing outer-level declarations that may not be prepared to be passed.

Solutions

As described in the High-level Problem section, the big picture problem is the recursive nature of FinishedDeserializing where we can perform further deserialization within FinishedDeserializing, and the inner FinishedDeserializing call operates on the shared data structures such as Pending* and PotentiallyInterestingDecls.

Proposed Solution

We already have a guard within PassInterestingDeclsToConsumer because we can end up with recursive deserialization within PassInterestingDeclsToConsumer. The proposed solution is to apply this guard to the portion of FinishedDeserializing that performs further deserialization as well. This ensures that recursive deserialization does not trigger PassInterestingDeclsToConsumer which may operate on entries that are not ready to be passed.

Alternative: Save and restore the PotentiallyInterestingDecls

An alternative approach would be to save the PotentiallyInterestingDecls on the side while recursive deserialization is happening, such that we do invoke PassInterestingDeclsToConsumer, but only with the decls that are within its scope. This approach works, and has been tested. The reason this approach is not taken is because this approach (in principle) should also save other shared data structures on the side as well, and the proposed solution fit in nicer with the existing mechanism.

Alternative: Guard recursive entering of the second part of FinishedDeserializing

This approach adds a new field bool FinishingDeserializing = false; to ASTReader, and guards the NumCurrentElementsDeserializing == 0 portion of the FinishedDeserializing like so:

if (NumCurrentElementsDeserializing == 0) {

• if (FinishingDeserializing)

• return;

• // Guard variable to avoid recursively redoing the process of passing

• // decls to consumer.

• SaveAndRestore GuardFinishingDeserializing(FinishingDeserializing, true);

  while (!PendingExceptionSpecUpdates.empty() || !PendingDeducedTypeUpdates.empty() || !PendingUndeducedFunctionDecls.empty()) { // ... }

  if (ReadTimer) ReadTimer->stopTimer();

  diagnoseOdrViolations();

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

}

This mirrors the current mechanism of the PassingDeclsToConsumer guard. This approach passes the test-suite. However, I don't believe it's quite correct.

Suppose we're at the diagnoseOdrViolations() part in the outer FinishedDeserializing where a redecls() is invoked. We could have a recursive deserialization at that point, and with this approach, the inner FinishedDeserializing would simply return rather than processing the Pending* data structures. We can't just rely on the outer FinishedDeserializing to finish the job.