Skip to content

Commit

Permalink
clang-tidy: port over ImplicitAtomicsChecker (JuliaLang#42510)
Browse files Browse the repository at this point in the history
More correct to be a clang-tidy pass as we get better coverage. But requires
upstream support for loading clang-tidy pass plugins (not in clang v13).
  • Loading branch information
vtjnash authored and LilithHafner committed Mar 8, 2022
1 parent 92005df commit 7794a06
Show file tree
Hide file tree
Showing 9 changed files with 243 additions and 25 deletions.
20 changes: 16 additions & 4 deletions src/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -433,16 +433,18 @@ ifneq ($(BUILD_LLVM_CLANG),1)
endif
endif

clangsa: $(build_shlibdir)/libGCCheckerPlugin.$(SHLIB_EXT) $(build_shlibdir)/libImplicitAtomicsPlugin.$(SHLIB_EXT)
clangsa: $(build_shlibdir)/libGCCheckerPlugin.$(SHLIB_EXT)
clangsa: $(build_shlibdir)/libImplicitAtomicsPlugin.$(SHLIB_EXT)
# TODO: clangsa: $(build_shlibdir)/libImplicitAtomics2Plugin.$(SHLIB_EXT)

clang-sagc-%: $(SRCDIR)/%.c $(build_shlibdir)/libGCCheckerPlugin.$(SHLIB_EXT) .FORCE | analyzegc-deps-check
@$(call PRINT_ANALYZE, $(build_depsbindir)/clang -D__clang_gcanalyzer__ --analyze -Xanalyzer -analyzer-werror -Xanalyzer -analyzer-output=text --analyzer-no-default-checks \
-Xclang -load -Xclang $(build_shlibdir)/libGCCheckerPlugin.$(SHLIB_EXT) -Xclang -analyzer-checker=core$(COMMA)julia.GCChecker \
$(CLANGSA_FLAGS) $(JCPPFLAGS) $(JCFLAGS) $(DEBUGFLAGS) -fcolor-diagnostics -Werror -x c $<)
$(CLANGSA_FLAGS) $(JCPPFLAGS) $(JCFLAGS) $(DEBUGFLAGS) -fcolor-diagnostics -x c $<)
clang-sagc-%: $(SRCDIR)/%.cpp $(build_shlibdir)/libGCCheckerPlugin.$(SHLIB_EXT) .FORCE | analyzegc-deps-check
@$(call PRINT_ANALYZE, $(build_depsbindir)/clang -D__clang_gcanalyzer__ --analyze -Xanalyzer -analyzer-werror -Xanalyzer -analyzer-output=text --analyzer-no-default-checks \
-Xclang -load -Xclang $(build_shlibdir)/libGCCheckerPlugin.$(SHLIB_EXT) -Xclang -analyzer-checker=core$(COMMA)julia.GCChecker \
$(CLANGSA_FLAGS) $(CLANGSA_CXXFLAGS) $(LLVM_CXXFLAGS) $(JCPPFLAGS) $(JCXXFLAGS) $(DEBUGFLAGS) -fcolor-diagnostics -Werror -x c++ $<)
$(CLANGSA_FLAGS) $(CLANGSA_CXXFLAGS) $(LLVM_CXXFLAGS) $(JCPPFLAGS) $(JCXXFLAGS) $(DEBUGFLAGS) -fcolor-diagnostics -x c++ $<)

# optarg is a required_argument for these
SA_EXCEPTIONS-jloptions.c := -Xanalyzer -analyzer-disable-checker=core.NonNullParamChecker,unix.cstring.NullArg
Expand All @@ -466,8 +468,18 @@ clang-sa-%: $(SRCDIR)/%.cpp $(build_shlibdir)/libImplicitAtomicsPlugin.$(SHLIB_E
$(SA_EXCEPTIONS-$(notdir $<)) \
$(CLANGSA_FLAGS) $(CLANGSA_CXXFLAGS) $(LLVM_CXXFLAGS) $(JCPPFLAGS) $(JCXXFLAGS) $(DEBUGFLAGS) -fcolor-diagnostics -Werror -x c++ $<)

