[clang-tidy] Add a check to detect returning static locals in public headers

[jranieri-grammatech]

Summary

This adds a checker, llvm-problematic-statics, that detects functions in header files that return the address of a local static variable. These cause problems if the resulting pointer is expected to be unique because it may have different values across shared library boundaries.

It's has been a problem in the static analyzer that I've had to deal with in the past (https://reviews.llvm.org/D52905 and https://reviews.llvm.org/D52906).

[Eugene.Zelenko]

C++ Core Guidelines also mentioned similar patterns, so may be this check belongs to bugprone module?

docs/clang-tidy/checks/llvm-problematic-statics.rst

7 ↗

(On Diff #173008)

static local variable? Probably Looks for should be replaced with Detects. Please also wrap with 80 characters.

[JonasToth]

I agree with @Eugene.Zelenko that this check should rather live in bugprone- as the issue its diagnosing is not LLVM specific. It is ok to add an alias into the llvm module.

clang-tidy/llvm/ProblematicStaticsCheck.cpp

22 ↗

(On Diff #173008)

Nit: you can inline this variable, having it does not improve readability (IMHO) nor does it reduce duplication.

26 ↗

(On Diff #173008)

I think the matching on include should not be done. Maybe it's ok to filter for header files, but we should not assume they live in include/... (as i think this check is generally useful and not too LLVM specific).

More filters for false positives could be checking if the function itself is static or in an anonymous namespace.

33 ↗

(On Diff #173008)

Please assert on VD and Return

test/clang-tidy/llvm-problematic-statics.cpp

3 ↗

(On Diff #173008)

I think the testing should include more cases. At least normal functions, and a false case as well.

[aaron.ballman]

clang-tidy/llvm/ProblematicStaticsCheck.cpp

33 ↗

(On Diff #173008)

Please assert on VD and Return

Why?

[Eugene.Zelenko]

Please mention new check in Release Notes (in alphabetical order).

[JonasToth]

clang-tidy/llvm/ProblematicStaticsCheck.cpp

33 ↗

(On Diff #173008)

because the matcher might evolve over time. Right now this can not be triggered, that is correct. Asking for it was inappropriate but having it doesn't hurt.

[aaron.ballman]

clang-tidy/llvm/ProblematicStaticsCheck.cpp

33 ↗

(On Diff #173008)

All checks may evolve over time -- this isn't a pattern I'd like to see used in this sort of case, personally.

[JonasToth]

clang-tidy/llvm/ProblematicStaticsCheck.cpp

33 ↗

(On Diff #173008)

I don't insist.

[aaron.ballman]

docs/clang-tidy/checks/llvm-problematic-statics.rst

7 ↗

(On Diff #173008)

to have the address -> to have the same address

[NoQ]

Thanks! I don't have much to add to the review, but i very much approve this check.

[JonasToth]

ping.
This checks seems pretty much done. @jranieri-grammatech do you think there is something missing? Otherwise it could be rebased and comitted?

[jranieri-grammatech]

@JonasToth It looks like there are some outstanding review comments that need to be addressed. I've gotten some time allocated to work on this next week.

[JonasToth]

In D54222#2281642, @jranieri-grammatech wrote:

@JonasToth It looks like there are some outstanding review comments that need to be addressed. I've gotten some time allocated to work on this next week.

Great!

[jranieri-grammatech]

In D54222#1290789, @JonasToth wrote:

I agree with @Eugene.Zelenko that this check should rather live in bugprone- as the issue its diagnosing is not LLVM specific. It is ok to add an alias into the llvm module.

Here are the warnings it issues with the patch's matcher:

clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:625:43: warning: address of static local variable 'tag' may not be identical across library boundaries [llvm-problematic-statics]
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:219:1: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h:25:1: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]

And here are the additional warnings it issues when relaxing the matcher (to be unless(isExpansionInMainFile())):

clang/lib/StaticAnalyzer/Checkers/Iterator.h:130:47: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
clang/lib/StaticAnalyzer/Checkers/Iterator.h:136:47: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
clang/lib/StaticAnalyzer/Checkers/Iterator.h:142:47: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:60:5: warning: address of static local variable 'index' may not be identical across library boundaries [llvm-problematic-statics]
llvm/utils/unittest/googletest/src/gtest.cc:3989:3: warning: address of static local variable 'instance' may not be identical across library boundaries [llvm-problematic-statics]

And also this one, which was hidden by default but made running tidy noisier by always saying there was a hidden warning:

/usr/bin/../include/c++/v1/functional:1960:9: warning: address of static local variable '__policy_' may not be identical across library boundaries [llvm-problematic-statics]

All of the first set of warnings might be actual problems, but the second set definitely aren't problems because they aren't going to be used across shared library boundaries. By making this be a LLVM-specific check, we can avoid the noise of these false positives. Also, I looked through the C++ Core Guidelines for any rules that might address this but didn't see anything relevant.

Thoughts?

[JonasToth]

In D54222#2294123, @jranieri-grammatech wrote:

In D54222#1290789, @JonasToth wrote:

I agree with @Eugene.Zelenko that this check should rather live in bugprone- as the issue its diagnosing is not LLVM specific. It is ok to add an alias into the llvm module.

Here are the warnings it issues with the patch's matcher:

clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:625:43: warning: address of static local variable 'tag' may not be identical across library boundaries [llvm-problematic-statics]
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:219:1: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h:25:1: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]

And here are the additional warnings it issues when relaxing the matcher (to be unless(isExpansionInMainFile())):

clang/lib/StaticAnalyzer/Checkers/Iterator.h:130:47: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
clang/lib/StaticAnalyzer/Checkers/Iterator.h:136:47: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
clang/lib/StaticAnalyzer/Checkers/Iterator.h:142:47: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:60:5: warning: address of static local variable 'index' may not be identical across library boundaries [llvm-problematic-statics]
llvm/utils/unittest/googletest/src/gtest.cc:3989:3: warning: address of static local variable 'instance' may not be identical across library boundaries [llvm-problematic-statics]

And also this one, which was hidden by default but made running tidy noisier by always saying there was a hidden warning:

/usr/bin/../include/c++/v1/functional:1960:9: warning: address of static local variable '__policy_' may not be identical across library boundaries [llvm-problematic-statics]

All of the first set of warnings might be actual problems, but the second set definitely aren't problems because they aren't going to be used across shared library boundaries. By making this be a LLVM-specific check, we can avoid the noise of these false positives. Also, I looked through the C++ Core Guidelines for any rules that might address this but didn't see anything relevant.

Thoughts?

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.
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 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".
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?).

[jranieri-grammatech]

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

[rsmith]

clang-tools-extra/docs/clang-tidy/checks/bugprone-problematic-statics.rst

7–8

The C++ language rules do guarantee that such static locals have the same address throughout the entire program. Should this warning check the symbol visibility / dllexportedness? People are going to learn the wrong thing if we give them this diagnostic without any further explanation.

If the concern is plugins loaded with RTLD_LOCAL, sharing C++ interfaces across an RTLD_LOCAL boundary is pretty fundamentally broken, and this is far from the only way in which it goes wrong. It might be better to warn on all non-RTLD_GLOBAL calls to dlopen from C++ code...

[JonasToth]

I am confused now, but richard knows this stuff very well! The patches you referenced showed only cases where the static was defined in an inline-header definition (and templates). Does this make a difference on the semantics?
This should be clear before we continue.

[rsmith]

In D54222#2313686, @JonasToth wrote:

I am confused now, but richard knows this stuff very well! The patches you referenced showed only cases where the static was defined in an inline-header definition (and templates). Does this make a difference on the semantics?
This should be clear before we continue.

Local static variables in inline functions and function templates are required by the C++ language semantics to result in exactly one variable in the whole program. In easy / typical cases, that works. However, this fails in at least the following cases:

• Under the MS ABI, DLLs behave more like separate programs sharing an address space than like a single program, at least by default, so by default each DLL will see a different static local variable (and similarly for class statics and other vague linkage entities). Marking the class or function as __declspec(dllexport) / __declspec(dllimport) as appropriate would be sufficient to make the static variable behave properly if it appears on the DLL interface; if you don't mark it suitably, you broke the DLL import/export rules.
• On POSIX systems, dlopen with RTLD_LOCAL (which is, unfortunately, the default for many systems), or linking a shared library with -Bsymbolic, can result in symbols being duplicated across the program. That's sort of fundamental -- that's what those options are supposed to do -- but they break C++ language semantics in various ways, this being one of them. (The idea that dlopen with RTLD_LOCAL creates a local copy of the specified shared library doesn't work, because -- due to C++'s heavy reliance on vague linkage definitions in interfaces -- the shared library will in general also contain symbols that are not part of that library, such as instantiations of someone else's templates or copies of someone else's local static variables, inline functions, type info, etc., and so you can also end up with local copies of those other things that aren't part of the shared library and that are expected to be unique across the program.) The solution is, don't do that -- it's not safe to use those options for shared libraries that either expose or use a vague linkage interface (which is more or less any modern C++ interface) from a different library and for which symbol uniqueness across the boundary matters (which is very hard to falsify in general).

I'm not sure whether we can locally distinguish the first case from harmless cases where the exposed state is not on a DLL boundary. We can detect instances of the second case by looking for dlopen calls in C++ code that use RTLD_LOCAL and warning on them (but we presumably can't detect -Bsymbolic from clang-tidy, because it's a linker flag).

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "NotNullTerminatedResultCheck.h"
 #include "ParentVirtualCallCheck.h"
 #include "PosixReturnCheck.h"
+#include "ProblematicStaticsCheck.h"
 #include "RedundantBranchConditionCheck.h"
 #include "ReservedIdentifierCheck.h"
 #include "SignedCharMisuseCheck.h"

         "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>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

   NotNullTerminatedResultCheck.cpp
   ParentVirtualCallCheck.cpp
   PosixReturnCheck.cpp
+  ProblematicStaticsCheck.cpp
   RedundantBranchConditionCheck.cpp
   ReservedIdentifierCheck.cpp
   SignedCharMisuseCheck.cpp

clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.h

+//===--- 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

clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.cpp

+//===--- 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

clang-tools-extra/docs/clang-tidy/checks/bugprone-problematic-statics.rst

+.. 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;
+    }
+  };

clang-tools-extra/docs/clang-tidy/checks/list.rst

    `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>`_,

clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-problematic-statics/bugprone-problematic-statics.h

+#ifndef PROBLEMATIC_STATICS_H
+#define PROBLEMATIC_STATICS_H
+
+struct ProgramStateTrait {
+  static void *GDMIndex() {
+    static int index = 0;
+    return &index;
+  }
+};
+
+#endif

clang-tools-extra/test/clang-tidy/checkers/bugprone-problematic-statics.cpp

+// 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;
+  }
+};