[flang] Fix spurious error with separate module procedures #106768
Merged
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
When the implementation of one SMP apparently references another in what might be a specification expression, semantics may need to resolve it as a forward reference, and to allow for the replacement of a SubprogramNameDetails place-holding symbol with the final SubprogramDetails symbol. Otherwise, as in the bug report below, confusing error messages may result. (The reference in question isn't really in the specification part of a subprogram, but due to the syntactic ambiguity between the array element assignment statement and a statement function definition, it appears to be so at the time that the reference is processed.) I needed to make DumpSymbols() available via SemanticsContext to analyze this bug, and left that new API in place to make things easier next time. Fixes llvm#106705.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-semantics Author: Peter Klausler (klausler) ChangesWhen the implementation of one SMP apparently references another in what might be a specification expression, semantics may need to resolve it as a forward reference, and to allow for the replacement of a SubprogramNameDetails place-holding symbol with the final SubprogramDetails symbol. Otherwise, as in the bug report below, confusing error messages may result. (The reference in question isn't really in the specification part of a subprogram, but due to the syntactic ambiguity between the array element assignment statement and a statement function definition, it appears to be so at the time that the reference is processed.) I needed to make DumpSymbols() available via SemanticsContext to analyze this bug, and left that new API in place to make things easier next time. Fixes #106705. Full diff: https://github.com/llvm/llvm-project/pull/106768.diff 5 Files Affected:
diff --git a/flang/include/flang/Semantics/expression.h b/flang/include/flang/Semantics/expression.h
index a224b08da21da7..b1304d704232dc 100644
--- a/flang/include/flang/Semantics/expression.h
+++ b/flang/include/flang/Semantics/expression.h
@@ -354,7 +354,7 @@ class ExpressionAnalyzer {
parser::CharBlock, const ProcedureDesignator &, ActualArguments &);
using AdjustActuals =
std::optional<std::function<bool(const Symbol &, ActualArguments &)>>;
- bool ResolveForward(const Symbol &);
+ const Symbol *ResolveForward(const Symbol &);
std::pair<const Symbol *, bool /* failure due ambiguity */> ResolveGeneric(
const Symbol &, const ActualArguments &, const AdjustActuals &,
bool isSubroutine, bool mightBeStructureConstructor = false);
diff --git a/flang/include/flang/Semantics/semantics.h b/flang/include/flang/Semantics/semantics.h
index ec8d12b0f98653..e73f9d2e85d589 100644
--- a/flang/include/flang/Semantics/semantics.h
+++ b/flang/include/flang/Semantics/semantics.h
@@ -257,6 +257,8 @@ class SemanticsContext {
void NoteDefinedSymbol(const Symbol &);
bool IsSymbolDefined(const Symbol &) const;
+ void DumpSymbols(llvm::raw_ostream &);
+
private:
struct ScopeIndexComparator {
bool operator()(parser::CharBlock, parser::CharBlock) const;
diff --git a/flang/lib/Semantics/expression.cpp b/flang/lib/Semantics/expression.cpp
index 4f8632a2055d99..60db02dc764b46 100644
--- a/flang/lib/Semantics/expression.cpp
+++ b/flang/lib/Semantics/expression.cpp
@@ -2650,9 +2650,9 @@ static int ComputeCudaMatchingDistance(
// Handles a forward reference to a module function from what must
// be a specification expression. Return false if the symbol is
// an invalid forward reference.
-bool ExpressionAnalyzer::ResolveForward(const Symbol &symbol) {
+const Symbol *ExpressionAnalyzer::ResolveForward(const Symbol &symbol) {
if (context_.HasError(symbol)) {
- return false;
+ return nullptr;
}
if (const auto *details{
symbol.detailsIf<semantics::SubprogramNameDetails>()}) {
@@ -2661,8 +2661,13 @@ bool ExpressionAnalyzer::ResolveForward(const Symbol &symbol) {
// checking a specification expression in a sibling module
// procedure. Resolve its names now so that its interface
// is known.
+ const semantics::Scope &scope{symbol.owner()};
semantics::ResolveSpecificationParts(context_, symbol);
- if (symbol.has<semantics::SubprogramNameDetails>()) {
+ const Symbol *resolved{nullptr};
+ if (auto iter{scope.find(symbol.name())}; iter != scope.cend()) {
+ resolved = &*iter->second;
+ }
+ if (!resolved || resolved->has<semantics::SubprogramNameDetails>()) {
// When the symbol hasn't had its details updated, we must have
// already been in the process of resolving the function's
// specification part; but recursive function calls are not
@@ -2670,8 +2675,8 @@ bool ExpressionAnalyzer::ResolveForward(const Symbol &symbol) {
Say("The module function '%s' may not be referenced recursively in a specification expression"_err_en_US,
symbol.name());
context_.SetError(symbol);
- return false;
}
+ return resolved;
} else if (inStmtFunctionDefinition_) {
semantics::ResolveSpecificationParts(context_, symbol);
CHECK(symbol.has<semantics::SubprogramDetails>());
@@ -2679,10 +2684,10 @@ bool ExpressionAnalyzer::ResolveForward(const Symbol &symbol) {
Say("The internal function '%s' may not be referenced in a specification expression"_err_en_US,
symbol.name());
context_.SetError(symbol);
- return false;
+ return nullptr;
}
}
- return true;
+ return &symbol;
}
// Resolve a call to a generic procedure with given actual arguments.
@@ -2709,20 +2714,21 @@ std::pair<const Symbol *, bool> ExpressionAnalyzer::ResolveGeneric(
}
if (const auto *details{ultimate.detailsIf<semantics::GenericDetails>()}) {
for (const Symbol &specific0 : details->specificProcs()) {
- const Symbol &specific{BypassGeneric(specific0)};
- if (isSubroutine != !IsFunction(specific)) {
+ const Symbol &specific1{BypassGeneric(specific0)};
+ if (isSubroutine != !IsFunction(specific1)) {
continue;
}
- if (!ResolveForward(specific)) {
+ const Symbol *specific{ResolveForward(specific1)};
+ if (!specific) {
continue;
}
if (std::optional<characteristics::Procedure> procedure{
characteristics::Procedure::Characterize(
- ProcedureDesignator{specific}, context_.foldingContext(),
+ ProcedureDesignator{*specific}, context_.foldingContext(),
/*emitError=*/false)}) {
ActualArguments localActuals{actuals};
- if (specific.has<semantics::ProcBindingDetails>()) {
- if (!adjustActuals.value()(specific, localActuals)) {
+ if (specific->has<semantics::ProcBindingDetails>()) {
+ if (!adjustActuals.value()(*specific, localActuals)) {
continue;
}
}
@@ -2751,9 +2757,9 @@ std::pair<const Symbol *, bool> ExpressionAnalyzer::ResolveGeneric(
}
if (!procedure->IsElemental()) {
// takes priority over elemental match
- nonElemental = &specific;
+ nonElemental = specific;
} else {
- elemental = &specific;
+ elemental = specific;
}
crtMatchingDistance = ComputeCudaMatchingDistance(
context_.languageFeatures(), *procedure, localActuals);
@@ -2866,7 +2872,12 @@ auto ExpressionAnalyzer::GetCalleeAndArguments(const parser::Name &name,
if (context_.HasError(symbol)) {
return std::nullopt; // also handles null symbol
}
- const Symbol &ultimate{DEREF(symbol).GetUltimate()};
+ symbol = ResolveForward(*symbol);
+ if (!symbol) {
+ return std::nullopt;
+ }
+ name.symbol = const_cast<Symbol *>(symbol);
+ const Symbol &ultimate{symbol->GetUltimate()};
CheckForBadRecursion(name.source, ultimate);
bool dueToAmbiguity{false};
bool isGenericInterface{ultimate.has<semantics::GenericDetails>()};
diff --git a/flang/lib/Semantics/semantics.cpp b/flang/lib/Semantics/semantics.cpp
index f7a277d1b414f6..8592d1e5d6217e 100644
--- a/flang/lib/Semantics/semantics.cpp
+++ b/flang/lib/Semantics/semantics.cpp
@@ -655,10 +655,12 @@ void Semantics::EmitMessages(llvm::raw_ostream &os) {
context_.messages().Emit(os, context_.allCookedSources());
}
-void Semantics::DumpSymbols(llvm::raw_ostream &os) {
- DoDumpSymbols(os, context_.globalScope());
+void SemanticsContext::DumpSymbols(llvm::raw_ostream &os) {
+ DoDumpSymbols(os, globalScope());
}
+void Semantics::DumpSymbols(llvm::raw_ostream &os) { context_.DumpSymbols(os); }
+
void Semantics::DumpSymbolsSources(llvm::raw_ostream &os) const {
NameToSymbolMap symbols;
GetSymbolNames(context_.globalScope(), symbols);
diff --git a/flang/test/Semantics/smp-proc-ref.f90 b/flang/test/Semantics/smp-proc-ref.f90
new file mode 100644
index 00000000000000..9a2fae442e8e70
--- /dev/null
+++ b/flang/test/Semantics/smp-proc-ref.f90
@@ -0,0 +1,20 @@
+!RUN: %flang_fc1 -fsyntax-only %s
+module m
+ real :: qux(10)
+ interface
+ module subroutine bar(i)
+ end
+ module function baz()
+ end
+ end interface
+end
+
+submodule(m) sm
+ contains
+ module procedure bar
+ qux(i) = baz() ! ensure no bogus error here
+ end
+ module procedure baz
+ baz = 1.
+ end
+end
|
|
@klausler |
|
On Solaris/sparcv9, the new The Haven't investigated further yet. but that's not particularly useful in a |
|
That's an important clue. Would you be willing to try a possible fix for me? I can't test in your environments. |
|
#108271 is an experimental patch. |
|
I've now finally done a The In the caller I see ISTM that instead of passing the address of the second string to I suspect this is here in your patch: |
|
|
It looks like one of the CharBlock-typed names used as a std::map<> key in that scope's symbol table have gotten clobbered. Dumping the scope ( |
|
When printing the Solaris |
|
Sorry I got distracted for a while. With dumping |
|
These are valuable clues; thanks! I think I know what's wrong, and should soon have a patch to test. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When the implementation of one SMP apparently references another in what might be a specification expression, semantics may need to resolve it as a forward reference, and to allow for the replacement of a SubprogramNameDetails place-holding symbol with the final SubprogramDetails symbol. Otherwise, as in the bug report below, confusing error messages may result.
(The reference in question isn't really in the specification part of a subprogram, but due to the syntactic ambiguity between the array element assignment statement and a statement function definition, it appears to be so at the time that the reference is processed.)
I needed to make DumpSymbols() available via SemanticsContext to analyze this bug, and left that new API in place to make things easier next time.
Fixes #106705.