[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added reviewers: aaron.ballman, aaronpuchert.
aaronpuchert added a comment.

Thanks for working on this! Someone recently pointed out to me that we have a gap there.



================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1046
 def ThreadSafetyPrecise    : DiagGroup<"thread-safety-precise">;
 def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReturn     : DiagGroup<"thread-safety-return">;
----------------
Why not under `-Wthread-safety-reference`, as it's return-by-reference that you're warning on? This seems too small for a separate flag to me.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3805
 
+// Thread safety warnings on return
+def warn_guarded_return_by_reference : Warning<
----------------
Or do you expect more warnings on return?


================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2159-2161
+  // If returning by reference, add the return value to the set to check on
+  // function exit.
+  if (RetVal->isLValue()) {
----------------
Wouldn't it be more straightforward to check the actual return type? We have the `FunctionDecl` and could store it in `ThreadSafetyAnalyzer` instead of `CurrentMethod`.


================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2162
+  if (RetVal->isLValue()) {
+    Analyzer->ReturnValues.insert(RetVal);
+  }
----------------
You're presumably collecting them because automatic destructor calls are after `return` in the CFG, right?

If that's the case, can't we immediately check against the declared exit set? It should be known before we walk the CFG, unless I'm missing something.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153131/new/

https://reviews.llvm.org/D153131