[clang-tidy] Add Fuchsia checker for visibility attributes

[juliehockett]

Summary

Adding a checker to find a function given its name and add a given visibility attribute if none is present (i.e. __attribute__((visibility("hidden"))).

[aaron.ballman]

clang-tidy/fuchsia/AddVisibilityCheck.cpp

33

Would it make more sense to store a list of names and then find all of them at once, rather than a single name at a time?

35

It'd be nice to pick a slightly better string literal to use.

41

This diagnostic doesn't really tell the user what's wrong with their code.

test/clang-tidy/fuchsia-add-visibility.cpp

11

I'd also like to see tests where the attribute is already present on the definition (to ensure it doesn't get added), or is already present on the declaration and is inherited on the definition.

What should happen with code like this:

__attribute__((visibility("internal"))) void f();

void f() {

}

Will this check attempt to add __attribute__((visibility("hidden"))) to the definition?

[Eugene.Zelenko]

docs/ReleaseNotes.rst

60

Please move into new checks section.

63

Check to find -> Finds.

docs/clang-tidy/checks/fuchsia-add-visibility.rst

7

Check to find -> Finds.

[jakehehrlich]

I'd like a test for #pragma GCC visibility push(hidden) which should cause each symbol to be treated as though it were explicitly hidden.

clang-tidy/fuchsia/AddVisibilityCheck.cpp

33

+1 on this we generally want to make this change to many different functions at the same time.

[jakehehrlich]

I'd like a test for #pragma GCC visibility push(hidden) which should cause each symbol to be treated as though it were explicitly hidden.

To clarify, this check should still give a warning if the specified variable doesn't actually have that in source code. e.g. simply because "#pragma GCC visibility push(hidden)" was at the top dosn't mean these functions should get a pass.

[juliehockett]

1. Added visibility parameter
2. Updated Name parameter to be a list of names, as opposed to a single name
3. Fixed comments and documentation

[aaron.ballman]

clang-tidy/fuchsia/AddVisibilityCheck.cpp

37–40

Rather than do this, you should use parseStringList() from OptionsUtils.h. This will also ensure you don't use the wrong separator (we use semi-colons for this, not commas).

49

You should use serializeStringList() for this.

See InefficientVectorOperationCheck for an example of parsing and serializing the string lists.

57

Rather than a for loop, you can pass the list to hasAnyName().

64

Once you drop the for loop, you can pick a single name to bind to -- the check() function will be called once for each match anyway, so you don't need an explicit loop checking all of the name.

66–67

You should use a Twine to construct this in place.

69

How about: "function expected to be annotated with '%0' visibility"

I'm mostly worried about the case where the function has a visibility attribute, but it has the *wrong* visibility specified. The current wording would be confusing in that circumstance. Or is that not a scenario you care about, just that *any* visibility is specified on the function?

clang-tidy/fuchsia/AddVisibilityCheck.h

22

The backtick is in the wrong place.

[jakehehrlich]

clang-tidy/fuchsia/AddVisibilityCheck.cpp

69

The use case for this check is forcing code bases to carefully control what symbols are exported rather than just exporting everything. So if someone took the time to explicitly set the visibility of one of these symbols we care about then we should assume they knew what they were doing.

The specific use case I care about for this check is using -fvisibility=hidden and then checking to make sure a certain curated list of symbols has explicit default visibility.

[aaron.ballman]

clang-tidy/fuchsia/AddVisibilityCheck.cpp

69

Ah, so a mismatch is unimportant, that's good to know. How about: "function expected to be annotated with the 'visibility' attribute"?

[juliehockett]

Updating check to allow for #pragma and command line visibility specs, and updating tests

clang-tidy/fuchsia/AddVisibilityCheck.cpp

69

So in implementing a way to handle #pragma and command line directives, it became clear that the more comprehensive check (that replaces existing attrs) was a trivial implementation on top of that, so I put it in. If we just want to ignore those cases, I can take out the conditional path.

[aaron.ballman]

clang-tidy/fuchsia/AddVisibilityCheck.cpp

24–26

Can remove these comments.

53–54

Can you use makeArrayRef() rather than using a SmallVector that allocates?

63

const auto *

67

Why are you passing D as well?

73

Why are you passing in both VisAttr and D?

test/clang-tidy/fuchsia-add-visibility.cpp

26

Should this test case diagnose, assuming foo is in the list of functions to check?

__attribute__((visibility("default"))) void foo(int);
void foo(double); // Diagnose?

How about this case?

template <typename Ty>
__attribute__((visibility("default"))) void foo(Ty);

template <>
void foo<int>(int); // Diagnose?

[juliehockett]

