DiagnosticHandler: refactor error checking #75889
Conversation
Created using spr 1.3.4
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Fangrui Song (MaskRay) ChangesIn LLVMContext::diagnose, set An alternative is to set Full diff: https://github.com/llvm/llvm-project/pull/75889.diff 3 Files Affected:
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<ToolOutputFile> 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<DiagnosticInfoSrcMgr>(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<DiagnosticInfoOptimizationBase>(&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<LLCDiagnosticHandler>(&HasError));
+ Context.setDiagnosticHandler(std::make_unique<LLCDiagnosticHandler>());
Expected<std::unique_ptr<ToolOutputFile>> 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
|
|
@llvm/pr-subscribers-llvm-ir Author: Fangrui Song (MaskRay) ChangesIn LLVMContext::diagnose, set An alternative is to set Full diff: https://github.com/llvm/llvm-project/pull/75889.diff 3 Files Affected:
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<ToolOutputFile> 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<DiagnosticInfoSrcMgr>(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<DiagnosticInfoOptimizationBase>(&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<LLCDiagnosticHandler>(&HasError));
+ Context.setDiagnosticHandler(std::make_unique<LLCDiagnosticHandler>());
Expected<std::unique_ptr<ToolOutputFile>> 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
|
|
@llvm/pr-subscribers-clang-codegen Author: Fangrui Song (MaskRay) ChangesIn LLVMContext::diagnose, set An alternative is to set Full diff: https://github.com/llvm/llvm-project/pull/75889.diff 3 Files Affected:
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<ToolOutputFile> 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<DiagnosticInfoSrcMgr>(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<DiagnosticInfoOptimizationBase>(&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<LLCDiagnosticHandler>(&HasError));
+ Context.setDiagnosticHandler(std::make_unique<LLCDiagnosticHandler>());
Expected<std::unique_ptr<ToolOutputFile>> 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
|
| return; | ||
| if (pImpl->DiagHandler) { | ||
| if (DI.getSeverity() == DS_Error) | ||
| pImpl->DiagHandler->HasErrors = true; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Perhaps move the set of HasErrors into a new non-virtual method that we call below just before calling handleDiagnostics.
Also, should this be set when handleDiagnostics is not called?
There was a problem hiding this comment.
I've considered this choice. Currently,
- LLVMContext::diagnose and handleDiagnostics need to cooperate and handle part of the process.
handleDiagnosticsis only called byLLVMContext::diagnose.handleDiagnosticscan be considered an extension point provided byLLVMContext::diagnose.
If we call handleDiagnostics in more places, it's clear that we need to duplicate some code in diagnose to the new call site, at least the LLVMRemarkStreamer stuff. Therefore, adding a new method to DiagnosticHandler does not abstract away things, and do not make things less error-prone.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Since DiagnosticHandler members are all public (and I do not want to change this), it seems that we could just avoid defining a new function, which is not supposed to be called elsewhere anyway...
MaskRay
deleted the
users/MaskRay/spr/diagnostichandler-refactor-error-checking
branch
In LLVMContext::diagnose, set
HasErrorsforDS_Errorso that allderived
DiagnosticHandlerhave correctHasErrorsinformation.An alternative is to set
HasErrorsinDiagnosticHandler::handleDiagnostics, but all derivedhandleDiagnosticswould have to call the base function.