llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Michael Park (mpark) <details> <summary>Changes</summary> This is fix for an unreachable code path being reached, for an internal use case at Meta. I'm unfortunately still not able to reproduce a minimal example that I can share 😞 # Description There is a defaulted constructor `_Hashtable_alloc() = default;` which ends up in [`CodeGenFunction::GenerateCode`](https://github.com/llvm/llvm-project/blob/48bf0a9457fd60d0872d9b9b4804a95c833a72e1/clang/lib/CodeGen/CodeGenFunction.cpp#L1448) and eventually calls [`FunctionProtoType::canThrow`](https://github.com/llvm/llvm-project/blob/48bf0a9457fd60d0872d9b9b4804a95c833a72e1/clang/lib/AST/Type.cpp#L3758) with `EST_Unevaluated`. In the "good" cases I observed, I see that the decl is also loaded with the `noexcept-unevaluated` state, but it gets fixed up later by a [call to `adjustExceptionSpec`](https://github.com/llvm/llvm-project/blob/48bf0a9457fd60d0872d9b9b4804a95c833a72e1/clang/lib/Serialization/ASTReader.cpp#L10656). The update gets [added to `PendingExceptionSpecUpdates` here](https://github.com/llvm/llvm-project/blob/48bf0a9457fd60d0872d9b9b4804a95c833a72e1/clang/lib/Serialization/ASTReaderDecl.cpp#L4749-L4752). In the "bad" case, the update does not get added to `PendingExceptionSpecUpdates` and hence the call to `adjustExceptionSpec` also does not occur. # Observations I made two observations in Clang Discord: [[1](https://discord.com/channels/636084430946959380/636732781086638081/1317290104431185921)] and [[2](https://discord.com/channels/636084430946959380/636732781086638081/1317291980413206608)]. 1. [FinishedDeserializating](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10641) can exit with stuff still in `PendingIncompleteDeclChains`. `FinishedDeserializing` [calls `finishPendingActions` only if `NumCurrentElementsDeserializing == 1`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10647). In [`finishPendingActions`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10028), it tries to [clear out `PendingIncompleteDeclChains`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10082-L10087). This is done in a loop, which is fine. But, later in `finishPendingActions`, it calls things like `hasBody` [here](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10296). These can call `getMostRecentDecl` / `getNextRedeclaration` that ultimately calls [`CompleteDeclChain`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L7719). `CompleteDeclChain` is "called each time we need the most recent declaration of a declaration after the generation count is incremented." according to [this comment](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/ExternalASTSource.h#L213-L215). Anyway, since we're still in `finishPendingActions` with `NumCurrentElementsDeserializing == 1`, calls to `CompleteDeclChain` simply [adds more stuff to `PendingIncompleteDeclChains`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L7725). ~~I think the emptying out of `PendingIncompleteDeclChains` should actually happen in `FinishedDeserializing`, in [this loop](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10657-L10658) instead.~~ 2. `LazyGenerationalUpdatePtr::get` seems a bit dubious. In the `LazyData` case, it does [the following](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/ExternalASTSource.h#L482-L486): ```cpp if (LazyVal->LastGeneration != LazyVal->ExternalSource->getGeneration()) { LazyVal->LastGeneration = LazyVal->ExternalSource->getGeneration(); (LazyVal->ExternalSource->*Update)(O); } return LazyVal->LastValue; ``` so for example, after `markIncomplete`, `LastGeneration` gets set to `0` to force the update. For example, `LazyVal->LastGeneration == 0` and `LazyVal->ExternalSource->getGeneration() == 6`. The `Update` function pointer calls `CompleteDeclChain`, which, if we're in the middle of deserialization, do nothing and just add the decl to `PendingIncompleteDeclChains`. So the update was not completed, but the `LastGeneration` is updated to `6` now... that seems potentially problematic, since subsequent calls will simply return `LazyVal->LastValue` since the generation numbers match now. I would've maybe expected something like: ``` if (LazyVal->LastGeneration != LazyVal->ExternalSource->getGeneration() && (LazyVal->ExternalSource->*Update)(O)) { LazyVal->LastGeneration = LazyVal->ExternalSource->getGeneration(); } return LazyVal->LastValue; ``` where the generation is updated once the intended update actually happens. # Solution The proposed solution is to perform the marking of incomplete decl chains at the end of `finishPendingActions`. We know that calls such as `hasBody` can add entries to `PendingIncompleteDeclChains` and change the generation counter without actually performing the update. By clearing out the `PendingIncompleteDeclChains` at the end of `finishPendingActions`, we reset the generation counter to force reload post-`finishPendingActions`. It's also safe to delay this operation since any operation being done within `finishPendingActions` has `NumCurrentElementsDeserializing == 1`, which means that any calls to `CompleteDeclChain` would simply add to the `PendingIncompleteDeclChains` without doing anything anyway. --- Full diff: https://github.com/llvm/llvm-project/pull/121245.diff 1 Files Affected: - (modified) clang/lib/Serialization/ASTReader.cpp (+13-13) ``````````diff diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index ec85fad3389a1c..842d00951a2675 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -9807,12 +9807,12 @@ void ASTReader::visitTopLevelModuleMaps( } void ASTReader::finishPendingActions() { - while ( - !PendingIdentifierInfos.empty() || !PendingDeducedFunctionTypes.empty() || - !PendingDeducedVarTypes.empty() || !PendingIncompleteDeclChains.empty() || - !PendingDeclChains.empty() || !PendingMacroIDs.empty() || - !PendingDeclContextInfos.empty() || !PendingUpdateRecords.empty() || - !PendingObjCExtensionIvarRedeclarations.empty()) { + while (!PendingIdentifierInfos.empty() || + !PendingDeducedFunctionTypes.empty() || + !PendingDeducedVarTypes.empty() || !PendingDeclChains.empty() || + !PendingMacroIDs.empty() || !PendingDeclContextInfos.empty() || + !PendingUpdateRecords.empty() || + !PendingObjCExtensionIvarRedeclarations.empty()) { // If any identifiers with corresponding top-level declarations have // been loaded, load those declarations now. using TopLevelDeclsMap = @@ -9860,13 +9860,6 @@ void ASTReader::finishPendingActions() { } PendingDeducedVarTypes.clear(); - // For each decl chain that we wanted to complete while deserializing, mark - // it as "still needs to be completed". - for (unsigned I = 0; I != PendingIncompleteDeclChains.size(); ++I) { - markIncompleteDeclChain(PendingIncompleteDeclChains[I]); - } - PendingIncompleteDeclChains.clear(); - // Load pending declaration chains. for (unsigned I = 0; I != PendingDeclChains.size(); ++I) loadPendingDeclChain(PendingDeclChains[I].first, @@ -10117,6 +10110,13 @@ void ASTReader::finishPendingActions() { for (auto *ND : PendingMergedDefinitionsToDeduplicate) getContext().deduplicateMergedDefinitonsFor(ND); PendingMergedDefinitionsToDeduplicate.clear(); + + // For each decl chain that we wanted to complete while deserializing, mark + // it as "still needs to be completed". + for (unsigned I = 0; I != PendingIncompleteDeclChains.size(); ++I) { + markIncompleteDeclChain(PendingIncompleteDeclChains[I]); + } + PendingIncompleteDeclChains.clear(); } void ASTReader::diagnoseOdrViolations() { `````````` </details> https://github.com/llvm/llvm-project/pull/121245 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
