[libc-commits] [PATCH] D77281: [libc] Add libc-tidy.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

In D77281#1982806 <https://reviews.llvm.org/D77281#1982806>, @sivachandra wrote:

> The TableGen part LGTM. I have left couple of comments about the clang-tidy part.
>
> I have a higher level question. Given our rule for include files, the check performed by the qualified call checker seems redundant to me. Am I right, or am I missing something?


I was indeed missing one particular case here. Our header file rules protect us from header file mixup. Likewise, our policy of enclosing everything within the `__llvm_libc` namespace protects us from symbol mixup with another libc. But, there is one case for which this patch gives us additional protection. Example:

  // some_entrypoint.cpp
  #include "include/string.h" // This makes the public strlen available
  
  namespace __llvm_libc {
  
  void LLVM_LIBC_ENTRYPOINT(some_entrypoint)(...) {
    // We want this to be disallowed.
    // Note that this strlen is not being picked from the system libc header
    // but is picked up from LLVM libc's public string.h header. This can
    // potentially lead to symbol mixup with another libc.
    ::strlen(...);
  }
  
  } // namespace __llvm_libc



================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:72
+
+    // Only look for libc public API function calls.
+    if (functions.find(funcDecl->getNameInfo().getAsString()) ==
----------------
Should we restrict to only entrypoints? Seems to me like we should do this for all function calls? That is, LLVM libc implementations (not redirectors) should only call functions declared in the namespace `__llvm_libc`.


================
Comment at: libc/utils/LibcTidy/tests/Test.cpp:20
+  // Disallow just plain calls.
+  libc_api_func();
+  // CHECK: :[[@LINE-1]]:3: error: libc_api_func must be called with qualified name '__llvm_libc'.
----------------
sivachandra wrote:
> This error is confusing. At least, it confused me. IIUC, an error will be reported even if the call were to be made in this fashion:
> 
> ```
> ::libc_api_func();
> ```
> 
> From the user's point of view, this is total fine: they are explicitly calling the global version so there should be no error. Even if we had a non-global namespace like this, an error will be reported:
> 
> ```
> namespace nn {
> void libc_api_func() {}
> }
> ...
> nn::libc_api_func();
> ```
> 
> Such uses cases are relevant in tests and I think we should allow them in tests. Also, did you intend to allow a use case like this:
> 
> ```
> namespace __llvm_libc {
> void some_func() {
>   libc_api_func(); // Unqualified call to libc_api_func.
> }
> }
> ```
I think this test is mostly correct. Few minor changes:
1. Enclose `Test` in the namespace `__llvm_libc`. 
2. The call at line 20 should not result in an error after that.
3. Add a call to `::libc_api_func` which should result in an error.
4. The error message in general should say something like, `error: call to {function} not declared in __llvm_libc namespace.` You can additionally point to the file:line of the function declaration to which the erroneous call resolves to.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77281/new/

https://reviews.llvm.org/D77281