clang-tidy-%: $(SRCDIR)/%.c $(build_shlibdir)/libImplicitAtomics2Plugin.$(SHLIB_EXT) .FORCE | analyzegc-deps-check
@$(call PRINT_ANALYZE, $(build_depsbindir)/clang-tidy $< -header-filter='.*' --quiet \
-load $(build_shlibdir)/libImplicitAtomics2Plugin.$(SHLIB_EXT) --checks='-clang-analyzer-*$(COMMA)-clang-diagnostic-*$(COMMA)concurrency-implicit-atomics' --warnings-as-errors='*' \
-- $(CLANGSA_FLAGS) $(JCPPFLAGS) $(JCFLAGS) $(DEBUGFLAGS) -fcolor-diagnostics -fno-caret-diagnostics -x c)
clang-tidy-%: $(SRCDIR)/%.cpp $(build_shlibdir)/libImplicitAtomics2Plugin.$(SHLIB_EXT) .FORCE | analyzegc-deps-check
@$(call PRINT_ANALYZE, $(build_depsbindir)/clang-tidy $< -header-filter='.*' --quiet \
-load $(build_shlibdir)/libImplicitAtomics2Plugin.$(SHLIB_EXT) --checks='-clang-analyzer-*$(COMMA)-clang-diagnostic-*$(COMMA)concurrency-implicit-atomics' --warnings-as-errors='*' \
-- $(CLANGSA_FLAGS) $(CLANGSA_CXXFLAGS) $(LLVM_CXXFLAGS) $(JCPPFLAGS) $(JCXXFLAGS) $(DEBUGFLAGS) -fcolor-diagnostics --system-header-prefix=llvm -Wno-deprecated-declarations -fno-caret-diagnostics -x c++)


# Add C files as a target of `analyzesrc` and `analyzegc`
# Add C files as a target of `analyzesrc` and `analyzegc` and `tidysrc`
tidysrc: $(addprefix clang-tidy-,$(filter-out $(basename $(SKIP_IMPLICIT_ATOMICS)), $(SRCS)))
analyzesrc: $(addprefix clang-sa-,$(SRCS))
analyzegc: analyzesrc $(addprefix clang-sagc-,$(RUNTIME_SRCS))

Expand Down
4 changes: 4 additions & 0 deletions src/clangsa/GCChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,10 @@ bool GCChecker::isFDAnnotatedNotSafepoint(const clang::FunctionDecl *FD) {
return declHasAnnotation(FD, "julia_not_safepoint");
}

#if LLVM_VERSION_MAJOR >= 13
#define endswith_lower endswith_insensitive
#endif

