From 9f07859077fb6df79388d1cc346f3e57f2832f7c Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Mon, 18 Aug 2025 16:46:48 -0700 Subject: [PATCH 1/4] [clang][timers][modules] Fix a timer being started when it's running `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 --- clang/lib/Serialization/ASTReader.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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(); } From 686c6c3d28e945fc56a0fbd565b5b9a21f99dcd5 Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Mon, 18 Aug 2025 17:22:44 -0700 Subject: [PATCH 2/4] fix namespace identifier issue --- clang/lib/Serialization/ASTReader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index bff289f1c1b76..a61238fb7698d 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -11003,7 +11003,7 @@ void ASTReader::diagnoseOdrViolations() { } void ASTReader::StartedDeserializing() { - if (Timer *T = ReadTimer.get(); + if (llvm::Timer *T = ReadTimer.get(); ++NumCurrentElementsDeserializing == 1 && T && !T->isRunning()) ReadTimer->startTimer(); } From b45ce002b10987c0a6c8d8bda15f572c58c75e9d Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Mon, 18 Aug 2025 21:47:07 -0700 Subject: [PATCH 3/4] use T instead of ReadTimer --- clang/lib/Serialization/ASTReader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index a61238fb7698d..51222956ae0e1 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -11005,7 +11005,7 @@ void ASTReader::diagnoseOdrViolations() { void ASTReader::StartedDeserializing() { if (llvm::Timer *T = ReadTimer.get(); ++NumCurrentElementsDeserializing == 1 && T && !T->isRunning()) - ReadTimer->startTimer(); + T->startTimer(); } void ASTReader::FinishedDeserializing() { From 6b03a89dd500ba9b31cfb25d87d4fbc58c05de99 Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Wed, 20 Aug 2025 14:05:07 -0700 Subject: [PATCH 4/4] use std::optional to enforce ReadTimer --- clang/include/clang/Serialization/ASTReader.h | 3 +++ clang/lib/Serialization/ASTReader.cpp | 7 +++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 7ce626cedae74..3ede0925676e8 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -526,6 +526,9 @@ class ASTReader /// A timer used to track the time spent deserializing. std::unique_ptr ReadTimer; + // A TimeRegion used to start and stop ReadTimer via RAII. + std::optional ReadTimeRegion; + /// The location where the module file will be considered as /// imported from. For non-module AST types it should be invalid. SourceLocation CurrentImportLoc; diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 51222956ae0e1..23ba26c484ad7 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -11004,8 +11004,8 @@ void ASTReader::diagnoseOdrViolations() { void ASTReader::StartedDeserializing() { if (llvm::Timer *T = ReadTimer.get(); - ++NumCurrentElementsDeserializing == 1 && T && !T->isRunning()) - T->startTimer(); + ++NumCurrentElementsDeserializing == 1 && T) + ReadTimeRegion.emplace(T); } void ASTReader::FinishedDeserializing() { @@ -11063,8 +11063,7 @@ void ASTReader::FinishedDeserializing() { (void)UndeducedFD->getMostRecentDecl(); } - if (ReadTimer) - ReadTimer->stopTimer(); + ReadTimeRegion.reset(); diagnoseOdrViolations(); }