[libc][clang-tidy] fix namespace check for externals

[michaelrj]

Summary

Up until now, all references to errno were marked with NOLINT, since
it was technically calling an external function. This fixes the lint
rules so that errno, as well as malloc, calloc, realloc, and
free are all allowed to be called as external functions. All of the
relevant NOLINT comments have been removed, and the documentation has
been updated.

[lntue]

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp

59

Look like diag() is used to print messages in this module?

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp

13

Should be <cstdio> and without newline separation form rest of headers.

39

Why not std::array of appropriate LLVM container?

[sivachandra]

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp

1

You should add tests for this exception check. See https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp.

13

Looks like this is present only for the debug printf?

39

May be static const std::uordered_set<llvm::StringRef>? It would likely make the lookup below much neater.

59

This looks like a debug printf?

[michaelrj]

clean up the code and remove debug statements

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp

13

yes, I didn't mean to leave that in there.

39

an unordered set is a good idea, but the documentation for StringRef says it's best not to use them for storage, so I went with std::string instead. The code is still a lot nicer.

59

yes, I didn't mean to leave that in there.

[michaelrj]

add test for the new lint behavior:

[sivachandra]

For libc requirements, LGTM. Please wait for @aaron.ballman for stamping the clang-tidy parts.

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp

39

Literal strings will not count as "storage". They are global data. On the other hand, std::string will make copies of the literals and require "storage".

[michaelrj]

update the set type

[michaelrj]

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp

39

When I tried to put a StringRef into an unordered_set it told me that StringRef doesn't have a hash function, so I went looking for what the proper, LLVM way of doing this was. In the end, I discovered that unordered_set is not recommended, and that StringSet does exactly what we need here, so I've switched to that.

[michaelrj]

ping for clang-tidy review

[aaron.ballman]

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp

39

This is more in line with the coding style guidelines.

40

Is there a reason that aligned_alloc function is excluded?

55–57

Style guideline nits.

[michaelrj]

address comments and rebase

[aaron.ballman]

LGTM aside from a tiny nit with the documentation.

libc/docs/clang_tidy_checks.rst

71

[michaelrj]

fix final comments before commit

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp

 //===-- CalleeNamespaceCheck.cpp ------------------------------------------===//
 //
 // 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 "CalleeNamespaceCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
+#include "llvm/ADT/StringSet.h"
+
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace llvm_libc {
 
 // Gets the outermost namespace of a DeclContext, right under the Translation
 // Unit.
 const DeclContext *getOutermostNamespace(const DeclContext *Decl) {
   const DeclContext *Parent = Decl->getParent();
   if (Parent && Parent->isTranslationUnit())
     return Decl;
   return getOutermostNamespace(Parent);
 }
 
 void CalleeNamespaceCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       declRefExpr(to(functionDecl().bind("func"))).bind("use-site"), this);
 }
 
+// A list of functions that are exempted from this check. The __errno_location
+// function is for setting errno, which is allowed in libc, and the other
+// functions are specifically allowed to be external so that they can be
+// intercepted.
+static const llvm::StringSet<> IgnoredFunctions = {
+    "__errno_location", "malloc", "calloc", "realloc", "free", "aligned_alloc"};
+
 void CalleeNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *UsageSiteExpr = Result.Nodes.getNodeAs<DeclRefExpr>("use-site");
   const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("func");
 
   // Ignore compiler builtin functions.
   if (FuncDecl->getBuiltinID() != 0)
     return;
 
   // If the outermost namespace of the function is __llvm_libc, we're good.
   const auto *NS = dyn_cast<NamespaceDecl>(getOutermostNamespace(FuncDecl));
   if (NS && NS->getName() == "__llvm_libc")
     return;
 