bool GCChecker::isGCTrackedType(QualType QT) {
return isValueCollection(QT) ||
isJuliaType(
Expand Down
13 changes: 1 addition & 12 deletions src/clangsa/ImplicitAtomics.cpp
Original file line number Diff line number Diff line change
@@ -1,15 +1,4 @@
//===-- ImplicitAtomicsChecker.cpp - Null dereference checker -----------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// This defines NullDerefChecker, a builtin check in ExprEngine that performs
// checks for null pointers at loads and stores.
//
//===----------------------------------------------------------------------===//
// This file is a part of Julia. License is MIT: https://julialang.org/license

#include "clang/AST/ExprObjC.h"
#include "clang/AST/ExprOpenMP.h"
Expand Down
155 changes: 155 additions & 0 deletions src/clangsa/ImplicitAtomics2.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
// This file is a part of Julia. License is MIT: https://julialang.org/license

#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang-tidy/ClangTidy.h"
#include "clang-tidy/ClangTidyCheck.h"
#include "clang-tidy/ClangTidyModule.h"
#include "clang-tidy/ClangTidyModuleRegistry.h"

using namespace clang;
using namespace clang::tidy;
using namespace clang::ast_matchers;

class ImplicitAtomicsChecker : public ClangTidyCheck {
void reportBug(const Stmt *S, StringRef desc="");

public:
ImplicitAtomicsChecker(StringRef Name, ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;

private:
};

// Checks if RD has name in Names and is in std namespace
static bool hasStdClassWithName(const CXXRecordDecl *RD,
ArrayRef<llvm::StringLiteral> Names) {
// or could check ASTContext::getQualifiedTemplateName()->isDerivedFrom() ?
if (!RD || !RD->getDeclContext()->isStdNamespace())
return false;
if (RD->getDeclName().isIdentifier()) {
StringRef Name = RD->getName();
return llvm::any_of(Names, [&Name](StringRef GivenName) -> bool {
return Name == GivenName;
});
}
return false;
}

constexpr llvm::StringLiteral STD_PTR_NAMES[] = {"atomic", "atomic_ref"};

static bool isStdAtomic(const CXXRecordDecl *RD) {
return hasStdClassWithName(RD, STD_PTR_NAMES);
}

static bool isStdAtomicCall(const Expr *E) {
return E && isStdAtomic(E->IgnoreImplicit()->getType()->getAsCXXRecordDecl());
}

static bool isStdAtomic(const Expr *E) {
return E->getType()->isAtomicType();
}

void ImplicitAtomicsChecker::reportBug(const Stmt *S, StringRef desc) {
// try to find the "best" node to attach this to, so we generate fewer duplicate reports
while (1) {
const auto *expr = dyn_cast<Expr>(S);
if (!expr)
break;
expr = expr->IgnoreParenCasts();
if (const auto *UO = dyn_cast<UnaryOperator>(expr))
S = UO->getSubExpr();
else if (const auto *BO = dyn_cast<BinaryOperator>(expr))
S = isStdAtomic(BO->getLHS()) ? BO->getLHS() :
isStdAtomic(BO->getRHS()) ? BO->getRHS() :
BO->getLHS();
else
break;
}
SmallString<100> buf;
llvm::raw_svector_ostream os(buf);
os << "Implicit Atomic seq_cst synchronization" << desc;
diag(S->getBeginLoc(), buf.str());
}


ImplicitAtomicsChecker::
ImplicitAtomicsChecker(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {
}

void ImplicitAtomicsChecker::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(castExpr(hasCastKind(CK_AtomicToNonAtomic))
.bind("cast"),
this);
Finder->addMatcher(unaryOperator(unless(hasAnyOperatorName("&")))
.bind("unary-op"),
this);
Finder->addMatcher(binaryOperator()
.bind("binary-op"),
this);
Finder->addMatcher(cxxOperatorCallExpr()
.bind("cxxcall"),
this);
Finder->addMatcher(cxxMemberCallExpr()
.bind("cxxcall"),
this);
}

void ImplicitAtomicsChecker::check(const MatchFinder::MatchResult &Result) {
if (const auto *UOp = Result.Nodes.getNodeAs<UnaryOperator>("unary-op")) {
const Expr *Sub = UOp->getSubExpr();
if (isStdAtomic(UOp) || isStdAtomic(Sub))
reportBug(UOp);
}
if (const auto *BOp = Result.Nodes.getNodeAs<BinaryOperator>("binary-op")) {
const Expr *Lhs = BOp->getLHS();
const Expr *Rhs = BOp->getRHS();
if (isStdAtomic(Lhs) || isStdAtomic(Rhs) || isStdAtomic(BOp))
reportBug(BOp);
}
if (const auto *CE = Result.Nodes.getNodeAs<CastExpr>("cast")) {
reportBug(CE);
}
if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("cxxcall")) {
if (const auto *OC = dyn_cast<CXXOperatorCallExpr>(Call)) {
const auto *CXXThisExpr = OC->getArg(0);
if (isStdAtomicCall(CXXThisExpr)) {
OverloadedOperatorKind OOK = OC->getOperator();
if (CXXOperatorCallExpr::isAssignmentOp(OOK) || OOK == OO_PlusPlus || OOK == OO_MinusMinus) {
reportBug(CXXThisExpr, " (std::atomic operator)");
}
}
}
else if (const auto *OC = dyn_cast<CXXMemberCallExpr>(Call)) {
const auto *CXXThisExpr = OC->getImplicitObjectArgument();
if (isStdAtomicCall(CXXThisExpr)) {
if (isa<CXXConversionDecl>(OC->getMethodDecl())) {
reportBug(CXXThisExpr, " (std::atomic cast)");
}
}
}
}
}

class ImplicitAtomicsCheckerModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
CheckFactories.registerCheck<ImplicitAtomicsChecker>("concurrency-implicit-atomics");
}
};

