Skip to content

Commit

Permalink
Support LLVM 10 in GC checker (#37153)
Browse files Browse the repository at this point in the history 
Ref llvm/llvm-project@2f169e7
Ref llvm/llvm-project@6b85f8e

Also handle noreturn function call in GC frame counting,
and remove some old code for LLVM <= 7
  • Loading branch information
@yuyichao
yuyichao authored Aug 24, 2020
1 parent 92248d2 commit 4ed484f
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 55 deletions.
88 changes: 33 additions & 55 deletions src/clangsa/GCChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@
#include "clang/StaticAnalyzer/Frontend/FrontendActions.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "clang/Tooling/Tooling.h"
#if LLVM_VERSION_MAJOR >= 8
#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
#else
#include "clang/StaticAnalyzer/Core/CheckerRegistry.h"
#endif

#include <iostream>
#include <memory>
Expand All @@ -34,6 +30,7 @@
using std::make_unique;
#else
using llvm::make_unique;
#define PathSensitiveBugReport BugReport
#endif

namespace {
Expand All @@ -43,6 +40,16 @@ using namespace ento;
#define PDP std::shared_ptr<PathDiagnosticPiece>
#define MakePDP make_unique<PathDiagnosticEventPiece>

static const Stmt *getStmtForDiagnostics(const ExplodedNode *N)
{
#if LLVM_VERSION_MAJOR >= 10
return N->getStmtForDiagnostics();
#else
return PathDiagnosticLocation::getStmt(N);
#endif
}


class GCChecker
: public Checker<
eval::Call,
Expand All @@ -60,7 +67,7 @@ class GCChecker
template <typename callback>
void report_error(callback f, CheckerContext &C, const char *message) const;
void report_error(CheckerContext &C, const char *message) const {
return report_error([](BugReport *) {}, C, message);
return report_error([](PathSensitiveBugReport *) {}, C, message);
}
void
report_value_error(CheckerContext &C, SymbolRef Sym, const char *message,
Expand Down Expand Up @@ -227,11 +234,7 @@ class GCChecker

public:
void checkBeginFunction(CheckerContext &Ctx) const;
#if LLVM_VERSION_MAJOR >= 7
void checkEndFunction(const clang::ReturnStmt *RS, CheckerContext &Ctx) const;
#else
void checkEndFunction(CheckerContext &Ctx) const;
#endif
#if LLVM_VERSION_MAJOR >= 9
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
#else
Expand All @@ -248,12 +251,7 @@ class GCChecker
void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &) const;
void checkLocation(SVal Loc, bool IsLoad, const Stmt *S,
CheckerContext &) const;
class GCBugVisitor
#if LLVM_VERSION_MAJOR >= 7
: public BugReporterVisitor {
#else
: public BugReporterVisitorImpl<GCBugVisitor> {
#endif
class GCBugVisitor : public BugReporterVisitor {
public:
GCBugVisitor() {}

Expand All @@ -262,19 +260,10 @@ class GCChecker
ID.AddPointer(&X);
}

PDP VisitNode(const ExplodedNode *N,
#if LLVM_VERSION_MAJOR < 8
const ExplodedNode *PrevN,
#endif
BugReporterContext &BRC, BugReport &BR) override;
PDP VisitNode(const ExplodedNode *N, BugReporterContext &BRC, PathSensitiveBugReport &BR) override;
};

class GCValueBugVisitor
#if LLVM_VERSION_MAJOR >= 7
: public BugReporterVisitor {
#else
: public BugReporterVisitorImpl<GCValueBugVisitor> {
#endif
class GCValueBugVisitor : public BugReporterVisitor {
protected:
SymbolRef Sym;

Expand All @@ -288,17 +277,13 @@ class GCChecker
}

PDP ExplainNoPropagation(const ExplodedNode *N, PathDiagnosticLocation Pos,
BugReporterContext &BRC, BugReport &BR);
BugReporterContext &BRC, PathSensitiveBugReport &BR);
PDP ExplainNoPropagationFromExpr(const clang::Expr *FromWhere,
const ExplodedNode *N,
PathDiagnosticLocation Pos,
BugReporterContext &BRC, BugReport &BR);
BugReporterContext &BRC, PathSensitiveBugReport &BR);

PDP VisitNode(const ExplodedNode *N,
#if LLVM_VERSION_MAJOR < 8
const ExplodedNode *PrevN,
#endif
BugReporterContext &BRC, BugReport &BR) override;
PDP VisitNode(const ExplodedNode *N, BugReporterContext &BRC, PathSensitiveBugReport &BR) override;
}; // namespace
};

Expand Down Expand Up @@ -372,22 +357,19 @@ static const VarRegion *walk_back_to_global_VR(const MemRegion *Region) {
} // namespace Helpers

PDP GCChecker::GCBugVisitor::VisitNode(const ExplodedNode *N,
#if LLVM_VERSION_MAJOR < 8
const ExplodedNode *,
#endif
BugReporterContext &BRC, BugReport &BR) {
BugReporterContext &BRC, PathSensitiveBugReport &BR) {
const ExplodedNode *PrevN = N->getFirstPred();
unsigned NewGCDepth = N->getState()->get<GCDepth>();
unsigned OldGCDepth = PrevN->getState()->get<GCDepth>();
if (NewGCDepth != OldGCDepth) {
PathDiagnosticLocation Pos(PathDiagnosticLocation::getStmt(N),
PathDiagnosticLocation Pos(getStmtForDiagnostics(N),
BRC.getSourceManager(), N->getLocationContext());
return MakePDP(Pos, "GC frame changed here.");
}
unsigned NewGCState = N->getState()->get<GCDisabledAt>();
unsigned OldGCState = PrevN->getState()->get<GCDisabledAt>();
if (false /*NewGCState != OldGCState*/) {
PathDiagnosticLocation Pos(PathDiagnosticLocation::getStmt(N),
PathDiagnosticLocation Pos(getStmtForDiagnostics(N),
BRC.getSourceManager(), N->getLocationContext());
return MakePDP(Pos, "GC enabledness changed here.");
}
Expand All @@ -396,7 +378,7 @@ PDP GCChecker::GCBugVisitor::VisitNode(const ExplodedNode *N,

PDP GCChecker::GCValueBugVisitor::ExplainNoPropagationFromExpr(
const clang::Expr *FromWhere, const ExplodedNode *N,
PathDiagnosticLocation Pos, BugReporterContext &BRC, BugReport &BR) {
PathDiagnosticLocation Pos, BugReporterContext &BRC, PathSensitiveBugReport &BR) {
const MemRegion *Region =
N->getState()->getSVal(FromWhere, N->getLocationContext()).getAsRegion();
SymbolRef Parent = walkToRoot(
Expand Down Expand Up @@ -450,7 +432,7 @@ PDP GCChecker::GCValueBugVisitor::ExplainNoPropagationFromExpr(

PDP GCChecker::GCValueBugVisitor::ExplainNoPropagation(
const ExplodedNode *N, PathDiagnosticLocation Pos, BugReporterContext &BRC,
BugReport &BR) {
PathSensitiveBugReport &BR) {
if (N->getLocation().getAs<StmtPoint>()) {
const clang::Stmt *TheS = N->getLocation().castAs<StmtPoint>().getStmt();
const clang::CallExpr *CE = dyn_cast<CallExpr>(TheS);
Expand All @@ -476,15 +458,11 @@ PDP GCChecker::GCValueBugVisitor::ExplainNoPropagation(
}

PDP GCChecker::GCValueBugVisitor::VisitNode(const ExplodedNode *N,
#if LLVM_VERSION_MAJOR < 8
const ExplodedNode *,
#endif
BugReporterContext &BRC,
BugReport &BR) {
BugReporterContext &BRC, PathSensitiveBugReport &BR) {
const ExplodedNode *PrevN = N->getFirstPred();
const ValueState *NewValueState = N->getState()->get<GCValueMap>(Sym);
const ValueState *OldValueState = PrevN->getState()->get<GCValueMap>(Sym);
const Stmt *Stmt = PathDiagnosticLocation::getStmt(N);
const Stmt *Stmt = getStmtForDiagnostics(N);

PathDiagnosticLocation Pos;
if (Stmt)
Expand Down Expand Up @@ -555,7 +533,7 @@ void GCChecker::report_error(callback f, CheckerContext &C,

if (!BT)
BT.reset(new BugType(this, "Invalid GC thingy", categories::LogicError));
auto Report = make_unique<BugReport>(*BT, message, N);
auto Report = make_unique<PathSensitiveBugReport>(*BT, message, N);
Report->addVisitor(make_unique<GCBugVisitor>());
f(Report.get());
C.emitReport(std::move(Report));
Expand All @@ -571,7 +549,7 @@ void GCChecker::report_value_error(CheckerContext &C, SymbolRef Sym,

if (!BT)
BT.reset(new BugType(this, "Invalid GC thingy", categories::LogicError));
auto Report = make_unique<BugReport>(*BT, message, N);
auto Report = make_unique<PathSensitiveBugReport>(*BT, message, N);
Report->addVisitor(make_unique<GCValueBugVisitor>(Sym));
Report->addVisitor(make_unique<GCBugVisitor>());
Report->addVisitor(make_unique<ConditionBRVisitor>());
Expand Down Expand Up @@ -625,7 +603,7 @@ bool GCChecker::propagateArgumentRootedness(CheckerContext &C,
const ValueState *ValS = State->get<GCValueMap>(ArgSym);
if (!ValS) {
report_error(
[&](BugReport *Report) {
[&](PathSensitiveBugReport *Report) {
Report->addNote(
"Tried to find root for this parameter in inlined call",
PathDiagnosticLocation::create(P, C.getSourceManager()));
Expand Down Expand Up @@ -717,7 +695,10 @@ void GCChecker::checkEndFunction(const clang::ReturnStmt *RS,
C.addTransition(State);
if (!C.inTopFrame())
return;
if (C.getState()->get<GCDepth>() > 0)
// If `RS` is NULL, the function did not return normally.

This comment has been minimized.

Sign in to view

Copy link
@Keno

Keno Sep 3, 2020

Member

This is not true and fails the clangsa self tests (which for some reason we're not running on CI). I'll revert this part of the change
// This could be either an abort/assertion failure or an exception throw.
// Do not require the GC frame to match in such case.
if (RS && C.getState()->get<GCDepth>() > 0)
report_error(C, "Non-popped GC frame present at end of function");
}

Expand Down Expand Up @@ -1649,11 +1630,8 @@ extern "C" const char clang_analyzerAPIVersionString[] =
CLANG_ANALYZER_API_VERSION_STRING;
extern "C" void clang_registerCheckers(CheckerRegistry &registry) {
registry.addChecker<GCChecker>(
"julia.GCChecker", "Validates julia gc invariants"
#if LLVM_VERSION_MAJOR >= 8
,
"julia.GCChecker", "Validates julia gc invariants",
"https://docs.julialang.org/en/v1/devdocs/gc-sa/"
#endif
);
}
#endif
1 change: 1 addition & 0 deletions src/llvm-alloc-opt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1318,6 +1318,7 @@ void Optimizer::splitOnStack(CallInst *orig_inst)
}
else if (auto call = dyn_cast<CallInst>(user)) {
auto callee = call->getCalledOperand();
assert(callee); // makes it clear for clang analyser that `callee` is not NULL
if (auto intrinsic = dyn_cast<IntrinsicInst>(call)) {
if (Intrinsic::ID id = intrinsic->getIntrinsicID()) {
if (id == Intrinsic::memset) {
Expand Down

2 comments on commit 4ed484f

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Please sign in to comment.