[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 3 inline comments as done.
xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:261
+
+  // TODO: do we want to emit notes for escapes?
+  //       do we want to emit notes for for error checks? I.e. on which branch
----------------
NoQ wrote:
> Will we ever emit a report against an escaped symbol? If not, there will never be a report to attach the note to.
I think there might be cases where we go from an escaped symbol to released and so on. But you are right, it would not contribute to understanding the actual bug report.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:262-263
+  // TODO: do we want to emit notes for escapes?
+  //       do we want to emit notes for for error checks? I.e. on which branch
+  //       do we consider the acquire/release success or fail.
+
----------------
NoQ wrote:
> If the function is implemented as an eager state split, having a note is great and easy to implement.
> 
> If it's implemented as a Schrödinger state split, then i think we should still emit a note at the collapse point (especially given that it may happen in a deeper stack frame than the call that produced the symbol). But this makes me recognize the need for note tags in `evalAssume`.
I agree.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:521
+  if (Type.isSuppressOnSink()) {
+    const ExplodedNode *AcquireNode = getAcquireSite(ErrorNode, Sym, C);
+    if (AcquireNode) {
----------------
NoQ wrote:
> I wonder how terrible would it be to allow bug visitors (or note tags, which are just an "API sugar" for bug visitors) to mutate the uniqueing location. Because such information is naturally available in the visitor.
> 
> It most likely won't work because the information is necessary before the visitors are run. But it would have been nice though, so i feel as if we're missing an opportunity here.
On one hand, it would be nice :) On the other hand we could have more than one visitor and it would be a hell to debug when the visitors disagree an the uniqueing location. If we can come up with an API that is not that easy to misuse, I am in favor of a change like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70725