After discussion, the goal of this checker slightly changed to target definitions in header files, rather than declarations. As a result, the check now adds the attribute to any definition (declaration or not) that appears in a header file.

Also updating tests.

clang-tidy/fuchsia/AddVisibilityCheck.cpp

53–54

Type-wise it gets funky -- makeArrayRef() creates an ArrayRef<std::string>, and the matcher wants a container of StringRefs. Is there a good way to do that without allocating?

[jakehehrlich]

clang-tidy/fuchsia/AddVisibilityCheck.cpp

26

What are these comments doing here?

49

What about internal?

64

Something in the list that simply has explicit visibility should pass I think. For instance saying you have a blacklist of symbols instead of a whitelist. Sometimes internal or hidden might be used but we might want to add "hidden" by default. So "Vis" might be DefaultVisibility but we don't want to raise an error/change something labeled with internal visibility to hidden.

[jakehehrlich]

Can you add a test for when #pragma GCC visibility push(hidden) is used and the visibility attribute is hidden?

[aaron.ballman]

clang-tidy/fuchsia/AddVisibilityCheck.cpp

40

This works for now, but it would be nice to expose "driver diagnostics" for sanity checking clang-tidy configuration options. These diagnostics would not be required to pass in a source location, merely the diagnostic text. We already support thing kind of things for driver-generated diagnostics, but it's not exposed to clang-tidy yet.

49

Might be a good use of StringSwitch.

53–54

I think there is, but it is orthogonal to your patch to make that many changes, so you can ignore my suggestion. I didn't see the type shenanigans there.

75

Capitalization

clang-tidy/fuchsia/AddVisibilityCheck.h

+//===--- AddVisibilityCheck.h - clang-tidy-----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_ADDVISIBILITYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_ADDVISIBILITYCHECK_H
+
+#include "../ClangTidy.h"
+#include "../utils/HeaderFileExtensionsUtils.h"
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+/// Finds functions given a list of names and suggests a fix to add
+/// an `__attribute__((visibility("VISIBILITY")))` to their definitions,
+/// if they do not already have a visibility attribute.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-add-visibility.html
+class AddVisibilityCheck : public ClangTidyCheck {
+ public:
+  AddVisibilityCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+ private:
+  std::vector<std::string> Names;
+  const std::string VisAttr;
+  Visibility Vis;
+  const std::string RawStringHeaderFileExtensions;
+  utils::HeaderFileExtensionsSet HeaderFileExtensions;
+};
+
+}  // namespace fuchsia
+}  // namespace tidy
+}  // namespace clang
+
+#endif  // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_ADDVISIBILITYCHECK_H

clang-tidy/fuchsia/AddVisibilityCheck.cpp

+//===--- AddVisibilityCheck.cpp - clang-tidy-------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "AddVisibilityCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/Visibility.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+AST_MATCHER_P(FunctionDecl, hasVisibilityAttr, Visibility, V) {
+  return Node.getExplicitVisibility(NamedDecl::VisibilityForType) == V;
+}
+
+// AST_MATCHER(FunctionDecl, isInHeaderFile) {
+//   return Node.getExplicitVisibility(NamedDecl::VisibilityForType);
+// }
+
+AddVisibilityCheck::AddVisibilityCheck(StringRef Name,
+                                       ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      Names(utils::options::parseStringList(
+          Options.get("Names", "::std::vector"))),
+      VisAttr(Options.get("Visibility", "hidden")),
+      RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
+          "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
+  if (!utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                        HeaderFileExtensions, ',')) {
+    llvm::errs() << "Invalid header file extension: "
+                 << RawStringHeaderFileExtensions << "\n";
+  }
+  if (VisAttr == "hidden")
+    Vis = HiddenVisibility;
+  else if (VisAttr == "protected")
+    Vis = ProtectedVisibility;
+  else if (VisAttr == "default")
+    Vis = DefaultVisibility;
+  else
+    llvm::errs() << "Invalid visibliity attribute: " << VisAttr << "\n";
+}
+
+void AddVisibilityCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+  Options.store(Opts, "Names", utils::options::serializeStringList(Names));
+  Options.store(Opts, "Visibility", VisAttr);
+}
+
+void AddVisibilityCheck::registerMatchers(MatchFinder *Finder) {
+
+  Finder->addMatcher(
+      functionDecl(allOf(hasAnyName(SmallVector<StringRef, 5>(Names.begin(),
+                                                              Names.end())),
+                         unless(hasVisibilityAttr(Vis))))
+          .bind("no-visibility"),
+      this);
+}
+
+void AddVisibilityCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<FunctionDecl>("no-visibility")) {
+
+    // Ignore if it comes from the "main" file ...
+    if (Result.SourceManager->isInMainFile(
+            Result.SourceManager->getExpansionLoc(D->getLocStart()))) {
+      // unless that file is a header.
+      if (!utils::isSpellingLocInHeaderFile(
+              D->getLocStart(), *Result.SourceManager, HeaderFileExtensions))
+        return;
+    }
+
+    const Attr *A = D->getAttr<VisibilityAttr>();
+    if (A && !A->isInherited() && A->getLocation().isValid())
+      diag(D->getLocStart(),
+           "%0 visibility attribute not set for specified function.")
+          << VisAttr
+          << FixItHint::CreateReplacement(A->getRange(),
+                 "visibility(\"" + VisAttr + "\")");
+    else
+       diag(D->getLocStart(),
+           "no explicit visibility attribute set for specified function")
+          << FixItHint::CreateInsertion(D->getLocStart(),
+                 "__attribute__((visibility(\"" + VisAttr + "\")))\n");
+  }
+}
+
+}  // namespace fuchsia
+}  // namespace tidy
+}  // namespace clang

