[PATCH] D107078: [analyzer] Catch leaking stack addresses via stack variables

[Balázs Benics via Phabricator via cfe-commits]

steakhal planned changes to this revision.
steakhal marked an inline comment as done.
steakhal added a comment.

I'm going to check how the notes look like on real code.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:397-399
+      Report->addNote("The temporary object gets destroyed at the end of the "
+                      "full expression",
+                      L);
----------------
I'm actually not sure about this. We the `genName()` returns the appropriate `SourceRange` for `CXXTempObject`s, which will be added to the bug report regardless. That might be enough. I'm going to check that.



================
Comment at: clang/test/Analysis/copy-elision.cpp:400-419
+struct Foo {
+  Foo(Foo **q) {
+    *q = this;
+  }
+};
+
+Foo make1(Foo **r) {
----------------
I'm going to move this somewhere else.
Probably outside of the namespace `address_vector_tests` to the end of this file.


================
Comment at: clang/test/Analysis/copy-elision.cpp:195
+  // expected-warning at -1 {{Address of stack memory associated with local \
+variable 'c' is still referred to by the stack variable 'v' upon returning \
+to the caller}}
----------------
martong wrote:
> It would be useful to have a test with `// RUN:   -analyzer-output=text \` to make sure that we have a note placed for `v` at `consume(make3(v))`. Do we have such a note? This could be done perhaps in another test file.
I could do that, but we should consider the size of the test file.
The notes will duplicate each `expected-warning` and add even more besides those.

Should I add them to this file or to another test file covering the same?
If we decide to have that I think it would be valuable to do that in a follow-up patch IMO.
The size of this is already hitting the limit.


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

https://reviews.llvm.org/D107078