[PATCH] D135682: Fix false positive related to handling of [[noreturn]] function pointers

[Arseniy Zaostrovnykh via Phabricator via cfe-commits]

arseniy-sonar created this revision.
arseniy-sonar added reviewers: xazax.hun, martong, NoQ, Szelethus, steakhal.
arseniy-sonar added a project: clang.
Herald added a subscriber: rnkovacs.
Herald added a project: All.
arseniy-sonar requested review of this revision.
Herald added a subscriber: cfe-commits.

Before this change, the NoReturnFunctionChecker was missing function pointers with a [[noreturn]] attribute, while CFG was constructed taking that into account, which lead CSA to take impossible paths. The reason was that the NoReturnFunctionChecker was looking for the attribute in the type of the entire call expression rather than the type of the function being called.

This change makes the [[noreturn]] attribute of a function pointer visible to NoReturnFunctionChecker. This leads to a more coherent behavior of the CSA on the AST involving


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135682

Files:
  clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
  clang/test/Analysis/no-return.c


Index: clang/test/Analysis/no-return.c
===================================================================
--- /dev/null
+++ clang/test/Analysis/no-return.c
@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+typedef void(fatal_fun)() __attribute__((__noreturn__));
+fatal_fun* fatal_fptr;
+void fatal_decl() __attribute__((__noreturn__));
+
+int rng();
+
+/// This code calls a [[noreturn]] function pointer, which used to be handled
+/// inconsistently between AST builder and CSA.
+/// In the result, CSA produces a path where this function returns non-0.
+int return_zero_or_abort_by_fnptr() {
+  if (rng()) fatal_fptr();
+  return 0;
+}
+
+/// This function calls a [[noreturn]] function.
+/// If it does return, it always returns 0.
+int return_zero_or_abort_by_direct_fun() {
+  if (rng()) fatal_decl();
+  return 0;
+}
+
+/// Trigger a division by zero issue depending on the return value
+/// of the called functions.
+int caller() {
+  int x = 0;
+  // The following if branches must never be taken.
+  if (return_zero_or_abort_by_fnptr())
+    return 1 / x; // no-warning: Dead code.
+  if (return_zero_or_abort_by_direct_fun())
+    return 1 / x; // no-warning: Dead code.
+
+  // Make sure the warning is still reported when viable.
+  return 1 / x; // expected-warning {{Division by zero}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
@@ -44,9 +44,11 @@
   if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CE.getDecl()))
     BuildSinks = FD->hasAttr<AnalyzerNoReturnAttr>() || FD->isNoReturn();
 
-  const Expr *Callee = CE.getOriginExpr();
-  if (!BuildSinks && Callee)
-    BuildSinks = getFunctionExtInfo(Callee->getType()).getNoReturn();
+  if (const CallExpr* CExpr = dyn_cast_or_null<CallExpr>(CE.getOriginExpr());
+      CExpr && !BuildSinks) {
+    if (const Expr *C = CExpr->getCallee())
+      BuildSinks = getFunctionExtInfo(C->getType()).getNoReturn();
+  }
 
   if (!BuildSinks && CE.isGlobalCFunction()) {
     if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D135682.466827.patch
Type: text/x-patch
Size: 2267 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221011/2919dba6/attachment-0001.bin>