+  if (IgnoredFunctions.contains(FuncDecl->getName()))
+    return;
+
   diag(UsageSiteExpr->getBeginLoc(), "%0 must resolve to a function declared "
                                      "within the '__llvm_libc' namespace")
       << FuncDecl;
 
   diag(FuncDecl->getLocation(), "resolves to this declaration",
        clang::DiagnosticIDs::Note);
 }
 
 } // namespace llvm_libc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp

 // RUN: %check_clang_tidy %s llvmlibc-callee-namespace %t
 
 namespace __llvm_libc {
 namespace nested {
 void nested_func() {}
 } // namespace nested
 void libc_api_func() {}
 } // namespace __llvm_libc
 
 // Emulate a function from the public headers like string.h
 void libc_api_func() {}
 
+// Emulate a function specifically allowed by the exception list.
+void malloc() {}
+
 namespace __llvm_libc {
 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;
   qualifiedPtr();
 
void Test() {
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'libc_api_func' must resolve to a function declared within the '__llvm_libc' namespace
   // CHECK-MESSAGES: :11: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: :11:6: note: resolves to this declaration
+
+  // Allow calling into global namespace for specific functions.
+  ::malloc();
 }
 
 } // namespace __llvm_libc

libc/docs/clang_tidy_checks.rst

 LLVM-libc is distinct because it is designed to maintain interoperability with
 other libc libraries, including the one that lives on the system. This feature
 creates some uncertainty about which library a call resolves to especially when
 a public header with non-namespaced functions like ``string.h`` is included.
 
 This check ensures any function call resolves to a function within the
 __llvm_libc namespace.
 
+There are exceptions for the following functions: 
+``__errno_location`` so that ``errno`` can be set;
+``malloc``, ``calloc``, ``realloc``, ``aligned_alloc``, and ``free`` since they
+are always external and can be intercepted.
+
 .. code-block:: c++
 
     namespace __llvm_libc {
 
     // Allow calls with the fully qualified name.
     __llvm_libc::strlen("hello");
 
     // Allow calls to compiler provided functions.
     (void)__builtin_abs(-1);
 
     // Bare calls are allowed as long as they resolve to the correct namespace.
     strlen("world");
 
     // Disallow calling into functions in the global namespace.
     ::strlen("!");
 
+    // Allow calling into specific global functions (explained above)
+    ::malloc(10);
+
     } // namespace __llvm_libc

libc/src/__support/FPUtil/NearestIntegerOperations.h

