Fix assertion failure "PathDiagnosticSpotPiece's must have a valid location." in ReturnPtrRange checker on builtin functions

[arseniy-sonar]

Summary

Fixes #55347
Builtin functions (such ast std::move, std::forward, std::as_const) have a body generated during the analysis not related to any source file so their statements have no valid source locations.
ReturnPtrRange checker should not report issues for these builtin functions because they only forward its parameter and do not create any new pointers.

[steakhal]

I already reviewed this internally. For me, it looks great.

@arseniy-sonar consider adding the Fixes #55347 marker to the summary, so the commit will auto-reference the fixed issue.
Also mention the commit author sign to which we should accredit this revision if accepted.

[NoQ]

LGTM, thanks!

This patch seems obvious but please upload with context next time (https://llvm.org/docs/Phabricator.html#phabricator-request-review-web).

clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp

48

I suspect that you might run into more similar problems with functions coming from "body farms". A direct check like

RetE->getBeginLoc().isValid()

might be more reliable.

You might need to check the entire range though. We probably need a unified solution for such checks, because many checkers end up implementing them.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp

48

We were actually considering it. We decided against it to keep the impact of this fix minimal.

That being said, I wonder if a similar check should be at some higher level API, lets say inside the emitReport.
That way no chevker would experience such crashes. WDYT?

[steakhal]

I think we should backport this to release/16.x.
Here is the ticket for it: https://github.com/llvm/llvm-project/issues/61115

clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp

 void ReturnPointerRangeChecker::checkPreStmt(const ReturnStmt *RS,
                                              CheckerContext &C) const {
   ProgramStateRef state = C.getState();
 
   const Expr *RetE = RS->getRetValue();
   if (!RetE)
     return;
 
+  // Skip "body farmed" functions.
+  if (RetE->getSourceRange().isInvalid())
+    return;
+
   SVal V = C.getSVal(RetE);
   const MemRegion *R = V.getAsRegion();
 
   const ElementRegion *ER = dyn_cast_or_null<ElementRegion>(R);
   if (!ER)
     return;
 
   DefinedOrUnknownSVal Idx = ER->getIndex().castAs<DefinedOrUnknownSVal>();
   // Zero index is always in bound, this also passes ElementRegions created for

clang/test/Analysis/return-ptr-range.cpp

if (I != 11) // expected-note{{Assuming 'I' is equal to 11}}
     return DataArr;
   return DataArr + I; // expected-warning{{Returned pointer value points outside the original object}}
                       // expected-note@-1{{Returned pointer value points outside the original object}}
                       // expected-note@-2{{Original object 'DataArr' is an array of 10 'test_array_of_struct::Data' objects, returned pointer points at index 11}}
 }
 
 }
 
+namespace std {
+// A builtin function with the body generated on the fly.
+template <typename T> T&& move(T &&) noexcept;
+} // namespace std
+
+char buf[2];
+
+void top() {
+  // see https://github.com/llvm/llvm-project/issues/55347
+  (void)std::move(*(buf + 3)); // no-crash
+}