Skip to content

[flang] Intermix messages from parser and semantic analysis #90654

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 1, 2024

Conversation

klausler
Copy link
Contributor

When there are one or more fatal error messages produced by the parser, semantic analysis is not performed. But when there are messages produced by the parser and none of them are fatal, those messages are emitted to the user before compilation continues with semantic analysis, and any messages produced by semantics are emitted after the messages from parsing.

This can be confusing for the user, as the messages may no longer all be in source file location order. It also makes it difficult to write tests that check for both non-fatal messages from parsing as well as messages from semantics using inline CHECK: or other expected messages in test source code.

This patch ensures that if semantic analysis is performed, and non-fatal messages were produced by the parser, that all the messages will be combined and emitted in source file order.

@llvmbot llvmbot added flang:driver flang Flang issues not falling into any other category flang:semantics labels Apr 30, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 30, 2024

@llvm/pr-subscribers-flang-driver

Author: Peter Klausler (klausler)

Changes

When there are one or more fatal error messages produced by the parser, semantic analysis is not performed. But when there are messages produced by the parser and none of them are fatal, those messages are emitted to the user before compilation continues with semantic analysis, and any messages produced by semantics are emitted after the messages from parsing.

This can be confusing for the user, as the messages may no longer all be in source file location order. It also makes it difficult to write tests that check for both non-fatal messages from parsing as well as messages from semantics using inline CHECK: or other expected messages in test source code.

This patch ensures that if semantic analysis is performed, and non-fatal messages were produced by the parser, that all the messages will be combined and emitted in source file order.


Full diff: https://github.com/llvm/llvm-project/pull/90654.diff

8 Files Affected:

  • (modified) flang/include/flang/Frontend/FrontendAction.h (+1-1)
  • (modified) flang/include/flang/Semantics/semantics.h (+1-1)
  • (modified) flang/lib/Frontend/CompilerInstance.cpp (+1)
  • (modified) flang/lib/Frontend/FrontendAction.cpp (+13-7)
  • (modified) flang/lib/Frontend/FrontendActions.cpp (+7-7)
  • (modified) flang/lib/Semantics/semantics.cpp (+4-1)
  • (added) flang/test/Driver/message-merging.f90 (+7)
  • (modified) flang/tools/bbc/bbc.cpp (+7-4)
diff --git a/flang/include/flang/Frontend/FrontendAction.h b/flang/include/flang/Frontend/FrontendAction.h
index 266050084f4c0d..1a800cc681cf9f 100644
--- a/flang/include/flang/Frontend/FrontendAction.h
+++ b/flang/include/flang/Frontend/FrontendAction.h
@@ -112,7 +112,7 @@ class FrontendAction {
   bool runPrescan();
   // Parse the current input file. Return False if fatal errors are reported,
   // True otherwise.
-  bool runParse();
+  bool runParse(bool emitMessages);
   // Run semantic checks for the current input file. Return False if fatal
   // errors are reported, True otherwise.
   bool runSemanticChecks();
diff --git a/flang/include/flang/Semantics/semantics.h b/flang/include/flang/Semantics/semantics.h
index c8ee71945d8bde..e6ba71d53e92b0 100644
--- a/flang/include/flang/Semantics/semantics.h
+++ b/flang/include/flang/Semantics/semantics.h
@@ -312,7 +312,7 @@ class Semantics {
     return context_.FindScope(where);
   }
   bool AnyFatalError() const { return context_.AnyFatalError(); }
-  void EmitMessages(llvm::raw_ostream &) const;
+  void EmitMessages(llvm::raw_ostream &);
   void DumpSymbols(llvm::raw_ostream &);
   void DumpSymbolsSources(llvm::raw_ostream &) const;
 
diff --git a/flang/lib/Frontend/CompilerInstance.cpp b/flang/lib/Frontend/CompilerInstance.cpp
index c78137346640a0..feb58e01c36e7a 100644
--- a/flang/lib/Frontend/CompilerInstance.cpp
+++ b/flang/lib/Frontend/CompilerInstance.cpp
@@ -176,6 +176,7 @@ bool CompilerInstance::executeAction(FrontendAction &act) {
       act.endSourceFile();
     }
   }
+
   return !getDiagnostics().getClient()->getNumErrors();
 }
 
diff --git a/flang/lib/Frontend/FrontendAction.cpp b/flang/lib/Frontend/FrontendAction.cpp
index bb1c239540d9f5..765e203ddfbe58 100644
--- a/flang/lib/Frontend/FrontendAction.cpp
+++ b/flang/lib/Frontend/FrontendAction.cpp
@@ -153,7 +153,7 @@ bool FrontendAction::runPrescan() {
   return !reportFatalScanningErrors();
 }
 
