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

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

Thanks.



================
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">;
----------------
aaronpuchert wrote:
> 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.
The main reason it so that we provide a soft transition period for users: If we put that in `-Wthread-safety-reference`, we'll start breaking compile for people who use `-Werror`, while a separate flag allows a transition period where people opt into the new feature.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3805
 
+// Thread safety warnings on return
+def warn_guarded_return_by_reference : Warning<
----------------
aaronpuchert wrote:
> Or do you expect more warnings on return?
We could do pointers too, but arguably pointers and references are the same.


================
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()) {
----------------
aaronpuchert wrote:
> 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`.
Good point. I've also added better checking and diagnostics for `const` (shared) vs `mutable` (exclusive) locks, with more tests.


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

Exactly.

> 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.

Given how the code was written I was under the impression that we only knew the entry set after walking the whole CFG (we're getting `ExpectedExitSet` after we walk the CFG). But now I see that we're actually adressing the entry blok beforehand. Thanks for  the suggestion, this makes the code much simpler indeed !




================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2165
+
+  VisitorBase::VisitReturnStmt(S);
+}
----------------
aaronpuchert wrote:
> Also wondering why we're doing this—no other visitor function seems to bother the `VisitorBase = ConstStmtVisitor<BuildLockset>`. Are these not just empty fallbacks?
The base code is hard to read because i't full of macros, but it looks like it't probably empty indeed - done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153131