clang-tidy/fuchsia/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyFuchsiaModule
+  AddVisibilityCheck.cpp
   DefaultArgumentsCheck.cpp
   FuchsiaTidyModule.cpp
   MultipleInheritanceCheck.cpp
   OverloadedOperatorCheck.cpp
   StaticallyConstructedObjectsCheck.cpp
   TrailingReturnCheck.cpp
   VirtualInheritanceCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/fuchsia/FuchsiaTidyModule.cpp

 //===--- FuchsiaTidyModule.cpp - clang-tidy--------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AddVisibilityCheck.h"
 #include "DefaultArgumentsCheck.h"
 #include "MultipleInheritanceCheck.h"
 #include "OverloadedOperatorCheck.h"
 #include "StaticallyConstructedObjectsCheck.h"
 #include "TrailingReturnCheck.h"
 #include "VirtualInheritanceCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace fuchsia {
 
 /// This module is for Fuchsia-specific checks.
 class FuchsiaModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AddVisibilityCheck>(
+        "fuchsia-add-visibility");
     CheckFactories.registerCheck<DefaultArgumentsCheck>(
         "fuchsia-default-arguments");
     CheckFactories.registerCheck<MultipleInheritanceCheck>(
         "fuchsia-multiple-inheritance");
     CheckFactories.registerCheck<OverloadedOperatorCheck>(
         "fuchsia-overloaded-operator");
     CheckFactories.registerCheck<StaticallyConstructedObjectsCheck>(
         "fuchsia-statically-constructed-objects");

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - The 'misc-incorrect-roundings' check was renamed to `bugprone-incorrect-roundings
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-incorrect-roundings.html>`_
 
 - The 'misc-string-compare' check was renamed to `readability-string-compare
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-string-compare.html>`_
 
 - New `cppcoreguidelines-avoid-goto
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_ check
 
   The usage of ``goto`` for control flow is error prone and should be replaced
   with looping constructs. Every backward jump is rejected. Forward jumps are
   only allowed in nested loops.
 
+- New `fuchsia-add-visibility
+  <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-add-visibility.html>`_ check
+
+  Finds functions given a list of names and suggests a fix to add
+  an `__attribute__((visibility("VISIBILITY")))` to their definitions,
+  if they do not already have a visibility attribute.
+
 - New `fuchsia-multiple-inheritance
   <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-multiple-inheritance.html>`_ check
 
   Warns if a class inherits from multiple classes that are not pure virtual.
 
 - New `fuchsia-statically-constructed-objects
   <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html>`_ check
 

docs/clang-tidy/checks/fuchsia-add-visibility.rst

+.. title:: clang-tidy - fuchsia-add-visibility
+
+fuchsia-add-visibility
+======================
+
+Finds functions given a list of names and suggests a fix to add
+an `__attribute__((visibility("VISIBILITY")))` to their header declaration,
+if they do not already have a visibility attribute.
+
+Note: this will apply the fix to all declarations/definitions in header
+files, so please use caution before applying it at large.
+
+For example, given the name 'func' in a header file:
+
+.. code-block:: c++
+
+  void func();
+
+would be changed to 
+
+.. code-block:: c++
+
+  __attribute__((visibility("hidden")))
+  void func();
+  
+Options
+-------
+
+.. option:: Names
+
+   A string containing a comma-separated list of function names to check for
+   a visibility attribute. Default is an empty string.
+
+.. option:: Visibility
+
+   A string specifying what visibility attribute to set, `hidden`, `default`, 
+   or `protected`. Default is `hidden`.
+
+.. option:: HeaderFileExtensions
+
+   A comma-separated list of filename extensions of header files (the filename
+   extensions should not contain "." prefix). Default is "h".
+   For header files without an extension, use an empty string (if there are no
+   other desired extensions) or leave an empty element in the list. e.g.,
+   "h,hh,hpp,hxx," (note the trailing comma).

docs/clang-tidy/checks/list.rst

.. toctree::
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    cppcoreguidelines-slicing
    cppcoreguidelines-special-member-functions
+   fuchsia-add-visibility
    fuchsia-default-arguments
    fuchsia-multiple-inheritance
    fuchsia-overloaded-operator
    fuchsia-statically-constructed-objects
    fuchsia-trailing-return
    fuchsia-virtual-inheritance
    google-build-explicit-make-pair
    google-build-namespaces

test/clang-tidy/fuchsia-add-visibility-pragma.hpp

+// RUN: %check_clang_tidy %s fuchsia-add-visibility %t -- \
+// RUN:   -config="{CheckOptions: [{key: fuchsia-add-visibility.Names, value: 'foo;bar;baz;foobar;f;g'}, \
+// RUN:               {key: fuchsia-add-visibility.Visibility, value: 'protected'}]}" \
+// RUN:   -header-filter=.* \
+// RUN: -- -std=c++11
+
+#pragma GCC visibility push(hidden)
+
+void foo();
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: protected visibility attribute not set for specified function
+// CHECK-FIXES: __attribute__((visibility("protected")))
+
+__attribute__((visibility("protected")))
+void bar() {}
+
+__attribute__((visibility("protected")))
+void baz();
+void baz() {}
+
+__attribute__((visibility("default")))
+void foobar() {}
+// CHECK-MESSAGES: [[@LINE-2]]:1: warning: protected visibility attribute not set for specified function
+// CHECK-FIXES: __attribute__((visibility("protected")))
+
+__attribute__((visibility("protected"))) 
+void foo(int);
+void foo(double);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: protected visibility attribute not set for specified function
+// CHECK-FIXES: __attribute__((visibility("protected")))
+
+template <typename Ty>
+__attribute__((visibility("protected"))) 
+void foo(Ty);
+
+template <>
+void foo<int>(int);

test/clang-tidy/fuchsia-add-visibility.hpp

+// RUN: %check_clang_tidy %s fuchsia-add-visibility %t -- \
+// RUN:   -config="{CheckOptions: [{key: fuchsia-add-visibility.Names, value: 'foo;bar;baz;foobar;f;g;h;i'}, \
+// RUN:               {key: fuchsia-add-visibility.Visibility, value: 'protected'}]}" \
+// RUN:   -header-filter=.* \
+// RUN: -- -std=c++11
+
+void foo();
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: no explicit visibility attribute set for specified function
+// CHECK-FIXES: __attribute__((visibility("protected")))
+
+void foo();
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: no explicit visibility attribute set for specified function
+// CHECK-FIXES: __attribute__((visibility("protected")))
+
+__attribute__((visibility("default")))
+void i();
+// CHECK-MESSAGES: [[@LINE-2]]:1: warning: protected visibility attribute not set for specified function
+// CHECK-FIXES: __attribute__((visibility("protected")))
+
+__attribute__((visibility("default")))
+void i();
+// CHECK-MESSAGES: [[@LINE-2]]:1: warning: protected visibility attribute not set for specified function
+// CHECK-FIXES: __attribute__((visibility("protected")))
+
+__attribute__((visibility("protected")))
+void bar() {}
+
+__attribute__((visibility("protected")))
+void baz();
+void baz() {}
+
+__attribute__((visibility("default")))
+void foobar() {}
+// CHECK-MESSAGES: [[@LINE-2]]:1: warning: protected visibility attribute not set for specified function
+// CHECK-FIXES: __attribute__((visibility("protected")))
+
+__attribute__((visibility("protected"))) 
+void foo(int);
+void foo(double);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: no explicit visibility attribute set for specified function
+// CHECK-FIXES: __attribute__((visibility("protected")))
+
+template <typename Ty>
+__attribute__((visibility("protected"))) 
+void h(Ty);
+
+template <>
+void h<int>(int);

test/clang-tidy/fuchsia-add-visibility.cpp

+// RUN: clang-tidy %s -checks=-*,fuchsia-add-visibility \
+// RUN:   -config="{CheckOptions: [{key: fuchsia-add-visibility.Names, value: 'foo;bar;baz;foobar;f;g'}, \
+// RUN:               {key: fuchsia-add-visibility.Visibility, value: 'protected'}]}" \
+// RUN: -- -std=c++11 | count 0
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.
+
+void foo();
+void foo() {}