Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-tidy][libc] Fix namespace check with macro #68134

Merged
merged 4 commits into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@
//===----------------------------------------------------------------------===//

#include "CalleeNamespaceCheck.h"
#include "NamespaceConstants.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"

#include "clang/ASTMatchers/ASTMatchers.h"
#include "llvm/ADT/StringSet.h"

using namespace clang::ast_matchers;
Expand Down Expand Up @@ -45,19 +47,22 @@ void CalleeNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
if (FuncDecl->getBuiltinID() != 0)
return;

// If the outermost namespace of the function is __llvm_libc, we're good.
// If the outermost namespace of the function is a macro that starts with
// __llvm_libc, we're good.
const auto *NS = dyn_cast<NamespaceDecl>(getOutermostNamespace(FuncDecl));
if (NS && NS->getName() == "__llvm_libc")
if (NS && Result.SourceManager->isMacroBodyExpansion(NS->getLocation()) &&
michaelrj-google marked this conversation as resolved.
Show resolved Hide resolved
NS->getName().starts_with(RequiredNamespaceStart))
PiotrZSL marked this conversation as resolved.
Show resolved Hide resolved
return;

const DeclarationName &Name = FuncDecl->getDeclName();
if (Name.isIdentifier() &&
IgnoredFunctions.contains(Name.getAsIdentifierInfo()->getName()))
return;

diag(UsageSiteExpr->getBeginLoc(), "%0 must resolve to a function declared "
"within the '__llvm_libc' namespace")
<< FuncDecl;
diag(UsageSiteExpr->getBeginLoc(),
"%0 must resolve to a function declared "
"within the namespace defined by the '%1' macro")
<< FuncDecl << RequiredNamespaceMacroName;

diag(FuncDecl->getLocation(), "resolves to this declaration",
clang::DiagnosticIDs::Note);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,14 @@
//===----------------------------------------------------------------------===//

#include "ImplementationInNamespaceCheck.h"
#include "NamespaceConstants.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"

using namespace clang::ast_matchers;

