[llvm] [clang] DiagnosticHandler: refactor error checking (PR #75889)

2023-12-19 Thread Teresa Johnson via cfe-commits 

https://github.com/teresajohnson approved this pull request.

lgtm

https://github.com/llvm/llvm-project/pull/75889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] DiagnosticHandler: refactor error checking (PR #75889)

2023-12-19 Thread Teresa Johnson via cfe-commits 


@@ -256,10 +256,13 @@ void LLVMContext::diagnose(const DiagnosticInfo &DI) {
   RS->emit(*OptDiagBase);
 
   // If there is a report handler, use it.
-  if (pImpl->DiagHandler &&
-  (!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
-  pImpl->DiagHandler->handleDiagnostics(DI))
-return;
+  if (pImpl->DiagHandler) {
+if (DI.getSeverity() == DS_Error)
+  pImpl->DiagHandler->HasErrors = true;

teresajohnson wrote:

I didn't mean calling handle Diagnostics in more places. I just meant rather 
than directly setting the HasErrors flags here, do that in a new non-virtual 
method (e.g. prepareToHandleDiagnostics() or whatever), and call it here just 
before calling handle Diagnostics. To abstract away what it is actually doing 
and leave the setting of the flag to the DiagnosticHandler class.

https://github.com/llvm/llvm-project/pull/75889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] DiagnosticHandler: refactor error checking (PR #75889)

2023-12-19 Thread Fangrui Song via cfe-commits 


@@ -256,10 +256,13 @@ void LLVMContext::diagnose(const DiagnosticInfo &DI) {
   RS->emit(*OptDiagBase);
 
   // If there is a report handler, use it.
-  if (pImpl->DiagHandler &&
-  (!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
-  pImpl->DiagHandler->handleDiagnostics(DI))
-return;
+  if (pImpl->DiagHandler) {
+if (DI.getSeverity() == DS_Error)
+  pImpl->DiagHandler->HasErrors = true;

MaskRay wrote:

> Also, should this be set when handleDiagnostics is not called?

The `RespectDiagnosticFilters` condition is here to suppress remarks for legacy 
LTOCodeGenerator (test: `llvm/test/LTO/X86/diagnostic-handler-remarks.ll`). 
It's not used for errors. I think conveying the `HasErrors` bit seems more 
useful, although the client does not use this information.

https://github.com/llvm/llvm-project/pull/75889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] DiagnosticHandler: refactor error checking (PR #75889)

2023-12-18 Thread Fangrui Song via cfe-commits 


@@ -256,10 +256,13 @@ void LLVMContext::diagnose(const DiagnosticInfo &DI) {
   RS->emit(*OptDiagBase);
 
   // If there is a report handler, use it.
-  if (pImpl->DiagHandler &&
-  (!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
-  pImpl->DiagHandler->handleDiagnostics(DI))
-return;
+  if (pImpl->DiagHandler) {
+if (DI.getSeverity() == DS_Error)
+  pImpl->DiagHandler->HasErrors = true;

MaskRay wrote:

This unfortunately exposes the detail of DiagnosticHandler, but the alternative 
(set HasErrors in `DiagnosticHandler::handleDiagnostics`) isn't better and a 
new client may forget to call the base.

https://github.com/llvm/llvm-project/pull/75889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] DiagnosticHandler: refactor error checking (PR #75889)

2023-12-18 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Fangrui Song (MaskRay)


Changes

In LLVMContext::diagnose, set `HasErrors` for `DS_Error` so that all
derived `DiagnosticHandler` have correct `HasErrors` information.

An alternative is to set `HasErrors` in
`DiagnosticHandler::handleDiagnostics`, but all derived
`handleDiagnostics` would have to call the base function.


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


3 Files Affected:

- (modified) clang/lib/CodeGen/CodeGenAction.cpp (-2) 
- (modified) llvm/lib/IR/LLVMContext.cpp (+7-4) 
- (modified) llvm/tools/llc/llc.cpp (+3-14) 


``diff
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp 
b/clang/lib/CodeGen/CodeGenAction.cpp
index 4121a3709bc3af..753a8fd74fa696 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -418,8 +418,6 @@ void BackendConsumer::anchor() { }
 } // namespace clang
 
 bool ClangDiagnosticHandler::handleDiagnostics(const DiagnosticInfo &DI) {
-  if (DI.getSeverity() == DS_Error)
-HasErrors = true;
   BackendCon->DiagnosticHandlerImpl(DI);
   return true;
 }
diff --git a/llvm/lib/IR/LLVMContext.cpp b/llvm/lib/IR/LLVMContext.cpp
index 8ddf51537ec1af..57077e786efc26 100644
--- a/llvm/lib/IR/LLVMContext.cpp
+++ b/llvm/lib/IR/LLVMContext.cpp
@@ -256,10 +256,13 @@ void LLVMContext::diagnose(const DiagnosticInfo &DI) {
   RS->emit(*OptDiagBase);
 
   // If there is a report handler, use it.
-  if (pImpl->DiagHandler &&
-  (!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
-  pImpl->DiagHandler->handleDiagnostics(DI))
-return;
+  if (pImpl->DiagHandler) {
+if (DI.getSeverity() == DS_Error)
+  pImpl->DiagHandler->HasErrors = true;
+if ((!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
+pImpl->DiagHandler->handleDiagnostics(DI))
+  return;
+  }
 
   if (!isDiagnosticEnabled(DI))
 return;
diff --git a/llvm/tools/llc/llc.cpp b/llvm/tools/llc/llc.cpp
index 8d906cf372878f..4a1957588a2243 100644
--- a/llvm/tools/llc/llc.cpp
+++ b/llvm/tools/llc/llc.cpp
@@ -307,16 +307,12 @@ static std::unique_ptr 
GetOutputStream(const char *TargetName,
 }
 
 struct LLCDiagnosticHandler : public DiagnosticHandler {
-  bool *HasError;
-  LLCDiagnosticHandler(bool *HasErrorPtr) : HasError(HasErrorPtr) {}
   bool handleDiagnostics(const DiagnosticInfo &DI) override {
+DiagnosticHandler::handleDiagnostics(DI);
 if (DI.getKind() == llvm::DK_SrcMgr) {
   const auto &DISM = cast(DI);
   const SMDiagnostic &SMD = DISM.getSMDiag();
 
-  if (SMD.getKind() == SourceMgr::DK_Error)
-*HasError = true;
-
   SMD.print(nullptr, errs());
 
   // For testing purposes, we print the LocCookie here.
@@ -326,9 +322,6 @@ struct LLCDiagnosticHandler : public DiagnosticHandler {
   return true;
 }
 
-if (DI.getSeverity() == DS_Error)
-  *HasError = true;
-
 if (auto *Remark = dyn_cast(&DI))
   if (!Remark->isEnabled())
 return true;
@@ -413,9 +406,7 @@ int main(int argc, char **argv) {
   Context.setDiscardValueNames(DiscardValueNames);
 
   // Set a diagnostic handler that doesn't exit on the first error
-  bool HasError = false;
-  Context.setDiagnosticHandler(
-  std::make_unique(&HasError));
+  Context.setDiagnosticHandler(std::make_unique());
 
   Expected> RemarksFileOrErr =
   setupLLVMOptimizationRemarks(Context, RemarksFilename, RemarksPasses,
@@ -757,9 +748,7 @@ static int compileModule(char **argv, LLVMContext &Context) 
{
 
 PM.run(*M);
 
-auto HasError =
-((const LLCDiagnosticHandler 
*)(Context.getDiagHandlerPtr()))->HasError;
-if (*HasError)
+if (Context.getDiagHandlerPtr()->HasErrors)
   return 1;
 
 // Compare the two outputs and make sure they're the same

``




https://github.com/llvm/llvm-project/pull/75889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] DiagnosticHandler: refactor error checking (PR #75889)

2023-12-18 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-llvm-ir

Author: Fangrui Song (MaskRay)


Changes

In LLVMContext::diagnose, set `HasErrors` for `DS_Error` so that all
derived `DiagnosticHandler` have correct `HasErrors` information.

An alternative is to set `HasErrors` in
`DiagnosticHandler::handleDiagnostics`, but all derived
`handleDiagnostics` would have to call the base function.


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


3 Files Affected:

- (modified) clang/lib/CodeGen/CodeGenAction.cpp (-2) 
- (modified) llvm/lib/IR/LLVMContext.cpp (+7-4) 
- (modified) llvm/tools/llc/llc.cpp (+3-14) 


``diff
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp 
b/clang/lib/CodeGen/CodeGenAction.cpp
index 4121a3709bc3af..753a8fd74fa696 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -418,8 +418,6 @@ void BackendConsumer::anchor() { }
 } // namespace clang
 
 bool ClangDiagnosticHandler::handleDiagnostics(const DiagnosticInfo &DI) {
-  if (DI.getSeverity() == DS_Error)
-HasErrors = true;
   BackendCon->DiagnosticHandlerImpl(DI);
   return true;
 }
diff --git a/llvm/lib/IR/LLVMContext.cpp b/llvm/lib/IR/LLVMContext.cpp
index 8ddf51537ec1af..57077e786efc26 100644
--- a/llvm/lib/IR/LLVMContext.cpp
+++ b/llvm/lib/IR/LLVMContext.cpp
@@ -256,10 +256,13 @@ void LLVMContext::diagnose(const DiagnosticInfo &DI) {
   RS->emit(*OptDiagBase);
 
   // If there is a report handler, use it.
-  if (pImpl->DiagHandler &&
-  (!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
-  pImpl->DiagHandler->handleDiagnostics(DI))
-return;
+  if (pImpl->DiagHandler) {
+if (DI.getSeverity() == DS_Error)
+  pImpl->DiagHandler->HasErrors = true;
+if ((!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
+pImpl->DiagHandler->handleDiagnostics(DI))
+  return;
+  }
 
   if (!isDiagnosticEnabled(DI))
 return;
diff --git a/llvm/tools/llc/llc.cpp b/llvm/tools/llc/llc.cpp
index 8d906cf372878f..4a1957588a2243 100644
--- a/llvm/tools/llc/llc.cpp
+++ b/llvm/tools/llc/llc.cpp
@@ -307,16 +307,12 @@ static std::unique_ptr 
GetOutputStream(const char *TargetName,
 }
 
 struct LLCDiagnosticHandler : public DiagnosticHandler {
-  bool *HasError;
-  LLCDiagnosticHandler(bool *HasErrorPtr) : HasError(HasErrorPtr) {}
   bool handleDiagnostics(const DiagnosticInfo &DI) override {
+DiagnosticHandler::handleDiagnostics(DI);
 if (DI.getKind() == llvm::DK_SrcMgr) {
   const auto &DISM = cast(DI);
   const SMDiagnostic &SMD = DISM.getSMDiag();
 
-  if (SMD.getKind() == SourceMgr::DK_Error)
-*HasError = true;
-
   SMD.print(nullptr, errs());
 
   // For testing purposes, we print the LocCookie here.
@@ -326,9 +322,6 @@ struct LLCDiagnosticHandler : public DiagnosticHandler {
   return true;
 }
 
-if (DI.getSeverity() == DS_Error)
-  *HasError = true;
-
 if (auto *Remark = dyn_cast(&DI))
   if (!Remark->isEnabled())
 return true;
@@ -413,9 +406,7 @@ int main(int argc, char **argv) {
   Context.setDiscardValueNames(DiscardValueNames);
 
   // Set a diagnostic handler that doesn't exit on the first error
-  bool HasError = false;
-  Context.setDiagnosticHandler(
-  std::make_unique(&HasError));
+  Context.setDiagnosticHandler(std::make_unique());
 
   Expected> RemarksFileOrErr =
   setupLLVMOptimizationRemarks(Context, RemarksFilename, RemarksPasses,
@@ -757,9 +748,7 @@ static int compileModule(char **argv, LLVMContext &Context) 
{
 
 PM.run(*M);
 
-auto HasError =
-((const LLCDiagnosticHandler 
*)(Context.getDiagHandlerPtr()))->HasError;
-if (*HasError)
+if (Context.getDiagHandlerPtr()->HasErrors)
   return 1;
 
 // Compare the two outputs and make sure they're the same

``




https://github.com/llvm/llvm-project/pull/75889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] DiagnosticHandler: refactor error checking (PR #75889)

2023-12-18 Thread Fangrui Song via cfe-commits 

https://github.com/MaskRay created 
https://github.com/llvm/llvm-project/pull/75889

In LLVMContext::diagnose, set `HasErrors` for `DS_Error` so that all
derived `DiagnosticHandler` have correct `HasErrors` information.

An alternative is to set `HasErrors` in
`DiagnosticHandler::handleDiagnostics`, but all derived
`handleDiagnostics` would have to call the base function.


>From 855adfece4c0c70b0198bbffa1831ccf7060e715 Mon Sep 17 00:00:00 2001
From: Fangrui Song 
Date: Mon, 18 Dec 2023 19:32:52 -0800
Subject: [PATCH] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20initia?=
 =?UTF-8?q?l=20version?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Created using spr 1.3.4
---
 clang/lib/CodeGen/CodeGenAction.cpp |  2 --
 llvm/lib/IR/LLVMContext.cpp | 11 +++
 llvm/tools/llc/llc.cpp  | 17 +++--
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/clang/lib/CodeGen/CodeGenAction.cpp 
b/clang/lib/CodeGen/CodeGenAction.cpp
index 4121a3709bc3af..753a8fd74fa696 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -418,8 +418,6 @@ void BackendConsumer::anchor() { }
 } // namespace clang
 
 bool ClangDiagnosticHandler::handleDiagnostics(const DiagnosticInfo &DI) {
-  if (DI.getSeverity() == DS_Error)
-HasErrors = true;
   BackendCon->DiagnosticHandlerImpl(DI);
   return true;
 }
diff --git a/llvm/lib/IR/LLVMContext.cpp b/llvm/lib/IR/LLVMContext.cpp
index 8ddf51537ec1af..57077e786efc26 100644
--- a/llvm/lib/IR/LLVMContext.cpp
+++ b/llvm/lib/IR/LLVMContext.cpp
@@ -256,10 +256,13 @@ void LLVMContext::diagnose(const DiagnosticInfo &DI) {
   RS->emit(*OptDiagBase);
 
   // If there is a report handler, use it.
-  if (pImpl->DiagHandler &&
-  (!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
-  pImpl->DiagHandler->handleDiagnostics(DI))
-return;
+  if (pImpl->DiagHandler) {
+if (DI.getSeverity() == DS_Error)
+  pImpl->DiagHandler->HasErrors = true;
+if ((!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
+pImpl->DiagHandler->handleDiagnostics(DI))
+  return;
+  }
 
   if (!isDiagnosticEnabled(DI))
 return;
diff --git a/llvm/tools/llc/llc.cpp b/llvm/tools/llc/llc.cpp
index 8d906cf372878f..4a1957588a2243 100644
--- a/llvm/tools/llc/llc.cpp
+++ b/llvm/tools/llc/llc.cpp
@@ -307,16 +307,12 @@ static std::unique_ptr 
GetOutputStream(const char *TargetName,
 }
 
 struct LLCDiagnosticHandler : public DiagnosticHandler {
-  bool *HasError;
-  LLCDiagnosticHandler(bool *HasErrorPtr) : HasError(HasErrorPtr) {}
   bool handleDiagnostics(const DiagnosticInfo &DI) override {
+DiagnosticHandler::handleDiagnostics(DI);
 if (DI.getKind() == llvm::DK_SrcMgr) {
   const auto &DISM = cast(DI);
   const SMDiagnostic &SMD = DISM.getSMDiag();
 
-  if (SMD.getKind() == SourceMgr::DK_Error)
-*HasError = true;
-
   SMD.print(nullptr, errs());
 
   // For testing purposes, we print the LocCookie here.
@@ -326,9 +322,6 @@ struct LLCDiagnosticHandler : public DiagnosticHandler {
   return true;
 }
 
-if (DI.getSeverity() == DS_Error)
-  *HasError = true;
-
 if (auto *Remark = dyn_cast(&DI))
   if (!Remark->isEnabled())
 return true;
@@ -413,9 +406,7 @@ int main(int argc, char **argv) {
   Context.setDiscardValueNames(DiscardValueNames);
 
   // Set a diagnostic handler that doesn't exit on the first error
-  bool HasError = false;
-  Context.setDiagnosticHandler(
-  std::make_unique(&HasError));
+  Context.setDiagnosticHandler(std::make_unique());
 
   Expected> RemarksFileOrErr =
   setupLLVMOptimizationRemarks(Context, RemarksFilename, RemarksPasses,
@@ -757,9 +748,7 @@ static int compileModule(char **argv, LLVMContext &Context) 
{
 
 PM.run(*M);
 
-auto HasError =
-((const LLCDiagnosticHandler 
*)(Context.getDiagHandlerPtr()))->HasError;
-if (*HasError)
+if (Context.getDiagHandlerPtr()->HasErrors)
   return 1;
 
 // Compare the two outputs and make sure they're the same

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits