llvm / llvm-project

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

[AST-Matchers] The nullPointerConstant matcher is problematic #87008

Open GKxxQAQ opened 5 months ago

GKxxQAQ commented 5 months ago

Current definition of clang::ast_matchers::nullPointerConstant (in clang/include/clang/ASTMatchers.h) is

AST_MATCHER_FUNCTION(internal::Matcher<Expr>, nullPointerConstant) {
  return anyOf(
      gnuNullExpr(), cxxNullPtrLiteralExpr(),
      integerLiteral(equals(0), hasParent(expr(hasType(pointerType())))));
}

The third argument to anyOf here is weird: As the C standard (C99, C11, C17) says (6.3.2.3 p3)

An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant.

So, if nullPointerConstant() is supposed to match any null pointer constant based on this definition, why is it restricted with hasParent(expr(hasType(pointerType())))? If it is only supposed to match null pointer constants that are really used as null pointers, it is still problematic:

int main() {
  int *p[1];
  p[0];
}

In this example, the integer literal 0 in p[0] is matched by expr(nullPointerConstant()), because its parent expression p[0] does have a pointer type.

llvmbot commented 5 months ago

@llvm/issue-subscribers-clang-frontend

Author: None (GKxxQAQ)

Current definition of `clang::ast_matchers::nullPointerConstant` (in `clang/include/clang/ASTMatchers.h`) is ```cpp AST_MATCHER_FUNCTION(internal::Matcher<Expr>, nullPointerConstant) { return anyOf( gnuNullExpr(), cxxNullPtrLiteralExpr(), integerLiteral(equals(0), hasParent(expr(hasType(pointerType()))))); } ``` The third argument to `anyOf` here is problematic, I think. Counterexample: ```cpp int main() { int *p[1]; p[0]; } ``` The integer literal `0` in `p[0]` is matched by `expr(nullPointerConstant())`.
Sirraide commented 5 months ago

In this example, the integer literal 0 in p[0] is matched by expr(nullPointerConstant()), because its parent expression p[0] does have a pointer type.

Yeah, this seems pretty wrong to me at least; I’m more familiar w/ C++ than C, but it would seem very strange to me candidly if the C standard really considered that 0 in p[0] to be a null pointer constant. I’m also not too familiar w/ AST matchers, but we should probably be able to check for an enclosing (possibly implicit) cast expression instead?

Sirraide commented 5 months ago

Godbolt link: https://godbolt.org/z/jaz4jYohe