Skip to content

Commit

Permalink
[clang-tidy][libc] Fix namespace check with macro (#68134)
Browse files Browse the repository at this point in the history
The name of the namespace for LLVM's libc is now provided by a macro.
The ImplementationNamespaceCheck was updated to handle this, but the
CalleeNamespaceCheck was missed. This patch updates the
CalleeNamespaceCheck to handle the macro.
  • Loading branch information
michaelrj-google committed Oct 6, 2023
1 parent aade746 commit daca972
Show file tree
Hide file tree
Showing 6 changed files with 73 additions and 24 deletions.
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()) &&
NS->getName().starts_with(RequiredNamespaceStart))
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 @@ -319,6 +324,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

0 comments on commit daca972

Please sign in to comment.