template <typename F, typename I,
                                 cpp::IsIntegral<I>::Value,
                             int> = 0>
 static inline I roundedFloatToSignedInteger(F x) {
   constexpr I IntegerMin = (I(1) << (sizeof(I) * 8 - 1));
   constexpr I IntegerMax = -(IntegerMin + 1);
   FPBits<F> bits(x);
   auto setDomainErrorAndRaiseInvalid = []() {
 #if math_errhandling & MATH_ERRNO
-    errno = EDOM; // NOLINT
+    errno = EDOM;
 #endif
 #if math_errhandling & MATH_ERREXCEPT
     raiseExcept(FE_INVALID);
 #endif
   };
 
   if (bits.isInfOrNaN()) {
     setDomainErrorAndRaiseInvalid();

libc/src/__support/str_to_float.h

simpleDecimalConversion(const char *__restrict numStart,
 
   // If the exponent is too large and can't be represented in this size of
   // float, return inf.
   if (hpd.getDecimalPoint() > 0 &&
       exp10ToExp2(hpd.getDecimalPoint() - 1) >
           static_cast<int64_t>(fputil::FloatProperties<T>::exponentBias)) {
     *outputMantissa = 0;
     *outputExp2 = fputil::FPBits<T>::maxExponent;
-    errno = ERANGE; // NOLINT
+    errno = ERANGE;
     return;
   }
   // If the exponent is too small even for a subnormal, return 0.
   if (hpd.getDecimalPoint() < 0 &&
       exp10ToExp2(-hpd.getDecimalPoint()) >
           static_cast<int64_t>(fputil::FloatProperties<T>::exponentBias +
                                fputil::FloatProperties<T>::mantissaWidth)) {
     *outputMantissa = 0;
     *outputExp2 = 0;
-    errno = ERANGE; // NOLINT
+    errno = ERANGE;
     return;
   }
 
   // Right shift until the number is smaller than 1.
   while (hpd.getDecimalPoint() > 0) {
     int32_t shiftAmount = 0;
     if (hpd.getDecimalPoint() >= NUM_POWERS_OF_TWO) {
       shiftAmount = 60;
simpleDecimalConversion(const char *__restrict numStart,
 
   // Get the biased exponent
   exp2 += fputil::FloatProperties<T>::exponentBias;
 
   // Handle the exponent being too large (and return inf).
   if (exp2 >= fputil::FPBits<T>::maxExponent) {
     *outputMantissa = 0;
     *outputExp2 = fputil::FPBits<T>::maxExponent;
-    errno = ERANGE; // NOLINT
+    errno = ERANGE;
     return;
   }
 
   // Shift left to fill the mantissa
   hpd.shift(fputil::FloatProperties<T>::mantissaWidth);
   typename fputil::FPBits<T>::UIntType finalMantissa =
       hpd.roundToIntegerType<typename fputil::FPBits<T>::UIntType>();
 
simpleDecimalConversion(const char *__restrict numStart,
   // Check if rounding added a bit, and shift down if that's the case.
   if (finalMantissa == typename fputil::FPBits<T>::UIntType(2)
                            << fputil::FloatProperties<T>::mantissaWidth) {
     finalMantissa >>= 1;
     ++exp2;
   }
 
   if (exp2 == 0) {
-    errno = ERANGE; // NOLINT
+    errno = ERANGE;
   }
 
   *outputMantissa = finalMantissa;
   *outputExp2 = exp2;
 }
 
 // This class is used for templating the constants for Clinger's Fast Path,
 // described as a method of approximation in
decimalExpToFloat(typename fputil::FPBits<T>::UIntType mantissa, int32_t exp10,
                   uint32_t *outputExp2) {
   // If the exponent is too large and can't be represented in this size of
   // float, return inf. These bounds are very loose, but are mostly serving as a
   // first pass. Some close numbers getting through is okay.
   if (exp10 >
       static_cast<int64_t>(fputil::FloatProperties<T>::exponentBias) / 3) {
     *outputMantissa = 0;
     *outputExp2 = fputil::FPBits<T>::maxExponent;
-    errno = ERANGE; // NOLINT
+    errno = ERANGE;
     return;
   }
   // If the exponent is too small even for a subnormal, return 0.
   if (exp10 < 0 &&
       -static_cast<int64_t>(exp10) >
           static_cast<int64_t>(fputil::FloatProperties<T>::exponentBias +
                                fputil::FloatProperties<T>::mantissaWidth) /
               2) {
     *outputMantissa = 0;
     *outputExp2 = 0;
-    errno = ERANGE; // NOLINT
+    errno = ERANGE;
     return;
   }
 
   if (!truncated) {
     if (clingerFastPath<T>(mantissa, exp10, outputMantissa, outputExp2)) {
       return;
     }
   }
binaryExpToFloat(typename fputil::FPBits<T>::UIntType mantissa, int32_t exp2,
   // biasedExponent represents the biased exponent of the most significant bit.
   int32_t biasedExponent = exp2 + NUMBITS + fputil::FPBits<T>::exponentBias - 1;
 
   // Handle numbers that're too large and get squashed to inf
   if (biasedExponent >= INF_EXP) {
     // This indicates an overflow, so we make the result INF and set errno.
     *outputExp2 = (1 << fputil::FloatProperties<T>::exponentWidth) - 1;
     *outputMantissa = 0;
-    errno = ERANGE; // NOLINT
+    errno = ERANGE;
     return;
   }
 
   uint32_t amountToShiftRight =
       NUMBITS - fputil::FloatProperties<T>::mantissaWidth - 1;
 
   // Handle subnormals.
   if (biasedExponent <= 0) {
     amountToShiftRight += 1 - biasedExponent;
     biasedExponent = 0;
 
     if (amountToShiftRight > NUMBITS) {
       // Return 0 if the exponent is too small.
       *outputMantissa = 0;
       *outputExp2 = 0;
-      errno = ERANGE; // NOLINT
+      errno = ERANGE;
       return;
     }
   }
 
   BitsType roundBitMask = BitsType(1) << (amountToShiftRight - 1);
   BitsType stickyMask = roundBitMask - 1;
   bool roundBit = mantissa & roundBitMask;
   bool stickyBit = static_cast<bool>(mantissa & stickyMask) || truncated;
if (roundBit && (leastSignificantBit || stickyBit)) {
     ++mantissa;
   }
 
   if (mantissa > fputil::FloatProperties<T>::mantissaMask) {
     // Rounding causes the exponent to increase.
     ++biasedExponent;
 
     if (biasedExponent == INF_EXP) {
-      errno = ERANGE; // NOLINT
+      errno = ERANGE;
     }
   }
 
   if (biasedExponent == 0) {
-    errno = ERANGE; // NOLINT
+    errno = ERANGE;
   }
 
   *outputMantissa = mantissa & fputil::FloatProperties<T>::mantissaMask;
   *outputExp2 = biasedExponent;
 }
 
 // checks if the next 4 characters of the string pointer are the start of a
 // hexadecimal floating point number. Does not advance the string pointer.

libc/src/__support/str_to_integer.h

 template <class T>
 static inline T strtointeger(const char *__restrict src,
                              char **__restrict str_end, int base) {
   unsigned long long result = 0;
   bool is_number = false;
   const char *original_src = src;
 
   if (base < 0 || base == 1 || base > 36) {
-    errno = EINVAL; // NOLINT
+    errno = EINVAL;
     return 0;
   }
 
   src = first_non_whitespace(src);
 
   char result_sign = '+';
   if (*src == '+' || *src == '-') {
     result_sign = *src;
while (isalnum(*src)) {
 
     is_number = true;
     ++src;
 
     // If the number has already hit the maximum value for the current type then
     // the result cannot change, but we still need to advance src to the end of
     // the number.
     if (result == ABS_MAX) {
-      errno = ERANGE; // NOLINT
+      errno = ERANGE;
       continue;
     }
 
     if (result > ABS_MAX_DIV_BY_BASE) {
       result = ABS_MAX;
-      errno = ERANGE; // NOLINT
+      errno = ERANGE;
     } else {
       result = result * base;
     }
     if (result > ABS_MAX - cur_digit) {
       result = ABS_MAX;
-      errno = ERANGE; // NOLINT
+      errno = ERANGE;
     } else {
       result = result + cur_digit;
     }
   }
 
   if (str_end != nullptr)
     *str_end = const_cast<char *>(is_number ? src : original_src);
 

libc/src/math/generic/math_utils.h

 template <> struct XFlowValues<double> {
   static const double overflow_value;
   static const double underflow_value;
   static const double may_underflow_value;
 };
 
 template <typename T> static inline T with_errno(T x, int err) {
   if (math_errhandling & MATH_ERRNO)
-    errno = err; // NOLINT
+    errno = err;
   return x;
 }
 
 template <typename T> static inline void force_eval(T x) {
   volatile T y UNUSED = x;
 }
 
 template <typename T> static inline T opt_barrier(T x) {

libc/src/string/strdup.cpp

 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(char *, strdup, (const char *src)) {
   if (src == nullptr) {
     return nullptr;
   }
   size_t len = internal::string_length(src) + 1;
-  char *dest = reinterpret_cast<char *>(::malloc(len)); // NOLINT
+  char *dest = reinterpret_cast<char *>(::malloc(len));
   if (dest == nullptr) {
     return nullptr;
   }
   inline_memcpy(dest, src, len);
   return dest;
 }
 
 } // namespace __llvm_libc

libc/src/string/strndup.cpp

 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(char *, strndup, (const char *src, size_t size)) {
   if (src == nullptr)
     return nullptr;
   size_t len = internal::string_length(src);
   if (len > size)
     len = size;
-  char *dest = reinterpret_cast<char *>(::malloc(len + 1)); // NOLINT
+  char *dest = reinterpret_cast<char *>(::malloc(len + 1));
   if (dest == nullptr)
     return nullptr;
   inline_memcpy(dest, src, len + 1);
   dest[len] = '\0';
   return dest;
 }
 
 } // namespace __llvm_libc