namespace clang::tidy::llvm_libc {

const static StringRef RequiredNamespaceStart = "__llvm_libc";
const static StringRef RequiredNamespaceMacroName = "LIBC_NAMESPACE";

void ImplementationInNamespaceCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
translationUnitDecl(
Expand Down
16 changes: 16 additions & 0 deletions clang-tools-extra/clang-tidy/llvmlibc/NamespaceConstants.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//===--- NamespaceConstants.h -----------------------------------*- 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
//
//===----------------------------------------------------------------------===//

#include "llvm/ADT/StringRef.h"

namespace clang::tidy::llvm_libc {

const static StringRef RequiredNamespaceStart = "__llvm_libc";
const static StringRef RequiredNamespaceMacroName = "LIBC_NAMESPACE";

} // namespace clang::tidy::llvm_libc
6 changes: 6 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,11 @@ Changes in existing checks
<clang-tidy/checks/llvm/namespace-comment>` check to provide fixes for
``inline`` namespaces in the same format as :program:`clang-format`.

- Improved :doc:`llvmlibc-callee-namespace
<clang-tidy/checks/llvmlibc/callee-namespace>` to support
customizable namespace. This matches the change made to implementation in
namespace.

- Improved :doc:`llvmlibc-implementation-in-namespace
<clang-tidy/checks/llvmlibc/implementation-in-namespace>` to support
customizable namespace. This further allows for testing the libc when the
Expand Down Expand Up @@ -321,6 +326,7 @@ Changes in existing checks
<clang-tidy/checks/readability/static-accessed-through-instance>` check to
identify calls to static member functions with out-of-class inline definitions.


Removed checks
^^^^^^^^^^^^^^

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@
llvmlibc-callee-namespace
====================================

Checks all calls resolve to functions within ``__llvm_libc`` namespace.
Checks all calls resolve to functions within correct namespace.

.. code-block:: c++

namespace __llvm_libc {
// Implementation inside the LIBC_NAMESPACE namespace.
// Correct if:
// - LIBC_NAMESPACE is a macro
// - LIBC_NAMESPACE expansion starts with `__llvm_libc`
namespace LIBC_NAMESPACE {

// Allow calls with the fully qualified name.
__llvm_libc::strlen("hello");
LIBC_NAMESPACE::strlen("hello");

// Allow calls to compiler provided functions.
(void)__builtin_abs(-1);
Expand All @@ -21,4 +25,4 @@ Checks all calls resolve to functions within ``__llvm_libc`` namespace.
// Disallow calling into functions in the global namespace.
::strlen("!");

} // namespace __llvm_libc
} // namespace LIBC_NAMESPACE
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
// RUN: %check_clang_tidy %s llvmlibc-callee-namespace %t

#define OTHER_MACRO_NAMESPACE custom_namespace
namespace OTHER_MACRO_NAMESPACE {
void wrong_name_macro_func() {}
}

namespace __llvm_libc {
void right_name_no_macro_func() {}
}

#define LIBC_NAMESPACE __llvm_libc_xyz
namespace LIBC_NAMESPACE {
namespace nested {
void nested_func() {}
} // namespace nested
Expand All @@ -22,12 +32,12 @@ struct global_struct {
int operator()() const { return 0; }
};

namespace __llvm_libc {
namespace LIBC_NAMESPACE {
void Test() {
// Allow calls with the fully qualified name.
__llvm_libc::libc_api_func();
__llvm_libc::nested::nested_func();
void (*qualifiedPtr)(void) = __llvm_libc::libc_api_func;
LIBC_NAMESPACE::libc_api_func();
LIBC_NAMESPACE::nested::nested_func();
void (*qualifiedPtr)(void) = LIBC_NAMESPACE::libc_api_func;
qualifiedPtr();

// Should not trigger on compiler provided function calls.
Expand All @@ -36,31 +46,41 @@ void Test() {
// Bare calls are allowed as long as they resolve to the correct namespace.
libc_api_func();
nested::nested_func();
void (*barePtr)(void) = __llvm_libc::libc_api_func;
void (*barePtr)(void) = LIBC_NAMESPACE::libc_api_func;
barePtr();

// Allow calling entities defined in the namespace.
__llvm_libc::libc_api_struct{}();
LIBC_NAMESPACE::libc_api_struct{}();

// Disallow calling into global namespace for implemented entrypoints.
::libc_api_func();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'libc_api_func' must resolve to a function declared within the '__llvm_libc' namespace
// CHECK-MESSAGES: :15:6: note: resolves to this declaration
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'libc_api_func' must resolve to a function declared within the namespace defined by the 'LIBC_NAMESPACE' macro
// CHECK-MESSAGES: :25:6: note: resolves to this declaration

// Disallow indirect references to functions in global namespace.
void (*badPtr)(void) = ::libc_api_func;
badPtr();
// CHECK-MESSAGES: :[[@LINE-2]]:26: warning: 'libc_api_func' must resolve to a function declared within the '__llvm_libc' namespace
// CHECK-MESSAGES: :15:6: note: resolves to this declaration
// CHECK-MESSAGES: :[[@LINE-2]]:26: warning: 'libc_api_func' must resolve to a function declared within the namespace defined by the 'LIBC_NAMESPACE' macro
// CHECK-MESSAGES: :25:6: note: resolves to this declaration

// Allow calling into global namespace for specific functions.
::malloc();

// Disallow calling on entities that are not in the namespace, but make sure
// no crashes happen.
global_struct{}();
// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'operator()' must resolve to a function declared within the '__llvm_libc' namespace
// CHECK-MESSAGES: :22:7: note: resolves to this declaration
// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'operator()' must resolve to a function declared within the namespace defined by the 'LIBC_NAMESPACE' macro
// CHECK-MESSAGES: :32:7: note: resolves to this declaration

OTHER_MACRO_NAMESPACE::wrong_name_macro_func();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'wrong_name_macro_func' must resolve to a function declared within the namespace defined by the 'LIBC_NAMESPACE' macro
// CHECK-MESSAGES: :3:31: note: expanded from macro 'OTHER_MACRO_NAMESPACE'
// CHECK-MESSAGES: :5:8: note: resolves to this declaration

__llvm_libc::right_name_no_macro_func();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'right_name_no_macro_func' must resolve to a function declared within the namespace defined by the 'LIBC_NAMESPACE' macro
// CHECK-MESSAGES: :9:8: note: resolves to this declaration

}

} // namespace __llvm_libc