llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.82k stars 11.91k forks source link

`-Wzero-as-null-pointer-constant` not reported with clang-tidy #73449

Open firewave opened 11 months ago

firewave commented 11 months ago
#include <cstddef>

void test()
{
        int* p = NULL;
}
$ clang -Wzero-as-null-pointer-constant test.cpp
test.cpp:5:11: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
        int* p = NULL;
                 ^~~~
                 nullptr
/usr/lib/clang/16/include/stddef.h:84:18: note: expanded from macro 'NULL'
#    define NULL __null
                 ^
$ clang-tidy -checks='*' -extra-arg=-Wzero-as-null-pointer-constant test.cpp
2335 warnings generated.
/home/user/test.cpp:1:1: warning: system include cstddef not allowed [llvmlibc-restrict-system-libc-headers]
#include <cstddef>
^~~~~~~~~~~~~~~~~~
/home/user/test.cpp:3:6: warning: declaration must be declared within the '__llvm_libc' namespace [llvmlibc-implementation-in-namespace]
void test()
     ^
/home/user/test.cpp:5:7: warning: variable name 'p' is too short, expected at least 3 characters [readability-identifier-length]
        int* p = NULL;
             ^
/home/user/test.cpp:5:11: warning: use nullptr [modernize-use-nullptr]
        int* p = NULL;
                 ^~~~
                 nullptr
Suppressed 2331 warnings (2331 in non-user code).

Strangely in Godbolt the warning is not reported with the compiler: https://godbolt.org/z/P489KenGx (updated as it lacked the warning flag for the compiler)

True, modernize-use-nullptr and -Wzero-as-null-pointer-constant are checking for the same thing but I did not expect that the compiler warning is disabled. I wonder if other warnings are being suppressed as well. I have seen the same "issue" for the same line for other checks in the past.

In our case we have modernize-use-nullptr disabled since it duplicates the compiler warning and we use the clang-tidy step in the CI as the step which is supposed to fail on compiler warnings as well.

PiotrZSL commented 11 months ago

First:

FunctionDecl 0x563e56499610 </root/1.cpp:3:1, line:6:1> line:3:6 test 'void ()'
  `-CompoundStmt 0x563e564997b8 <line:4:1, line:6:1>
    `-DeclStmt 0x563e564997a0 <line:5:9, col:22>
      `-VarDecl 0x563e56499708 <col:9, /usr/lib/llvm-18/lib/clang/18/include/__stddef_null.h:21:14> /root/1.cpp:5:14 p 'int *' cinit
        `-ImplicitCastExpr 0x563e56499788 </usr/lib/llvm-18/lib/clang/18/include/__stddef_null.h:21:14> 'int *' <NullToPointer>
          `-GNUNullExpr 0x563e56499770 <col:14> 'long'

In release notes you may find:

- ``-Wzero-as-null-pointer-constant`` diagnostic is no longer emitted when using ``__null``
 (or, more commonly, ``NULL`` when the platform defines it as ``__null``) to be more consistent
 with GCC.

This change were introduced with clang-18 about month ago by cd29e19e9476e221015d895618512717d872a83b.

But if you write code like this:

void test()
{
  int* a = 0;
}

clang-tidy-18 1.cpp -extra-arg=-Wzero-as-null-pointer-constant
1.cpp:3:12: warning: zero as null pointer constant [clang-diagnostic-zero-as-null-pointer-constant]
    3 |   int* a = 0;

Then issue is detected. I would assume that version of clang and clang-tidy does not match on Compiler Explorer. Please check on your end.

firewave commented 11 months ago

Thanks for the thorough analysis. I will re-enabled the modernize-use-nullptr check on our end.

This change were introduced with clang-18 about month ago by cd29e19.

That would explain the results from godbolt (I updated the link as it was lacking the warning flag for the compiler). But the systems I experienced this on are using LLVM 16 and 17.

Is there some debug output from the clang-tidy execution I can provide?

PiotrZSL commented 11 months ago

Looks on clang-tidy-17 this warning is emitted but only if -system-headers is used. This is because warning point to location under macro, and due to that it's considered a system header warning.

1.cpp:5:13: warning: zero as null pointer constant [clang-diagnostic-zero-as-null-pointer-constant]
     5 |    int* p = NULL;
        |            ^
  /usr/lib/llvm-17/lib/clang/17/include/stddef.h:84:18: note: expanded from macro 'NULL'
     84 | #    define NULL __null

Looks like this behavior changed in clang-tidy 14 by this commit: 670de10f9deaa83f4d1db6e793c74cbfd18c65c1. For me probably better option would be something like -system-macros, but as now this warning wont be reported anyway for this code, I do not see issue here.

firewave commented 11 months ago

Thanks again.

This appears a bit similar to #73163 which is about warnings from templates in system headers (but with clang and not clang-tidy).

I do not see issue here

The issue is that clang and clang-tidy behave differently in terms of reporting the compiler warning. This means that clang-tidy cannot be used to assure that code will compile without errors. We did this in our CI so we do not require two different jobs which will process the same files and provide essentially the same output.

I do get why those warnings are being suppressed as I have dealt with such "unfixable" warnings from system headers in the past. But it is highly dependent on the macro in question. I do appreciate it being consistent within clang-tidy but unfortunately that is inconsistent in terms of comparing it with the compiler behavior.

In this case it is unfortunate that with the change to the compiler this no longer exists but I assume it is possible to find another case with another warning and possibly even a more severe issue.