jranieri-grammatech updated this revision to Diff 295563.
jranieri-grammatech added a comment.
Herald added a project: clang.
In D54222#2295374 <https://reviews.llvm.org/D54222#2295374>, @JonasToth wrote:
> Do you have data for other projects? As this is not a very common thing and
> probably different for code-bases with plugins and so on, the "chattiness" of
> the check would be interesting to know.
I ran it on our large internal codebase. It generated one unique warning (2
total) with the filter on `/include/`. It was a false positive but at least it
was actually complaining about something plugin-related. When loosening it up
to any included file, that grows to five unique warnings (231 total) and all of
the additional warnings were false positives.
> If the check is usually quiet, then i would think its ok to have it as a
> general check together with proper documentation explaining why these statics
> can be a problem.
I think the fact that it's triggered in header files makes it inherently
noisier than other checkers. The warnings in LLVM I pasted above were only the
unique instances -- there were a total of 323 warnings issued for the first set
and a total of 353 warnings issued when relaxing the restrictions.
> I would still like to have it in `bugprone-`, because this is a real problem
> that can arise and the check is more likely to be ignored if considered to be
> "just for llvm".
I've updated the patch and moved it into `bugprone-`. I think it probably needs
a better name than `bugprone-problematic-statics`, now that it's expected to be
used outside of the LLVM context.
> The decision for true vs false positive is not possible to decide within
> clang-tidy, is it? I think this should then be left to the developer (as it
> is probably not that much?).
It can't be decided completely. I think that this checker would be more useful
with a configuration option to specify a regex for "plugin API headers", but I
don't have the time to implement it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54222/new/
https://reviews.llvm.org/D54222
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.cpp
clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.h
clang-tools-extra/docs/clang-tidy/checks/bugprone-problematic-statics.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-problematic-statics/bugprone-problematic-statics.h
clang-tools-extra/test/clang-tidy/checkers/bugprone-problematic-statics.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-problematic-statics.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-problematic-statics.cpp
@@ -0,0 +1,11 @@
+// RUN: %check_clang_tidy %s bugprone-problematic-statics %t -- -header-filter='.*' -- -I %S/Inputs/bugprone-problematic-statics
+
+#include "bugprone-problematic-statics.h"
+// CHECK-MESSAGES: bugprone-problematic-statics.h:7:5: warning: address of static local variable 'index' may not be identical across library boundaries [bugprone-problematic-statics]
+
+struct ProgramStateTraitInMain {
+ static void *GDMIndex() {
+ static int index = 0;
+ return &index;
+ }
+};
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-problematic-statics/bugprone-problematic-statics.h
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-problematic-statics/bugprone-problematic-statics.h
@@ -0,0 +1,11 @@
+#ifndef PROBLEMATIC_STATICS_H
+#define PROBLEMATIC_STATICS_H
+
+struct ProgramStateTrait {
+ static void *GDMIndex() {
+ static int index = 0;
+ return &index;
+ }
+};
+
+#endif
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -75,6 +75,7 @@
`bugprone-not-null-terminated-result <bugprone-not-null-terminated-result.html>`_, "Yes"
`bugprone-parent-virtual-call <bugprone-parent-virtual-call.html>`_, "Yes"
`bugprone-posix-return <bugprone-posix-return.html>`_, "Yes"
+ `bugprone-problematic-statics <bugprone-problematic-statics.html>`_,
`bugprone-redundant-branch-condition <bugprone-redundant-branch-condition.html>`_, "Yes"
`bugprone-reserved-identifier <bugprone-reserved-identifier.html>`_, "Yes"
`bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-problematic-statics.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-problematic-statics.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - bugprone-problematic-statics
+
+bugprone-problematic-statics
+============================
+
+Detects functions defined in headers that return the address of a static
+local variable. These are not guaranteed to have the same addresss across
+shared libraries and could be problematic for plugins.
+
+.. code-block:: c++
+
+ struct ProgramStateTrait {
+ static void *GDMIndex() {
+ static int index = 0;
+ return &index;
+ }
+ };
Index: clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.h
@@ -0,0 +1,32 @@
+//===--- ProblematicStaticsCheck.h - clang-tidy ----------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PROBLEMATIC_STATICS_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PROBLEMATIC_STATICS_CHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Looks for static variables in headers that may pose problems for users in
+/// other shared libraries.
+class ProblematicStaticsCheck : public ClangTidyCheck {
+public:
+ using ClangTidyCheck::ClangTidyCheck;
+
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PROBLEMATIC_STATICS_CHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.cpp
@@ -0,0 +1,40 @@
+//===--- ProblematicStaticsCheck.cpp - clang-tidy -------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "ProblematicStaticsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void ProblematicStaticsCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ returnStmt(hasReturnValue(ignoringImplicit(unaryOperator(
+ hasOperatorName("&"),
+ hasUnaryOperand(declRefExpr(
+ to(varDecl(isStaticLocal()).bind("var"))))))),
+ unless(isExpansionInMainFile()))
+ .bind("return"),
+ this);
+}
+
+void ProblematicStaticsCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *VD = Result.Nodes.getNodeAs<VarDecl>("var");
+ const auto *Return = Result.Nodes.getNodeAs<ReturnStmt>("return");
+ diag(Return->getBeginLoc(), "address of static local variable %0 may not "
+ "be identical across library boundaries")
+ << VD;
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -33,6 +33,7 @@
NotNullTerminatedResultCheck.cpp
ParentVirtualCallCheck.cpp
PosixReturnCheck.cpp
+ ProblematicStaticsCheck.cpp
RedundantBranchConditionCheck.cpp
ReservedIdentifierCheck.cpp
SignedCharMisuseCheck.cpp
Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -38,6 +38,7 @@
#include "NotNullTerminatedResultCheck.h"
#include "ParentVirtualCallCheck.h"
#include "PosixReturnCheck.h"
+#include "ProblematicStaticsCheck.h"
#include "RedundantBranchConditionCheck.h"
#include "ReservedIdentifierCheck.h"
#include "SignedCharMisuseCheck.h"
@@ -131,6 +132,8 @@
"bugprone-parent-virtual-call");
CheckFactories.registerCheck<PosixReturnCheck>(
"bugprone-posix-return");
+ CheckFactories.registerCheck<ProblematicStaticsCheck>(
+ "bugprone-problematic-statics");
CheckFactories.registerCheck<ReservedIdentifierCheck>(
"bugprone-reserved-identifier");
CheckFactories.registerCheck<SignedCharMisuseCheck>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
