From 54b244bec5fa42805dcaaabaa0ac992b04d22bbe Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Tue, 8 Oct 2024 15:49:07 -0700 Subject: [PATCH 1/5] Fix David's query system bug Signed-off-by: Danila Fedorin --- .../include/chpl/framework/Context-detail.h | 22 ++++++++++++++++--- frontend/include/chpl/framework/query-impl.h | 2 +- frontend/lib/framework/Context.cpp | 6 +++++ 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/frontend/include/chpl/framework/Context-detail.h b/frontend/include/chpl/framework/Context-detail.h index b88223dfc8ac..877230a56731 100644 --- a/frontend/include/chpl/framework/Context-detail.h +++ b/frontend/include/chpl/framework/Context-detail.h @@ -260,6 +260,20 @@ class QueryMapResultBase { // lastChanged indicates the last revision in which the query result // has changed mutable RevisionNumber lastChanged = -1; + // This field exists to support isQueryRunning. When traversingg the dependencies + // of a query, we may re-run dependency queries to see if their results change. + // Sometimes, these queries will check isQueryRunning. At this time, the + // actual query (this) is not running, but we want to return 'true' to hide + // the details of the query system from the query author. This field is set to + // 'true' when the query is being tested for re-computation, so that + // we can return 'true' when isQueryRunning is called in that case. + // + // Note (Daniel 10/08/2024): there may be a way to combine this field with + // lastChanged somehow, since that's what we use for isQueryRunning while + // the query really is running. However, the semantics of that are nontrivial, + // and that field is used for a lot of things, so for the time being, the + // extra boolean is fine. + mutable bool beingTestedForReuse = false; mutable QueryDependencyVec dependencies; @@ -280,6 +294,7 @@ class QueryMapResultBase { QueryMapResultBase(RevisionNumber lastChecked, RevisionNumber lastChanged, + bool beingTestedForReuse, bool emittedErrors, bool errorsPresentInSelfOrDependencies, std::set recursionErrors, @@ -302,20 +317,21 @@ class QueryMapResult final : public QueryMapResultBase { // * a default-constructed result QueryMapResult(QueryMap* parentQueryMap, std::tuple tupleOfArgs) - : QueryMapResultBase(-1, -1, false, false, {}, parentQueryMap), + : QueryMapResultBase(-1, -1, false, false, false, {}, parentQueryMap), tupleOfArgs(std::move(tupleOfArgs)), result() { } QueryMapResult(RevisionNumber lastChecked, RevisionNumber lastChanged, + bool beingTestedForReuse, bool emittedErrors, bool errorsPresentInSelfOrDependencies, std::set recursionErrors, QueryMap* parentQueryMap, std::tuple tupleOfArgs, ResultType result) - : QueryMapResultBase(lastChecked, lastChanged, emittedErrors, - errorsPresentInSelfOrDependencies, + : QueryMapResultBase(lastChecked, lastChanged, beingTestedForReuse, + emittedErrors, errorsPresentInSelfOrDependencies, std::move(recursionErrors), parentQueryMap), tupleOfArgs(std::move(tupleOfArgs)), result(std::move(result)) { diff --git a/frontend/include/chpl/framework/query-impl.h b/frontend/include/chpl/framework/query-impl.h index 6b5e87449be5..bf6603f3b759 100644 --- a/frontend/include/chpl/framework/query-impl.h +++ b/frontend/include/chpl/framework/query-impl.h @@ -560,7 +560,7 @@ Context::isQueryRunning( return false; } - return search2->lastChecked == -1; + return search2->lastChecked == -1 || search2->beingTestedForReuse; } templatebeingTestedForReuse = true; for (auto& dependency : resultEntry->dependencies) { const QueryMapResultBase* dependencyQuery = dependency.query; if (dependencyQuery->lastChanged > resultEntry->lastChanged) { @@ -1042,6 +1043,7 @@ void Context::recomputeIfNeeded(const QueryMapResultBase* resultEntry) { } } } + resultEntry->beingTestedForReuse = false; if (useSaved == false) { auto marker = markRecomputing(true); @@ -1124,6 +1126,7 @@ bool Context::queryCanUseSavedResult( useSaved = false; } else { useSaved = true; + resultEntry->beingTestedForReuse = true; for (auto& dependency: resultEntry->dependencies) { const QueryMapResultBase* dependencyQuery = dependency.query; @@ -1142,6 +1145,7 @@ bool Context::queryCanUseSavedResult( break; } } + resultEntry->beingTestedForReuse = false; if (useSaved == true) { updateForReuse(resultEntry); } @@ -1339,12 +1343,14 @@ void queryArgsPrintSep(std::ostream& s) { QueryMapResultBase::QueryMapResultBase(RevisionNumber lastChecked, RevisionNumber lastChanged, + bool beingTestedForReuse, bool emittedErrors, bool errorsPresentInSelfOrDependencies, std::set recursionErrors, QueryMapBase* parentQueryMap) : lastChecked(lastChecked), lastChanged(lastChanged), + beingTestedForReuse(beingTestedForReuse), dependencies(), emittedErrors(emittedErrors), errorsPresentInSelfOrDependencies(errorsPresentInSelfOrDependencies), From f96d06cc1bfd9bd0279005e49b1b36ee5fe15206 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Tue, 8 Oct 2024 15:49:25 -0700 Subject: [PATCH 2/5] Add comment and assertion to code that tripped me up Signed-off-by: Danila Fedorin --- frontend/include/chpl/framework/Context.h | 4 ++++ frontend/include/chpl/resolution/ResolutionContext.h | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/frontend/include/chpl/framework/Context.h b/frontend/include/chpl/framework/Context.h index 3d502879d316..33ee4190e2ec 100644 --- a/frontend/include/chpl/framework/Context.h +++ b/frontend/include/chpl/framework/Context.h @@ -227,6 +227,10 @@ class Context { context_ = nullptr; } + bool isCleared() { + return context_ == nullptr; + } + ~RecomputeMarker() { restore(); } diff --git a/frontend/include/chpl/resolution/ResolutionContext.h b/frontend/include/chpl/resolution/ResolutionContext.h index c6ed9b3b74d8..ec832a0b0bd0 100644 --- a/frontend/include/chpl/resolution/ResolutionContext.h +++ b/frontend/include/chpl/resolution/ResolutionContext.h @@ -454,8 +454,14 @@ class ResolutionContext::GlobalQuery { } // Otherwise we are computing, so set the recompute marker. + // We don't want it to go out of scope now (since that undoes the mark), + // but this is just the 'begin' function; we do want it to go out of scope + // when the query is done (when 'end' is called). So, marker it in a field. const bool isRecomputing = false; auto activeRecomputeMarker = context_->markRecomputing(isRecomputing); + + // We better not be saving another marker, since we only have room to save one. + CHPL_ASSERT(recomputeMarker_.isCleared()); std::swap(recomputeMarker_, activeRecomputeMarker); // Set the stopwatch if it is compile-time enabled. From aa6ac54c36f0d82e05a532aacf9ae85ab8cc402f Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Tue, 8 Oct 2024 16:04:35 -0700 Subject: [PATCH 3/5] Add David's reproducer to tests Signed-off-by: Danila Fedorin --- frontend/test/resolution/testResolve.cpp | 44 ++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/frontend/test/resolution/testResolve.cpp b/frontend/test/resolution/testResolve.cpp index 8bc3687b9b1b..83acc0e0c630 100644 --- a/frontend/test/resolution/testResolve.cpp +++ b/frontend/test/resolution/testResolve.cpp @@ -1761,6 +1761,48 @@ static void test29(Context* context) { } } +// This bug is hard to replicate with queries alone, but does seem to show +// up in some cases of the query system. +static void testInfiniteCycleBug() { + auto context = buildStdContext(); + auto ctx = context.get(); + + ctx->advanceToNextRevision(false); + setupModuleSearchPaths(ctx, false, false, {}, {}); + + CompilerFlags flags; + flags.set(CompilerFlags::WARN_UNSTABLE, true); + setCompilerFlags(ctx, std::move(flags)); + + std::string program0 = + R""""( + proc foo() { + var x = 0; + proc bar() { return x; } + return bar(); + } + var x = foo(); + )""""; + + std::ignore = resolveQualifiedTypeOfX(ctx, program0); + + ctx->advanceToNextRevision(false); + setupModuleSearchPaths(ctx, false, false, {}, {}); + + std::string program1 = + R""""( + proc baz() { + var x = 0; + proc ding() { return x; } + return bar(); + } + var x = baz(); + )""""; + + std::ignore = resolveQualifiedTypeOfX(ctx, program1); +} + + int main() { test1(); test2(); @@ -1794,5 +1836,7 @@ int main() { test28(ctx.get()); test29(ctx.get()); + testInfiniteCycleBug(); + return 0; } From 611510ad0061e31b6e7fa4a8ca99122d5479840d Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Tue, 8 Oct 2024 16:37:35 -0700 Subject: [PATCH 4/5] Emit recursion errors when they are dected during recomputation Signed-off-by: Danila Fedorin --- frontend/include/chpl/framework/query-impl.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/include/chpl/framework/query-impl.h b/frontend/include/chpl/framework/query-impl.h index bf6603f3b759..94d046a6992e 100644 --- a/frontend/include/chpl/framework/query-impl.h +++ b/frontend/include/chpl/framework/query-impl.h @@ -233,7 +233,9 @@ Context::getResult(QueryMap* queryMap, // printf("Found result %p %s\n", savedElement, queryMap->queryName); } - if (newElementWasAdded == false && savedElement->lastChecked == -1) { + if (newElementWasAdded == false && + (savedElement->lastChecked == -1 || + savedElement->beingTestedForReuse)) { // A recursion error was encountered. We will try to gracefully handle // this error by adding it to the set of recursion errors on this // result. From 9bc8e839c7f3ba6e34c3e5f45e0bfae3a2c753b2 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Fri, 11 Oct 2024 09:27:42 -0700 Subject: [PATCH 5/5] Reorder fields in query map result to help reduce memory footprint Signed-off-by: Danila Fedorin --- frontend/include/chpl/framework/Context-detail.h | 4 ++-- frontend/lib/framework/Context.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/include/chpl/framework/Context-detail.h b/frontend/include/chpl/framework/Context-detail.h index 877230a56731..d6816482b4a2 100644 --- a/frontend/include/chpl/framework/Context-detail.h +++ b/frontend/include/chpl/framework/Context-detail.h @@ -275,8 +275,6 @@ class QueryMapResultBase { // extra boolean is fine. mutable bool beingTestedForReuse = false; - mutable QueryDependencyVec dependencies; - // Whether or not errors from this query result have been shown to the // user (they may not have been if some query checked for errors). mutable bool emittedErrors = false; @@ -287,6 +285,8 @@ class QueryMapResultBase { // This is not too strongly connected to emittedErrors (which tracks whether // errors --- if any --- were shown to the user for this query result only) mutable bool errorsPresentInSelfOrDependencies = false; + + mutable QueryDependencyVec dependencies; mutable std::set recursionErrors; mutable QueryErrorVec errors; diff --git a/frontend/lib/framework/Context.cpp b/frontend/lib/framework/Context.cpp index a79141b8b7fb..50a15dd61412 100644 --- a/frontend/lib/framework/Context.cpp +++ b/frontend/lib/framework/Context.cpp @@ -1351,9 +1351,9 @@ QueryMapResultBase::QueryMapResultBase(RevisionNumber lastChecked, : lastChecked(lastChecked), lastChanged(lastChanged), beingTestedForReuse(beingTestedForReuse), - dependencies(), emittedErrors(emittedErrors), errorsPresentInSelfOrDependencies(errorsPresentInSelfOrDependencies), + dependencies(), recursionErrors(std::move(recursionErrors)), errors(), parentQueryMap(parentQueryMap) {