From 7bc44a06a987f4c2cb75a5d50dfe958ec9c6c034 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Fri, 5 Jul 2024 10:45:49 -0700 Subject: [PATCH 1/3] Revert "Fix bug with returning the result of ternary expressions" This reverts commit cba37d19a72e15edef4f7e25f4f0783b83a92112. While this change managed to fix the bad behavior in some cases and left the behavior in others unchanged, it did add memory leaks in some places that weren't experiencing leaks otherwise. Given that it was a quick fix to begin with, it doesn't seem worth those new leaks. We'll resolve this failure when we re-implement the pass in dyno ---- Signed-off-by: Lydia Duncan --- compiler/resolution/AutoDestroyScope.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/resolution/AutoDestroyScope.cpp b/compiler/resolution/AutoDestroyScope.cpp index 5236eaf38508..7ac410a485c8 100644 --- a/compiler/resolution/AutoDestroyScope.cpp +++ b/compiler/resolution/AutoDestroyScope.cpp @@ -436,10 +436,7 @@ void AutoDestroyScope::variablesDestroy(Expr* refStmt, bool outIntentFormalReturn = forErrorReturn == false && var->hasFlag(FLAG_FORMAL_TEMP_OUT); // No deinit for out formal returns - deinited at call site - // Similarly, no deinit for if expression results. This avoids a bug, - // see https://github.com/chapel-lang/chapel/issues/25032 - if (outIntentFormalReturn == false && - var->hasFlag(FLAG_IF_EXPR_RESULT) == false) + if (outIntentFormalReturn == false) deinitialize(insertBeforeStmt, NULL, var); } } From f6c199cd9b54e6257555d335e9e3f330068dce2d Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Fri, 5 Jul 2024 10:47:22 -0700 Subject: [PATCH 2/3] Revert "Update this test for the new expected behavior" This reverts commit 332dfbb5aed06245ad5eea80bc42f4bf4e6417e2. With the change that caused it undone, this test update is also no longer needed ---- Signed-off-by: Lydia Duncan --- test/types/records/expiring/deinit-points.good | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/types/records/expiring/deinit-points.good b/test/types/records/expiring/deinit-points.good index b26488c2f444..de27f3a54c86 100644 --- a/test/types/records/expiring/deinit-points.good +++ b/test/types/records/expiring/deinit-points.good @@ -114,9 +114,11 @@ deinit t21a in makeR end outer +deinit t21b in makeR end outer +deinit t21c in makeR end outer From d65f106638bfaabe1a9f84a2ec06c728048ce5b3 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Fri, 5 Jul 2024 11:34:20 -0700 Subject: [PATCH 3/3] Test updates for the change in behavior - Makes two additional tests futures since we had to back out the quick fix - Updates the comments on three tests for the new state of affairs - Modifies the future for ternaryPlainRef to reflect its new behavior - Remove execopts for ternaryPlainRef now that it fails more broadly instead of just with memory leaks ---- Signed-off-by: Lydia Duncan --- test/types/string/ternaryPlain.future | 2 ++ test/types/string/ternaryPlainRef.execopts | 1 - test/types/string/ternaryPlainRef.future | 6 ++---- test/types/string/ternaryReturn.chpl | 6 +++--- test/types/string/ternaryReturn.future | 2 ++ test/types/string/ternaryReturn2.chpl | 4 ++-- test/types/string/ternaryReturn3.chpl | 2 +- 7 files changed, 12 insertions(+), 11 deletions(-) create mode 100644 test/types/string/ternaryPlain.future delete mode 100644 test/types/string/ternaryPlainRef.execopts create mode 100644 test/types/string/ternaryReturn.future diff --git a/test/types/string/ternaryPlain.future b/test/types/string/ternaryPlain.future new file mode 100644 index 000000000000..a23d79d8ba84 --- /dev/null +++ b/test/types/string/ternaryPlain.future @@ -0,0 +1,2 @@ +bug: strings in ternary expressions will sometimes segfault +#25032 diff --git a/test/types/string/ternaryPlainRef.execopts b/test/types/string/ternaryPlainRef.execopts deleted file mode 100644 index 0e124c953c80..000000000000 --- a/test/types/string/ternaryPlainRef.execopts +++ /dev/null @@ -1 +0,0 @@ ---memLeaks diff --git a/test/types/string/ternaryPlainRef.future b/test/types/string/ternaryPlainRef.future index 78f90a5d6048..a23d79d8ba84 100644 --- a/test/types/string/ternaryPlainRef.future +++ b/test/types/string/ternaryPlainRef.future @@ -1,4 +1,2 @@ -bug: const ref strings in ternary expressions sometimes leak memory -#25417 - -When this future is resolved, please remove the .execopts file +bug: strings in ternary expressions will sometimes segfault +#25032 diff --git a/test/types/string/ternaryReturn.chpl b/test/types/string/ternaryReturn.chpl index 40d459299247..21e88ec0534e 100644 --- a/test/types/string/ternaryReturn.chpl +++ b/test/types/string/ternaryReturn.chpl @@ -1,8 +1,8 @@ -// This test is to lock in that a bug was resolved involving returning strings -// from nested ternary returns +// This test is to lock in a bug involving returning strings from nested ternary +// returns // This is the version that originally failed. Please do not modify the return -// statement, to ensure the test continues to lock in the fix +// statement, to ensure the test continues to lock in any future fix proc positive(z : string) { param EMPTY = ''; return if z == '~' then ' ' else (if z == '+' then z else EMPTY); diff --git a/test/types/string/ternaryReturn.future b/test/types/string/ternaryReturn.future new file mode 100644 index 000000000000..a23d79d8ba84 --- /dev/null +++ b/test/types/string/ternaryReturn.future @@ -0,0 +1,2 @@ +bug: strings in ternary expressions will sometimes segfault +#25032 diff --git a/test/types/string/ternaryReturn2.chpl b/test/types/string/ternaryReturn2.chpl index 3377f0bd4aff..e3435a7ba155 100644 --- a/test/types/string/ternaryReturn2.chpl +++ b/test/types/string/ternaryReturn2.chpl @@ -1,8 +1,8 @@ -// This test is to lock in that a bug was resolved involving returning strings +// This test is to lock in behavior around a bug involving returning strings // from nested ternary returns // This is not the version that failed, it was added to ensure that the cases -// that worked before were not broken by the solution +// that worked before are not broken by the solution proc positive(z : string) { param EMPTY = ''; const temp = if z == '+' then z else EMPTY; diff --git a/test/types/string/ternaryReturn3.chpl b/test/types/string/ternaryReturn3.chpl index 242cb53b2f5f..46aa11428050 100644 --- a/test/types/string/ternaryReturn3.chpl +++ b/test/types/string/ternaryReturn3.chpl @@ -1,4 +1,4 @@ -// This test is to lock in that a bug was resolved involving returning strings +// This test is to lock in behavior around a bug involving returning strings // from nested ternary returns // This is not the version that failed, it was added to ensure that the cases