[C++20][Modules][Serialization] Delay marking pending incomplete decl chains until the end of finishPendingActions.
#121245
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Michael Park (mpark) ChangesThis 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 😞 DescriptionThere is a defaulted constructor In the "good" cases I observed, I see that the decl is also loaded with the In the "bad" case, the update does not get added to ObservationsI made two observations in Clang Discord: [1] and [2].
if (LazyVal->LastGeneration != LazyVal->ExternalSource->getGeneration()) {
LazyVal->LastGeneration = LazyVal->ExternalSource->getGeneration();
(LazyVal->ExternalSource->*Update)(O);
}
return LazyVal->LastValue;so for example, after where the generation is updated once the intended update actually happens. SolutionThe proposed solution is to perform the marking of incomplete decl chains at the end of Full diff: https://github.com/llvm/llvm-project/pull/121245.diff 1 Files Affected:
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() {
|
|
Did you try creduce/cvise (although using that with modules might be a bit challenging) |
I did. It did help me reduce it significantly, but it still involves a few modules that are difficult to reduce. I may be able to make progress by running it on the modules themselves though? I'm not sure. I agree it'd be really nice to have a test case. |
|
+1. It is best to have more test case. Generally I reduce it by hand in this case. |
ChuanqiXu9
changed the title
finishPendingActions.finishPendingActions.
Yeah, I did already spent quite a bit of effort trying to reduce the creduce result by hand but have not been successful so far 😕. I'll try again and report back. Would you still review the logic / whether the suggested solution seems sensible? |
The solution looks neat to me. |
|
Can you reduce your modules test case into a self contained project with build system? If so, you can use cvise for reducing a whole project directory recursively in one invocation. |
I didn't know this before. It sounds great. If you'd like, would you like to write a simple example document to explain how users do in https://clang.llvm.org/docs/StandardCPlusPlusModules.html? I believe it will be pretty helpful. |
mpark
force-pushed
the
delay-mark-incomplete-decl-chains
branch
from
b892631 to
9879eb1
Compare
mpark
force-pushed
the
delay-mark-incomplete-decl-chains
branch
from
9879eb1 to
e6c5d9a
Compare
|
Okay folks, I've finally managed to get a reasonable repro as a unit test! The new unit test built in |
mpark
force-pushed
the
delay-mark-incomplete-decl-chains
branch
from
e6c5d9a to
2a5a87e
Compare
mpark
changed the title
finishPendingActions.finishPendingActions.
cor3ntin
left a comment
cor3ntin
left a comment
There was a problem hiding this comment.
Thanks for the test!
I'll let @ChuanqiXu9 approve.
Presumably, we want to cherry pick that in clang 20?
Yeah, I'd think so if it's not too late. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
dmpolukhin
left a comment
dmpolukhin
left a comment
There was a problem hiding this comment.
LGTM but let @ChuanqiXu9 approve.
clang/test/Modules/pr121245.cpp
Outdated
There was a problem hiding this comment.
Please add a comment that the test results in llvm_unreachable in debug builds so debug build should be used to investigate problems with the test.
… chains until the end of `finishPendingActions`.
Co-authored-by: cor3ntin <[email protected]>
mpark
force-pushed
the
delay-mark-incomplete-decl-chains
branch
from
d48f483 to
3e13c63
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/15158 Here is the relevant piece of the build log for the reference |
|
/cherry-pick a9e249f |
|
Failed to create pull request for issue121245 https://github.com/llvm/llvm-project/actions/runs/13206254625 |
|
/cherry-pick a9e249f |
|
/pull-request #126289 |
… chains until the end of `finishPendingActions`. (llvm#121245) The call to `hasBody` inside `finishPendingActions` that bumps the `PendingIncompleteDeclChains` size from `0` to `1`, and also sets the `LazyVal->LastGeneration` to `6` which matches the `LazyVal->ExternalSource->getGeneration()` value of `6`. Later, the iterations over `redecls()` (which calls `getNextRedeclaration`) is expected to trigger the reload, but it **does not** since the generation numbers match. The proposed solution is to perform the marking of incomplete decl chains at the end of `finishPendingActions`. This way, **all** of the incomplete decls are marked incomplete as a post-condition of `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. (cherry picked from commit a9e249f)
… chains until the end of `finishPendingActions`. (llvm#121245) The call to `hasBody` inside `finishPendingActions` that bumps the `PendingIncompleteDeclChains` size from `0` to `1`, and also sets the `LazyVal->LastGeneration` to `6` which matches the `LazyVal->ExternalSource->getGeneration()` value of `6`. Later, the iterations over `redecls()` (which calls `getNextRedeclaration`) is expected to trigger the reload, but it **does not** since the generation numbers match. The proposed solution is to perform the marking of incomplete decl chains at the end of `finishPendingActions`. This way, **all** of the incomplete decls are marked incomplete as a post-condition of `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.
|
Hi @mpark ! Unfortunately with this change we hit a crash in our bootstrap build with modules on macOS: Do you mind taking a look? Or temporarily revert this change. |
|
Hi @zixu-w, thanks for the ping. I can't take a look this week, but can on Monday. It would be great if I can look at a small repro though. I'll follow on the issue. |
Thanks! Yes, I'll try to construct a smaller and isolated reproducer. Is it okay if we revert this change for now in upstream to unblock us? |
…ete decl chains until the end of `finishPendingActions`. (llvm#121245)" This reverts commit a9e249f. Reverting this change because of issue llvm#126973.
|
@zixu-w and @ChuanqiXu9 Just want to clarify policies about changes reversion. As far as I understand it was one example in some private codebase with no reproducer publicly available. I can understand and completely agree if it breaks any existing test or llvm-build bot. But in this case I think it would be good to at least wait for the review. Such reversion without providing a reproducer does not allow to fix the issues or even verify that it is related to this PR. This PR fixes another crash with clear reproducers. Sorry, didn't notice that it is LLVM bootstrap build but still, no still I don't see steps to reproduce in #126973. |
CC @AaronBallman for the policy related things. Personally I do agree it is frustrating if the patch gets reverted without being able to reproduce it. And my point in the above post is, if we revert it in the trunk and it was backported to the release branch, we should revert it in the release branch too. My point is majorly the if. |
Our documented policy is at https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy The promise for a public reproducer is in line with our policy, but reverting a change after it's been in the tree for two weeks is not particularly timely (but not unreasonable either, sometimes it takes a while to churn through downstream team suites). However, it's not clear to me why the revert couldn't have been handled in the downstream given the lack of details on the problem and the fact that upstream does not seem to be obviously broken. For example, the issue could ultimately boil down to interactions with changes carried in the downstream. That said, I don't think anyone's doing anything wrong (certainly nobody is acting in bad faith), and there was more than a day for reviewers or the patch author to have questioned the revert before the revert was committed.
I think the rule of thumb should be to revert in the release branch if the revert happens in trunk. CC @tstellar @tru in case they have a different opinion. |
|
Hi @dmpolukhin @ChuanqiXu9 @AaronBallman ! My apologies for the late reversion and not providing more context more promptly. To clarify, the issue does also reproduce in upstream and I've provided reproducing steps in #126973 (comment). It was only caught in our downstream builds because we don't have the exact configuration to reproduce the problem in upstream builds and CIs. And our downstream CI had been running into other issues in the past two weeks, which masked the problem and slowed us down in triaging. It also took me a while to understand the assertion and find which commit caused it. Sorry again for the confusion. I can pick the reversion in the release branch. I'll help follow up with investigating and fixing #126973, and let me know if there's anything else I should do. |
llvm#127136) …ete decl chains until the end of `finishPendingActions`. (llvm#121245)" This reverts commit a9e249f. Reverting this change because of issue llvm#126973.
llvm#127136) …ete decl chains until the end of `finishPendingActions`. (llvm#121245)" This reverts commit a9e249f. Reverting this change because of issue llvm#126973.
9 participants
Description
There is a defaulted constructor
_Hashtable_alloc() = default;which ends up inCodeGenFunction::GenerateCodeand eventually callsFunctionProtoType::canThrowwithEST_Unevaluated.In the "good" cases I observed, I see that the decl is also loaded with the
noexcept-unevaluatedstate, but it gets fixed up later by a call toadjustExceptionSpec. The update gets added toPendingExceptionSpecUpdateshere.In the "bad" case, the update does not get added to
PendingExceptionSpecUpdatesand hence the call toadjustExceptionSpecalso does not occur.Observations
I made two observations in Clang Discord: [1] and [2].
PendingIncompleteDeclChains.FinishedDeserializingcallsfinishPendingActionsonly ifNumCurrentElementsDeserializing == 1. InfinishPendingActions, it tries to clear outPendingIncompleteDeclChains. This is done in a loop, which is fine. But, later infinishPendingActions, it calls things likehasBodyhere. These can callgetMostRecentDecl/getNextRedeclarationthat ultimately callsCompleteDeclChain.CompleteDeclChainis "called each time we need the most recent declaration of a declaration after the generation count is incremented." according to this comment. Anyway, since we're still infinishPendingActionswithNumCurrentElementsDeserializing == 1, calls toCompleteDeclChainsimply adds more stuff toPendingIncompleteDeclChains.I think the emptying out of(The proposed solution is to do this at the end ofPendingIncompleteDeclChainsshould actually happen inFinishedDeserializing, in this loop instead.finishPendingActionsinstead.LazyGenerationalUpdatePtr::getseems a bit dubious. In theLazyDatacase, it does the following:so for example, after
markIncomplete,LastGenerationgets set to0to force the update. For example,LazyVal->LastGeneration == 0andLazyVal->ExternalSource->getGeneration() == 6. TheUpdatefunction pointer callsCompleteDeclChain, which, if we're in the middle of deserialization, do nothing and just add the decl toPendingIncompleteDeclChains. So the update was not completed, but theLastGenerationis updated to6now... that seems potentially problematic, since subsequent calls will simply returnLazyVal->LastValuesince the generation numbers match now. I would've maybe expected something like:where the generation is updated once the intended update actually happens.
Solution
I verified that in my case, it is indeed the call to
hasBodyinsidefinishPendingActionsthat bumps thePendingIncompleteDeclChainssize from0to1, and also sets theLazyVal->LastGenerationto6which matches theLazyVal->ExternalSource->getGeneration()value of6. Later, the iterations overredecls()(which callsgetNextRedeclaration) is expected to trigger the reload, but it does not since the generation numbers match.The proposed solution is to perform the marking of incomplete decl chains at the end of
finishPendingActions. This way, all of the incomplete decls are marked incomplete as a post-condition offinishPendingActions. It's also safe to delay this operation since any operation being done withinfinishPendingActionshasNumCurrentElementsDeserializing == 1, which means that any calls toCompleteDeclChainwould simply add to thePendingIncompleteDeclChainswithout doing anything anyway.