-bool FrontendAction::runParse() {
+bool FrontendAction::runParse(bool emitMessages) {
   CompilerInstance &ci = this->getInstance();
 
   // Parse. In case of failure, report and return.
@@ -163,9 +163,11 @@ bool FrontendAction::runParse() {
     return false;
   }
 
-  // Report the diagnostics from getParsing
-  ci.getParsing().messages().Emit(llvm::errs(), ci.getAllCookedSources());
-
+  if (emitMessages) {
+    // Report any non-fatal diagnostics from getParsing now rather than
+    // combining them with messages from semantics.
+    ci.getParsing().messages().Emit(llvm::errs(), ci.getAllCookedSources());
+  }
   return true;
 }
 
@@ -174,10 +176,14 @@ bool FrontendAction::runSemanticChecks() {
   std::optional<parser::Program> &parseTree{ci.getParsing().parseTree()};
   assert(parseTree && "Cannot run semantic checks without a parse tree!");
 
+  // Transfer any pending non-fatal messages from parsing to semantics
+  // so that they are merged and all printed in order.
+  auto &semanticsCtx{ci.getSemanticsContext()};
+  semanticsCtx.messages().Annex(std::move(ci.getParsing().messages()));
+
   // Prepare semantics
   ci.setSemantics(std::make_unique<Fortran::semantics::Semantics>(
-      ci.getSemanticsContext(), *parseTree,
-      ci.getInvocation().getDebugModuleDir()));
+      semanticsCtx, *parseTree, ci.getInvocation().getDebugModuleDir()));
   auto &semantics = ci.getSemantics();
 
   // Run semantic checks
@@ -187,7 +193,7 @@ bool FrontendAction::runSemanticChecks() {
     return false;
   }
 
-  // Report the diagnostics from the semantic checks
+  // Report the diagnostics from parsing and the semantic checks
   semantics.EmitMessages(ci.getSemaOutputStream());
 
   return true;
diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index 87a714d17015cb..b61554947dce98 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -123,12 +123,12 @@ static bool saveMLIRTempFile(const CompilerInvocation &ci,
 bool PrescanAction::beginSourceFileAction() { return runPrescan(); }
 
 bool PrescanAndParseAction::beginSourceFileAction() {
-  return runPrescan() && runParse();
+  return runPrescan() && runParse(/*emitMessages=*/true);
 }
 
 bool PrescanAndSemaAction::beginSourceFileAction() {
-  return runPrescan() && runParse() && runSemanticChecks() &&
-         generateRtTypeTables();
+  return runPrescan() && runParse(/*emitMessages=*/false) &&
+         runSemanticChecks() && generateRtTypeTables();
 }
 
 bool PrescanAndSemaDebugAction::beginSourceFileAction() {
@@ -137,8 +137,8 @@ bool PrescanAndSemaDebugAction::beginSourceFileAction() {
   // from exiting early (i.e. in the presence of semantic errors). We should
   // never do this in actions intended for end-users or otherwise regular
   // compiler workflows!
-  return runPrescan() && runParse() && (runSemanticChecks() || true) &&
-         (generateRtTypeTables() || true);
+  return runPrescan() && runParse(/*emitMessages=*/false) &&
+         (runSemanticChecks() || true) && (generateRtTypeTables() || true);
 }
 
 static void addDependentLibs(mlir::ModuleOp &mlirModule, CompilerInstance &ci) {
@@ -275,8 +275,8 @@ bool CodeGenAction::beginSourceFileAction() {
     ci.getDiagnostics().Report(diagID);
     return false;
   }
-  bool res = runPrescan() && runParse() && runSemanticChecks() &&
-             generateRtTypeTables();
+  bool res = runPrescan() && runParse(/*emitMessages=*/false) &&
+             runSemanticChecks() && generateRtTypeTables();
   if (!res)
     return res;
 
diff --git a/flang/lib/Semantics/semantics.cpp b/flang/lib/Semantics/semantics.cpp
index e58a8f3b22c06c..7739b946c7b405 100644
--- a/flang/lib/Semantics/semantics.cpp
+++ b/flang/lib/Semantics/semantics.cpp
@@ -593,7 +593,10 @@ bool Semantics::Perform() {
       ModFileWriter{context_}.WriteAll();
 }
 
-void Semantics::EmitMessages(llvm::raw_ostream &os) const {
+void Semantics::EmitMessages(llvm::raw_ostream &os) {
+  // Resolve the CharBlock locations of the Messages to ProvenanceRanges
+  // so messages from parsing and semantics are intermixed in source order.
+  context_.messages().ResolveProvenances(context_.allCookedSources());
   context_.messages().Emit(os, context_.allCookedSources());
 }
 
diff --git a/flang/test/Driver/message-merging.f90 b/flang/test/Driver/message-merging.f90
new file mode 100644
index 00000000000000..85fed4669eb5f2
--- /dev/null
+++ b/flang/test/Driver/message-merging.f90
@@ -0,0 +1,7 @@
+!RUN: %flang -fsyntax-only -pedantic -I %S/Inputs/ %s 2>&1 | FileCheck %s
+!CHECK: warning: SAVE attribute was already specified on 'x'
+!CHECK: portability: #include: extra stuff ignored after file name
+save x
+save x
+#include <empty.h>    crud after header name
+end
diff --git a/flang/tools/bbc/bbc.cpp b/flang/tools/bbc/bbc.cpp
index a0870d3649c2e4..f9349d50055aba 100644
--- a/flang/tools/bbc/bbc.cpp
+++ b/flang/tools/bbc/bbc.cpp
@@ -289,17 +289,20 @@ static mlir::LogicalResult convertFortranSourceToMLIR(
 
   // parse the input Fortran
   parsing.Parse(llvm::outs());
-  parsing.messages().Emit(llvm::errs(), parsing.allCooked());
   if (!parsing.consumedWholeFile()) {
+    parsing.messages().Emit(llvm::errs(), parsing.allCooked());
     parsing.EmitMessage(llvm::errs(), parsing.finalRestingPlace(),
                         "parser FAIL (final position)",
                         "error: ", llvm::raw_ostream::RED);
     return mlir::failure();
-  }
-  if ((!parsing.messages().empty() && (parsing.messages().AnyFatalError())) ||
-      !parsing.parseTree().has_value()) {
+  } else if ((!parsing.messages().empty() &&
+              (parsing.messages().AnyFatalError())) ||
+             !parsing.parseTree().has_value()) {
+    parsing.messages().Emit(llvm::errs(), parsing.allCooked());
     llvm::errs() << programPrefix << "could not parse " << path << '\n';
     return mlir::failure();
+  } else {
+    semanticsContext.messages().Annex(std::move(parsing.messages()));
   }
 
   // run semantics

@llvmbot
Copy link
Member

llvmbot commented Apr 30, 2024

@llvm/pr-subscribers-flang-semantics

Author: Peter Klausler (klausler)

Changes

When there are one or more fatal error messages produced by the parser, semantic analysis is not performed. But when there are messages produced by the parser and none of them are fatal, those messages are emitted to the user before compilation continues with semantic analysis, and any messages produced by semantics are emitted after the messages from parsing.

This can be confusing for the user, as the messages may no longer all be in source file location order. It also makes it difficult to write tests that check for both non-fatal messages from parsing as well as messages from semantics using inline CHECK: or other expected messages in test source code.

This patch ensures that if semantic analysis is performed, and non-fatal messages were produced by the parser, that all the messages will be combined and emitted in source file order.


Full diff: https://github.com/llvm/llvm-project/pull/90654.diff

8 Files Affected:

  • (modified) flang/include/flang/Frontend/FrontendAction.h (+1-1)
  • (modified) flang/include/flang/Semantics/semantics.h (+1-1)
  • (modified) flang/lib/Frontend/CompilerInstance.cpp (+1)
  • (modified) flang/lib/Frontend/FrontendAction.cpp (+13-7)
  • (modified) flang/lib/Frontend/FrontendActions.cpp (+7-7)
  • (modified) flang/lib/Semantics/semantics.cpp (+4-1)
  • (added) flang/test/Driver/message-merging.f90 (+7)
  • (modified) flang/tools/bbc/bbc.cpp (+7-4)
diff --git a/flang/include/flang/Frontend/FrontendAction.h b/flang/include/flang/Frontend/FrontendAction.h
index 266050084f4c0d..1a800cc681cf9f 100644
--- a/flang/include/flang/Frontend/FrontendAction.h
+++ b/flang/include/flang/Frontend/FrontendAction.h
@@ -112,7 +112,7 @@ class FrontendAction {
   bool runPrescan();
   // Parse the current input file. Return False if fatal errors are reported,
   // True otherwise.
-  bool runParse();
+  bool runParse(bool emitMessages);
   // Run semantic checks for the current input file. Return False if fatal
   // errors are reported, True otherwise.
   bool runSemanticChecks();
diff --git a/flang/include/flang/Semantics/semantics.h b/flang/include/flang/Semantics/semantics.h
index c8ee71945d8bde..e6ba71d53e92b0 100644
--- a/flang/include/flang/Semantics/semantics.h
+++ b/flang/include/flang/Semantics/semantics.h
@@ -312,7 +312,7 @@ class Semantics {
     return context_.FindScope(where);
   }
   bool AnyFatalError() const { return context_.AnyFatalError(); }
-  void EmitMessages(llvm::raw_ostream &) const;
+  void EmitMessages(llvm::raw_ostream &);
   void DumpSymbols(llvm::raw_ostream &);
   void DumpSymbolsSources(llvm::raw_ostream &) const;
 
diff --git a/flang/lib/Frontend/CompilerInstance.cpp b/flang/lib/Frontend/CompilerInstance.cpp
index c78137346640a0..feb58e01c36e7a 100644
--- a/flang/lib/Frontend/CompilerInstance.cpp
+++ b/flang/lib/Frontend/CompilerInstance.cpp
@@ -176,6 +176,7 @@ bool CompilerInstance::executeAction(FrontendAction &act) {
       act.endSourceFile();
     }
   }
+
   return !getDiagnostics().getClient()->getNumErrors();
 }
 
diff --git a/flang/lib/Frontend/FrontendAction.cpp b/flang/lib/Frontend/FrontendAction.cpp
index bb1c239540d9f5..765e203ddfbe58 100644
--- a/flang/lib/Frontend/FrontendAction.cpp
+++ b/flang/lib/Frontend/FrontendAction.cpp
@@ -153,7 +153,7 @@ bool FrontendAction::runPrescan() {
   return !reportFatalScanningErrors();
 }
 
-bool FrontendAction::runParse() {
+bool FrontendAction::runParse(bool emitMessages) {
   CompilerInstance &ci = this->getInstance();
 
   // Parse. In case of failure, report and return.
@@ -163,9 +163,11 @@ bool FrontendAction::runParse() {
     return false;
   }
 
-  // Report the diagnostics from getParsing
-  ci.getParsing().messages().Emit(llvm::errs(), ci.getAllCookedSources());
-
+  if (emitMessages) {
+    // Report any non-fatal diagnostics from getParsing now rather than
+    // combining them with messages from semantics.
+    ci.getParsing().messages().Emit(llvm::errs(), ci.getAllCookedSources());
+  }
   return true;
 }
 
@@ -174,10 +176,14 @@ bool FrontendAction::runSemanticChecks() {
   std::optional<parser::Program> &parseTree{ci.getParsing().parseTree()};
   assert(parseTree && "Cannot run semantic checks without a parse tree!");
 
+  // Transfer any pending non-fatal messages from parsing to semantics
+  // so that they are merged and all printed in order.
+  auto &semanticsCtx{ci.getSemanticsContext()};
+  semanticsCtx.messages().Annex(std::move(ci.getParsing().messages()));
+
   // Prepare semantics
   ci.setSemantics(std::make_unique<Fortran::semantics::Semantics>(
-      ci.getSemanticsContext(), *parseTree,
-      ci.getInvocation().getDebugModuleDir()));
+      semanticsCtx, *parseTree, ci.getInvocation().getDebugModuleDir()));
   auto &semantics = ci.getSemantics();
 
   // Run semantic checks
@@ -187,7 +193,7 @@ bool FrontendAction::runSemanticChecks() {
     return false;
   }
 
-  // Report the diagnostics from the semantic checks
+  // Report the diagnostics from parsing and the semantic checks
   semantics.EmitMessages(ci.getSemaOutputStream());
 
   return true;
diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index 87a714d17015cb..b61554947dce98 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -123,12 +123,12 @@ static bool saveMLIRTempFile(const CompilerInvocation &ci,
 bool PrescanAction::beginSourceFileAction() { return runPrescan(); }
 
 bool PrescanAndParseAction::beginSourceFileAction() {
-  return runPrescan() && runParse();
+  return runPrescan() && runParse(/*emitMessages=*/true);
 }
 
 bool PrescanAndSemaAction::beginSourceFileAction() {
-  return runPrescan() && runParse() && runSemanticChecks() &&
-         generateRtTypeTables();
+  return runPrescan() && runParse(/*emitMessages=*/false) &&
+         runSemanticChecks() && generateRtTypeTables();
 }
 
 bool PrescanAndSemaDebugAction::beginSourceFileAction() {
@@ -137,8 +137,8 @@ bool PrescanAndSemaDebugAction::beginSourceFileAction() {
   // from exiting early (i.e. in the presence of semantic errors). We should
   // never do this in actions intended for end-users or otherwise regular
   // compiler workflows!
-  return runPrescan() && runParse() && (runSemanticChecks() || true) &&
-         (generateRtTypeTables() || true);
+  return runPrescan() && runParse(/*emitMessages=*/false) &&
+         (runSemanticChecks() || true) && (generateRtTypeTables() || true);
 }
 
 static void addDependentLibs(mlir::ModuleOp &mlirModule, CompilerInstance &ci) {
@@ -275,8 +275,8 @@ bool CodeGenAction::beginSourceFileAction() {
     ci.getDiagnostics().Report(diagID);
     return false;
   }
-  bool res = runPrescan() && runParse() && runSemanticChecks() &&
-             generateRtTypeTables();
+  bool res = runPrescan() && runParse(/*emitMessages=*/false) &&
+             runSemanticChecks() && generateRtTypeTables();
   if (!res)
     return res;
 
diff --git a/flang/lib/Semantics/semantics.cpp b/flang/lib/Semantics/semantics.cpp
index e58a8f3b22c06c..7739b946c7b405 100644
--- a/flang/lib/Semantics/semantics.cpp
+++ b/flang/lib/Semantics/semantics.cpp
@@ -593,7 +593,10 @@ bool Semantics::Perform() {
       ModFileWriter{context_}.WriteAll();
 }
 
-void Semantics::EmitMessages(llvm::raw_ostream &os) const {
+void Semantics::EmitMessages(llvm::raw_ostream &os) {
+  // Resolve the CharBlock locations of the Messages to ProvenanceRanges
+  // so messages from parsing and semantics are intermixed in source order.
+  context_.messages().ResolveProvenances(context_.allCookedSources());
   context_.messages().Emit(os, context_.allCookedSources());
 }
 
diff --git a/flang/test/Driver/message-merging.f90 b/flang/test/Driver/message-merging.f90
new file mode 100644
index 00000000000000..85fed4669eb5f2
--- /dev/null
+++ b/flang/test/Driver/message-merging.f90
@@ -0,0 +1,7 @@
+!RUN: %flang -fsyntax-only -pedantic -I %S/Inputs/ %s 2>&1 | FileCheck %s
+!CHECK: warning: SAVE attribute was already specified on 'x'
+!CHECK: portability: #include: extra stuff ignored after file name
+save x
+save x
+#include <empty.h>    crud after header name
+end
diff --git a/flang/tools/bbc/bbc.cpp b/flang/tools/bbc/bbc.cpp
index a0870d3649c2e4..f9349d50055aba 100644
--- a/flang/tools/bbc/bbc.cpp
+++ b/flang/tools/bbc/bbc.cpp
@@ -289,17 +289,20 @@ static mlir::LogicalResult convertFortranSourceToMLIR(
 
   // parse the input Fortran
   parsing.Parse(llvm::outs());
-  parsing.messages().Emit(llvm::errs(), parsing.allCooked());
   if (!parsing.consumedWholeFile()) {
+    parsing.messages().Emit(llvm::errs(), parsing.allCooked());
     parsing.EmitMessage(llvm::errs(), parsing.finalRestingPlace(),
                         "parser FAIL (final position)",
                         "error: ", llvm::raw_ostream::RED);
     return mlir::failure();
-  }
-  if ((!parsing.messages().empty() && (parsing.messages().AnyFatalError())) ||
-      !parsing.parseTree().has_value()) {
+  } else if ((!parsing.messages().empty() &&
+              (parsing.messages().AnyFatalError())) ||
+             !parsing.parseTree().has_value()) {
+    parsing.messages().Emit(llvm::errs(), parsing.allCooked());
     llvm::errs() << programPrefix << "could not parse " << path << '\n';
     return mlir::failure();
+  } else {
+    semanticsContext.messages().Annex(std::move(parsing.messages()));
   }
 
   // run semantics

Copy link
Contributor

@clementval clementval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@banach-space banach-space left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

When there are one or more fatal error messages produced by the
parser, semantic analysis is not performed.  But when there are
messages produced by the parser and none of them are fatal, those
messages are emitted to the user before compilation continues with
semantic analysis, and any messages produced by semantics are
emitted after the messages from parsing.

This can be confusing for the user, as the messages may no longer
all be in source file location order.  It also makes it difficult
to write tests that check for both non-fatal messages from parsing
as well as messages from semantics using inline CHECK: or other
expected messages in test source code.

This patch ensures that if semantic analysis is performed, and
non-fatal messages were produced by the parser, that all the
messages will be combined and emitted in source file order.
@klausler klausler merged commit f2e8089 into llvm:main May 1, 2024
@klausler klausler deleted the msgs branch May 1, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:driver flang:semantics flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants