[analyzer] Ignore annotations if func is inlined.

[aabbaabb]

Summary

When we annotating a function header so that it could be used by other TU, we also need to make sure the function is parsed correctly within the same TU.
So if we can find the function's implementation, ignore the annotations, otherwise, false positive would occur.
Move the escape by value case to post call and do not escape the handle if the function is inlined and we have analyzed the handle.

Tests: unit test.

[xazax.hun]

Thanks for working on this check! Please add the [analyzer] tag to the title of these patches.

Unfortunately, might be a hard problem to solve depending on what you want, see my comments inline.
The question is, do you actually need to skip annotation processing both in pre and postCall?

We usually check preconditions in pre, and apply effects from postconditions in post. So you might be
lucky, and it might be enough to only modify the checkPostCall. Do you observe any false positives
from checkPreCall?

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

312

The analyzer has complex logic to decide when to "inline" a function. In this context, inline means to analyze the body at this call site (not actual inlining). Unfortunately, the analyzer might not inline a function even when the body is available (e.g. when running out of some budget like max call stack). Moreover, it might inline functions without a body (see BodyFarm for details). The bottom line is that having a function body is not a good proxy to determine whether a function body will be analyzed or not. Fortunately, it is much easier in the post-call statement as we can observe after the fact whether inlining was done.

347

Here you can use C.wasInlined to achieve what you want. See my comment above why.

[xazax.hun]

LGTM, thanks!

[steakhal]

clang/test/Analysis/fuchsia_handle.cpp

352

Could we see which handle leaked?
Oh, I see, a is closed, so the only handle that could leak is b.
Before the patch, it was not reported, but now it is.

Am I right?

Regardless, IMO it would be cleaner to have some note about this in the expected message.

[aabbaabb]

clang/test/Analysis/fuchsia_handle.cpp

352

updated comment.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
         State = State->set<HStateMap>(Handle, HandleState::getEscaped());
     }
     C.addTransition(State);
     return;
   }
 
   for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
     if (Arg >= FuncDecl->getNumParams())
       break;
     const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
     SmallVector<SymbolRef, 1024> Handles =
         getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
 
     // Handled in checkPostCall.
     if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD) ||
         hasFuchsiaAttr<AcquireHandleAttr>(PVD))
       continue;
 
     for (SymbolRef Handle : Handles) {
       const HandleState *HState = State->get<HStateMap>(Handle);
       if (!HState || HState->isEscaped())
         continue;
 
       if (hasFuchsiaAttr<UseHandleAttr>(PVD) ||
           PVD->getType()->isIntegerType()) {
         if (HState->isReleased()) {
           reportUseAfterFree(Handle, Call.getArgSourceRange(Arg), C);
           return;
         }
       }
-      if (!hasFuchsiaAttr<UseHandleAttr>(PVD) &&
-          PVD->getType()->isIntegerType()) {
-        // Working around integer by-value escapes.
-        State = State->set<HStateMap>(Handle, HandleState::getEscaped());
-      }
     }
   }
   C.addTransition(State);
 }
 
 void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
                                          CheckerContext &C) const {
   const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
   if (!FuncDecl)
     return;
 
+  // If we analyzed the function body, then ignore the annotations.
+  if (C.wasInlined)
+    return;
+
   ProgramStateRef State = C.getState();
 
   std::vector<std::function<std::string(BugReport & BR)>> Notes;
   SymbolRef ResultSymbol = nullptr;
   if (const auto *TypeDefTy = FuncDecl->getReturnType()->getAs<TypedefType>())
     if (TypeDefTy->getDecl()->getName() == ErrorTypeName)
       ResultSymbol = Call.getReturnValue().getAsSymbol();
 
for (SymbolRef Handle : Handles) {
             OS << "Handle allocated through " << ParamDiagIdx
                << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
             return OS.str();
           } else
             return "";
         });
         State = State->set<HStateMap>(
             Handle, HandleState::getMaybeAllocated(ResultSymbol));
+      } else if (!hasFuchsiaAttr<UseHandleAttr>(PVD) &&
+                 PVD->getType()->isIntegerType()) {
+        // Working around integer by-value escapes.
+        // The by-value escape would not be captured in checkPointerEscape.
+        // If the function was not analyzed (otherwise wasInlined should be
+        // true) and there is no annotation on the handle, we assume the handle
+        // is escaped.
+        State = State->set<HStateMap>(Handle, HandleState::getEscaped());
       }
     }
   }
   const NoteTag *T = nullptr;
   if (!Notes.empty()) {
     T = C.getNoteTag([this, Notes{std::move(Notes)}](
                          PathSensitiveBugReport &BR) -> std::string {
       if (&BR.getBugType() != &UseAfterReleaseBugType &&

clang/test/Analysis/fuchsia_handle.cpp

void checkHandlePtrInStructureLeak() {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb); // expected-note {{Handle allocated through 3rd parameter}}
   HandlePtrStruct hs;
   hs.h = &sb;
   zx_handle_close(sa); // expected-warning {{Potential leak of handle}}
   // expected-note@-1 {{Potential leak of handle}}
 }
 
+// Assume this function's declaration that has the release annotation is in one
+// header file while its implementation is in another file. We have to annotate
+// the declaration because it might be used outside the TU.
+// We also want to make sure it is okay to call the function within the same TU.
+zx_status_t test_release_handle(zx_handle_t handle ZX_HANDLE_RELEASE) {
+  return zx_handle_close(handle);
+}
+
+void checkReleaseImplementedFunc() {
+  zx_handle_t a, b;
+  zx_channel_create(0, &a, &b);
+  zx_handle_close(a);
+  test_release_handle(b);
+}
+
+void use_handle(zx_handle_t handle) {
+  // Do nothing.
+}
+
+void test_call_by_value() {
+  zx_handle_t a, b;
+  zx_channel_create(0, &a, &b);
+  zx_handle_close(a);
+  use_handle(b);
+  zx_handle_close(b);
+}
+
+void test_call_by_value_leak() {
+  zx_handle_t a, b;
+  zx_channel_create(0, &a, &b); // expected-note {{Handle allocated through 3rd parameter}}
+  zx_handle_close(a);
+  // Here we are passing handle b as integer value to a function that could be
+  // analyzed by the analyzer, thus the handle should not be considered escaped.
+  // After the function 'use_handle', handle b is still tracked and should be
+  // reported leaked.
+  use_handle(b);
+} // expected-warning {{Potential leak of handle}}
+// expected-note@-1 {{Potential leak of handle}}
+
 // RAII
 
 template <typename T>
 struct HandleWrapper {
   ~HandleWrapper() { close(); }
   void close() {
     if (handle != ZX_HANDLE_INVALID)
       zx_handle_close(handle);