[clang][timers][modules] Fix a timer being started when it's running #154231
Conversation
`ASTReader::FinishedDeserializing()` calls `adjustDeductedFunctionResultType(...)` [0], which in turn calls `FunctionDecl::getMostRecentDecl()`[1]. In modules builds, `getMostRecentDecl()` may reach out to the `ASTReader` and start deserializing again. Starting deserialization starts `ReadTimer`; however, `FinishedDeserializing()` doesn't call `stopTimer()` until after it's call to `adjustDeductedFunctionResultType(...)` [2]. As a result, we hit an assert checking that we don't start an already started timer [3]. To fix this, we simply don't start the timer if it's already running. Unfortunately I don't have a test case for this yet as modules builds are notoriously difficult to reduce. [0]: https://github.com/llvm/llvm-project/blob/4d2288d31866013bd361800df4746fbc2fcf742a/clang/lib/Serialization/ASTReader.cpp#L11053 [1]: https://github.com/llvm/llvm-project/blob/4d2288d31866013bd361800df4746fbc2fcf742a/clang/lib/AST/ASTContext.cpp#L3804 [2]: https://github.com/llvm/llvm-project/blob/4d2288d31866013bd361800df4746fbc2fcf742a/clang/lib/Serialization/ASTReader.cpp#L11065-L11066 [3]: https://github.com/llvm/llvm-project/blob/4d2288d31866013bd361800df4746fbc2fcf742a/llvm/lib/Support/Timer.cpp#L150
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Alan Zhao (alanzhao1) Changes
Unfortunately I don't have a test case for this yet as modules builds are notoriously difficult to reduce. Full diff: https://github.com/llvm/llvm-project/pull/154231.diff 1 Files Affected:
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index e8dddda584a9b..bff289f1c1b76 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -11003,7 +11003,8 @@ void ASTReader::diagnoseOdrViolations() {
}
void ASTReader::StartedDeserializing() {
- if (++NumCurrentElementsDeserializing == 1 && ReadTimer.get())
+ if (Timer *T = ReadTimer.get();
+ ++NumCurrentElementsDeserializing == 1 && T && !T->isRunning())
ReadTimer->startTimer();
}
|
ilya-biryukov
left a comment
ilya-biryukov
left a comment
There was a problem hiding this comment.
LGTM! Thanks for trying the alternative approach and confirming it is not as easy as I anticipated.
The change solves a real assertion failure we saw in our codebase and is an improvement, thanks a lot!
See the rationale for the stylistic comment I made too. Happy for you to go either way on it, don't feel compelled to address it before landing if you disagree.
5 participants
ASTReader::FinishedDeserializing()callsadjustDeductedFunctionResultType(...)0, which in turn callsFunctionDecl::getMostRecentDecl()1. In modules builds,getMostRecentDecl()may reach out to theASTReaderand start deserializing again. Starting deserialization startsReadTimer; however,FinishedDeserializing()doesn't callstopTimer()until after it's call toadjustDeductedFunctionResultType(...)2. As a result, we hit an assert checking that we don't start an already started timer 3. To fix this, we simply don't start the timer if it's already running.Unfortunately I don't have a test case for this yet as modules builds are notoriously difficult to reduce.