[flang][semantics] fix crash involving equivalences #168909
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
llvmbot added flang 
Member
| @llvm/pr-subscribers-flang-semantics Author: Andre Kuhlenschmidt (akuhlens) ChangesThis PR does three things.
Mostly presenting both fixes, because I am not sure if there are missing test cases that don't cover why the code wasn't changed to be there second option from the beginning. Full diff: https://github.com/llvm/llvm-project/pull/168909.diff 4 Files Affected:
diff --git a/flang/lib/Semantics/check-data.cpp b/flang/lib/Semantics/check-data.cpp index 3bcf711735158..af89fd7a29e37 100644 --- a/flang/lib/Semantics/check-data.cpp +++ b/flang/lib/Semantics/check-data.cpp @@ -273,7 +273,7 @@ void DataChecker::Leave(const parser::EntityDecl &decl) { } void DataChecker::CompileDataInitializationsIntoInitializers() { - ConvertToInitializers(inits_, exprAnalyzer_); + ConvertToInitializers(inits_, exprAnalyzer_, /*processScopes=*/true); } } // namespace Fortran::semantics diff --git a/flang/lib/Semantics/data-to-inits.cpp b/flang/lib/Semantics/data-to-inits.cpp index bbf3b28fe03e6..af4fffaf9f23b 100644 --- a/flang/lib/Semantics/data-to-inits.cpp +++ b/flang/lib/Semantics/data-to-inits.cpp @@ -863,8 +863,18 @@ static bool ProcessScopes(const Scope &scope, if (std::find_if(associated.begin(), associated.end(), [](SymbolRef ref) { return IsInitialized(*ref); }) != associated.end()) { - result &= - CombineEquivalencedInitialization(associated, exprAnalyzer, inits); + if (std::find_if(associated.begin(), associated.end(), + [](SymbolRef ref) { return !ref->size(); }) != + associated.end()) { + // If a symbol has a non-legacy initialization, it won't have a size, + // so we can't combine its initializations the DataChecker Runs after + // this size is computed and runs this code again so it will have a + // size when encountered later. + result = false; + } else { + result &= CombineEquivalencedInitialization( + associated, exprAnalyzer, inits); + } } } if constexpr (makeDefaultInitializationExplicit) { @@ -944,8 +954,8 @@ void ConstructInitializer(const Symbol &symbol, } } -void ConvertToInitializers( - DataInitializations &inits, evaluate::ExpressionAnalyzer &exprAnalyzer) { +void ConvertToInitializers(DataInitializations &inits, + evaluate::ExpressionAnalyzer &exprAnalyzer, bool processScopes) { // Process DATA-style component /initializers/ now, so that they appear as // default values in time for EQUIVALENCE processing in ProcessScopes. for (auto &[symbolPtr, initialization] : inits) { @@ -953,7 +963,14 @@ void ConvertToInitializers( ConstructInitializer(*symbolPtr, initialization, exprAnalyzer); } } - if (ProcessScopes( + // FIXME: It is kinda weird that we need to repeatedly process the entire + // symbol table each time this is called by LegacyDataInitialization in + // ResolveNames. Could we do this once before the DataChecker and once after + // to combine initializations from Non-Legacy Initialization? Note, it passes + // all tests with just Running this code in + // CompileDataInitializationsIntoInitializers. + if (processScopes && + ProcessScopes( exprAnalyzer.context().globalScope(), exprAnalyzer, inits)) { for (auto &[symbolPtr, initialization] : inits) { if (!symbolPtr->owner().IsDerivedType()) { diff --git a/flang/lib/Semantics/data-to-inits.h b/flang/lib/Semantics/data-to-inits.h index 7486ac8113e90..57fce123ee325 100644 --- a/flang/lib/Semantics/data-to-inits.h +++ b/flang/lib/Semantics/data-to-inits.h @@ -62,8 +62,8 @@ void AccumulateDataInitializations(DataInitializations &, evaluate::ExpressionAnalyzer &, const Symbol &, const std::list<common::Indirection<parser::DataStmtValue>> &); -void ConvertToInitializers( - DataInitializations &, evaluate::ExpressionAnalyzer &); +void ConvertToInitializers(DataInitializations &, + evaluate::ExpressionAnalyzer &, bool ProcessScopes = false); } // namespace Fortran::semantics #endif // FORTRAN_SEMANTICS_DATA_TO_INITS_H_ diff --git a/flang/test/Semantics/equivalence02.f90 b/flang/test/Semantics/equivalence02.f90 new file mode 100644 index 0000000000000..6087f43d8c2c3 --- /dev/null +++ b/flang/test/Semantics/equivalence02.f90 @@ -0,0 +1,51 @@ +! RUN: %python %S/test_errors.py %s %flang_fc1 + +program main +type t1 + sequence + integer, dimension(2):: i/42, 1/ +end type +type t2 + sequence + integer :: j(2) = [41, 1] +end type + +! ERROR: Distinct default component initializations of equivalenced objects affect 'a%i(1_8)' more than once +type (t1) :: A +! ERROR: Distinct default component initializations of equivalenced objects affect 'b%j(1_8)' more than once +type (t2) :: B +! ERROR: Distinct default component initializations of equivalenced objects affect 'x' more than once +integer :: x +data x/42/ +equivalence (A, B, x) +call p(x) +call s() +end + +subroutine s() + type g1 + sequence + integer(kind=8)::d/1_8/ + end type + type g2 + sequence + integer(kind=8)::d = 2_8 + end type + ! ERROR: Distinct default component initializations of equivalenced objects affect 'c%d' more than once + type (g1) :: C + ! ERROR: Distinct default component initializations of equivalenced objects affect 'd%d' more than once + type (g2) :: D + ! ERROR: Distinct default component initializations of equivalenced objects affect 'x' more than once + ! ERROR: Distinct default component initializations of equivalenced objects affect 'y' more than once + integer :: x, y + data x/1/, y/2/ + equivalence (C, x) + equivalence (D, y) + equivalence (x, y) + print *, x, y +end subroutine + +subroutine p(x) + integer :: x + print *, x +end subroutine |
🐧 Linux x64 Test Results
|
akuhlens commented Nov 20, 2025
Comment on lines +966 to +971
| // FIXME: It is kinda weird that we need to repeatedly process the entire | ||
| // symbol table each time this is called by LegacyDataInitialization in | ||
| // ResolveNames. Could we do this once before the DataChecker and once after | ||
| // to combine initializations from Non-Legacy Initialization? Note, it passes | ||
| // all tests with just Running this code in | ||
| // CompileDataInitializationsIntoInitializers. |
Contributor Author
There was a problem hiding this comment.
This comment needs to be removed or updated depending on which path we decide to take.
klausler reviewed Nov 20, 2025
| if (std::find_if(associated.begin(), associated.end(), | ||
| [](SymbolRef ref) { return !ref->size(); }) != | ||
| associated.end()) { | ||
| // If a symbol has a non-legacy initialization, it won't have a size, |
Contributor
There was a problem hiding this comment.
What about an instance of a derived type that has no components -- if it has an explicit initializer that is an empty structure constructor, won't its size be zero?
Why does a "non-legacy initialization" imply that a symbol won't have a size?
Contributor Author
There was a problem hiding this comment.
Legacy Initialization happens during name resolution before this diff (in LegacyDataInitialization), It seems like symbols without legacy initialization don't get there sizes set until after name resolution. You are right about the zero size array, seems like we would need to make size optional to tell if it had been set or not since zero is a valid size.
Contributor Author
There was a problem hiding this comment.
I could see if removing the call to ConvertToInitializers in LegacyDataInitialization would fix the problem too without breaking anything.
Contributor
There was a problem hiding this comment.
DATA-style default initializers in PDTs might break; they need to be converted into init() values for each instantiation of the PDT for distinct kind parameter values.
| } | ||
| } | ||
| if (ProcessScopes( | ||
| // FIXME: It is kinda weird that we need to repeatedly process the entire |
Contributor
| It looks like the change to run ProcessScopes only at the end of semantics fixes this problem and avoids some of the difficulties of the first change. Barring any failures in our test suites, why wouldn't we go with this option? |
klausler approved these changes Nov 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
This PR does three things.
ProcessScopesmore resilient to data that hasn't had it's size set, instead it now just returns false to indicate that it encountered an error during processing instead of signalling an internal compiler error.ProcessScopeswe will need to keep this change.reason we were unable to give the variable a size.
Mostly presenting both fixes, because I am not sure if there are missing test cases that don't cover why the code wasn't changed to be there second option from the beginning.