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.

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

driver: add support for a more generic diagnostic handler #4302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
68 changes: 59 additions & 9 deletions driver/codegenerator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#else
#include "llvm/IR/RemarkStreamer.h"
#endif
#include "llvm/Demangle/Demangle.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/ToolOutputFile.h"
Expand Down Expand Up @@ -166,19 +167,68 @@ void inlineAsmDiagnosticHandler(const llvm::SMDiagnostic &d, void *context,
inlineAsmDiagnostic(static_cast<IRState *>(context), d, locCookie);
}
#else
struct InlineAsmDiagnosticHandler : public llvm::DiagnosticHandler {
struct LDCDiagnosticHandler : public llvm::DiagnosticHandler {
IRState *irs;
InlineAsmDiagnosticHandler(IRState *irs) : irs(irs) {}
LDCDiagnosticHandler(IRState *irs) : irs(irs) {}

// return false to defer to LLVMContext::diagnose()
bool handleDiagnostics(const llvm::DiagnosticInfo &DI) override {
if (DI.getKind() != llvm::DK_SrcMgr)
switch (DI.getKind())
{
case llvm::DK_SrcMgr:
{
const auto &DISM = llvm::cast<llvm::DiagnosticInfoSrcMgr>(DI);
switch (DISM.getSeverity())
{
case llvm::DS_Error:
++global.errors;
break;
case llvm::DS_Warning:
++global.warnings;
break;
case llvm::DS_Remark:
case llvm::DS_Note:
return false;
}

inlineAsmDiagnostic(irs, DISM.getSMDiag(), DISM.getLocCookie());
return true;
}
case llvm::DK_StackSize:
{
const auto &DISS = llvm::cast<llvm::DiagnosticInfoStackSize>(DI);
#if LDC_LLVM_VER >= 1500
llvm::StringRef fname;
unsigned line, column;
DISS.getLocation(fname, line, column);
Copy link
Member

Choose a reason for hiding this comment

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

is getting the location generalizable for all cases? (i.e. call it before switch using DI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can change this to use the IR state loc, when its not available through the diagnostic info object.

const auto loc = Loc(fname.str().c_str(), line, column);
#else
const auto loc = Loc(nullptr, 0, 0);
#endif
switch (DISS.getSeverity())
Copy link
Member

Choose a reason for hiding this comment

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

this also seems generalizable (i.e. not individual switches for severity for each kind of diag info type?)

{
case llvm::DS_Error:
error(loc, "stack frame size (%ld) exceeds limit (%ld) in '%s'",
DISS.getStackSize(),
DISS.getStackLimit(),
llvm::demangle(DISS.getFunction().getName().str()).c_str()
Copy link
Member

Choose a reason for hiding this comment

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

do we use the LLVM demangler anywhere else in the codebase? Not better to use D's own demangler?

Copy link
Member

@JohanEngelen JohanEngelen Jan 22, 2023

Choose a reason for hiding this comment

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

(because then it is always up-to-date, when building LDC with itself)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(because then it is always up-to-date, when building LDC with itself)

I don't think so. Makes sense to use the official one due to that, but the tradeoff is that is very slow. core.demangle use exceptions for control flow, which is an horrible implementation. I have a project to update the official demangler to a decent and fast parser, but I endedup getting out of time for these things :/

Copy link
Member

Choose a reason for hiding this comment

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

The compiler is erroring, don't worry about performance, it's fine if it takes an extra second even. ;)

);
return true;
case llvm::DS_Warning:
warning(loc, "stack frame size (%ld) exceeds limit (%ld) in '%s'",
DISS.getStackSize(),
DISS.getStackLimit(),
llvm::demangle(DISS.getFunction().getName().str()).c_str()
);
return true;
case llvm::DS_Remark:
case llvm::DS_Note:
return false;
}
return false;

const auto &DISM = llvm::cast<llvm::DiagnosticInfoSrcMgr>(DI);
if (DISM.getKind() == llvm::SourceMgr::DK_Error)
++global.errors;
return inlineAsmDiagnostic(irs, DISM.getSMDiag(), DISM.getLocCookie());
}
}
return false;
}
};
#endif
Expand Down Expand Up @@ -280,7 +330,7 @@ void CodeGenerator::writeAndFreeLLModule(const char *filename) {
context_.setInlineAsmDiagnosticHandler(inlineAsmDiagnosticHandler, ir_);
#else
context_.setDiagnosticHandler(
std::make_unique<InlineAsmDiagnosticHandler>(ir_));
std::make_unique<LDCDiagnosticHandler>(ir_));
#endif

std::unique_ptr<llvm::ToolOutputFile> diagnosticsOutputFile =
Expand Down
15 changes: 15 additions & 0 deletions tests/codegen/attr_warn_stack_size.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Test diagnostics for warnings on stack size

// REQUIRES: atleast_llvm1400

// RUN: %ldc -wi -c %s 2>&1 | FileCheck %s
Copy link
Member

Choose a reason for hiding this comment

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

This should be a not if you want it to error out, I think you had that correct in the first commit. But you need a fix for kinke's comment: #4293


import ldc.attributes;

// CHECK: {{.*}}Warning: stack frame size (8) exceeds limit (1) in '{{.*}}foobar{{.*}}'
@llvmAttr("warn-stack-size", "1")
float foobar(int f)
{
float _ = f;
return _;
}