Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DiagnosticHandler: refactor error checking #75889

Merged
merged 1 commit into from
Dec 20, 2023
Merged

DiagnosticHandler: refactor error checking #75889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions clang/lib/CodeGen/CodeGenAction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
11 changes: 7 additions & 4 deletions llvm/lib/IR/LLVMContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@MaskRay MaskRay Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've considered this choice. Currently,

  • LLVMContext::diagnose and handleDiagnostics need to cooperate and handle part of the process.
  • handleDiagnostics is only called by LLVMContext::diagnose. handleDiagnostics can be considered an extension point provided by LLVMContext::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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

if ((!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
pImpl->DiagHandler->handleDiagnostics(DI))
return;
}

if (!isDiagnosticEnabled(DI))
return;
Expand Down
17 changes: 3 additions & 14 deletions llvm/tools/llc/llc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down