namespace clang {
namespace tidy {

// Register the ImplicitAtomicsCheckerModule using this statically initialized variable.
static ClangTidyModuleRegistry::Add<::ImplicitAtomicsCheckerModule>
X("concurrency-module", "Adds my concurrency checks.");

// This anchor is used to force the linker to link in the generated object file
// and thus register the ImplicitAtomicsCheckerModule.
volatile int ImplicitAtomicsCheckerModuleAnchorSource = 0;

} // namespace tidy
} // namespace clang
2 changes: 1 addition & 1 deletion src/gc.c
Original file line number Diff line number Diff line change
Expand Up @@ -3075,7 +3075,7 @@ static int _jl_gc_collect(jl_ptls_t ptls, jl_gc_collection_t collection)
gc_mark_sp_init(gc_cache, &sp);
// Conservative marking relies on age to tell allocated objects
// and freelist entries apart.
mark_reset_age = !support_conservative_marking;
mark_reset_age = !jl_gc_conservative_gc_support_enabled();
// Reset the age and old bit for any unmarked objects referenced by the
// `to_finalize` list. These objects are only reachable from this list
// and should not be referenced by any old objects so this won't break
Expand Down
8 changes: 4 additions & 4 deletions src/iddict.c
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ static inline int jl_table_assign_bp(jl_array_t **pa, jl_value_t *key, jl_value_
empty_slot = -1;

do {
jl_value_t *k2 = tab[index];
jl_value_t *k2 = jl_atomic_load_relaxed(&tab[index]);
if (k2 == NULL) {
if (empty_slot == -1)
empty_slot = index;
break;
}
if (jl_egal(key, k2)) {
if (tab[index + 1] != NULL) {
if (jl_atomic_load_relaxed(&tab[index + 1]) != NULL) {
jl_atomic_store_release(&tab[index + 1], val);
jl_gc_wb(a, val);
return 0;
Expand All @@ -71,8 +71,8 @@ static inline int jl_table_assign_bp(jl_array_t **pa, jl_value_t *key, jl_value_
if (empty_slot == -1)
empty_slot = index;
}
if (empty_slot == -1 && tab[index + 1] == NULL) {
assert(tab[index] == jl_nothing);
if (empty_slot == -1 && jl_atomic_load_relaxed(&tab[index + 1]) == NULL) {
assert(jl_atomic_load_relaxed(&tab[index]) == jl_nothing);
empty_slot = index;
}

Expand Down
4 changes: 2 additions & 2 deletions src/signals-unix.c
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ static void segv_handler(int sig, siginfo_t *info, void *context)
if (jl_addr_is_safepoint((uintptr_t)info->si_addr)) {
jl_set_gc_and_wait();
// Do not raise sigint on worker thread
if (ct->tid != 0)
if (jl_atomic_load_relaxed(&ct->tid) != 0)
return;
if (ct->ptls->defer_signal) {
jl_safepoint_defer_sigint();
Expand Down Expand Up @@ -798,7 +798,7 @@ static void *signal_listener(void *arg)
bt_data_prof[bt_size_cur++].uintptr = cycleclock();

// store whether thread is sleeping but add 1 as 0 is preserved to indicate end of block
bt_data_prof[bt_size_cur++].uintptr = ptls->sleep_check_state + 1;
bt_data_prof[bt_size_cur++].uintptr = jl_atomic_load_relaxed(&ptls->sleep_check_state) + 1;

// Mark the end of this block with two 0's
bt_data_prof[bt_size_cur++].uintptr = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/subtype.c
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ static int var_gt(jl_tvar_t *b, jl_value_t *a, jl_stenv_t *e, int param)
if (!((bb->ub == (jl_value_t*)jl_any_type && !jl_is_type(a) && !jl_is_typevar(a)) || subtype_ccheck(a, bb->ub, e)))
return 0;
jl_value_t *lb = simple_join(bb->lb, a);
if (!e->intersection || !subtype_by_bounds(lb, b, e))
if (!e->intersection || !subtype_by_bounds(lb, (jl_value_t*)b, e))
bb->lb = lb;
// this bound should not be directly circular
assert(bb->lb != (jl_value_t*)b);
Expand Down
Loading

0 comments on commit 7794a06

Please sign in to comment.