[PATCH] D63720: [analyzer] ExprEngine: Escape pointers in bitwise operations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:123
         state = state->BindExpr(B, LCtx, Result);
       }
 
----------------
Charusso wrote:
> I have seen we are producing tons of Unknowns and I am still not sure where they go, but even if I remove that condition these Unknowns will not shown up in the ExplodedGraph. I believe that should be the correct behavior.
Unknowns don't show up in the Environment, but they do show up in the Store.


================
Comment at: clang/test/Analysis/symbol-escape.cpp:5-6
 
+// expected-no-diagnostics
+
 #include <stdint.h>
----------------
Charusso wrote:
> NoQ wrote:
> > I think something went wrong while uploading the patch. This diff should add this whole test file, not update it.
> This is a test-driven-development-driven patch, first make the test fail, then make it pass.
Sounds nice in your local git but for phabricator it's definitely better to upload the exact thing that you're going to commit. It is assumed that the tests would fail without the patch and will pass with the patch, and the information on how exactly did the tests fail previously would look nice in the description. Sometimes some (but usually not all) of the tests are added just because they were discovered to be nice tests related to the patch but not because they failed before the patch, which is also fine and should ideally be mentioned in the description.


================
Comment at: clang/test/Analysis/symbol-escape.cpp:7
+
 #include <stdint.h>
 
----------------
Charusso wrote:
> NoQ wrote:
> > Relying on everybody's system headers is super flaky, don't do this in tests.
> > 
> > Please define stuff that you use directly like other tests do:
> > ```lang=c++
> > typedef unsigned __INTPTR_TYPE__ uintptr_t;
> > ```
> It is a predefined header in Modules so Windows will not break.
> 
> Please note that only one of the test files (malloc.c) use that definition, most of them using that I have used in D62926, so I believe this header is the correct one as it is in four tests and Windows-approved by D62926.
Mmm, ok, if it's an internal file then i guess it might be non-flaky. But even then, if it's something more complicated than the definition of `uintptr_t`, then your test will be affected by the contents of this file, which may change. I'd slightly prefer to reduce the amount of such stuff in our tests simply for keeping them minimal.


================
Comment at: clang/test/Analysis/symbol-escape.cpp:15
                                ~static_cast<uintptr_t>(0x1)) |
                               (reinterpret_cast<uintptr_t>(Bar) & 0x1));
   (void)Bar;
----------------
Charusso wrote:
> NoQ wrote:
> > `Bar` is reduced to one bit here. It's a legit leak. I think you meant to swap `Foo` and `Bar`.
> I assumed that we are doing our low-bits magic, where we have two identical objects. The SymRegion sets up the HeapSymRegion's property, where no leak happen. We just could change the first low-bit and set up some crazy flag, like `isChecked()`, so we do not lost the tracking. Also we have four bits to change, so there is no edge case going on.
I guess just flip a single bit like i did in my test?


================
Comment at: clang/test/Analysis/symbol-escape.cpp:12
 
 C *payload(C *Foo) {
   C *Bar = new C();
----------------
Let's make fancier names, like, dunno, `test_escape_into_bitwise_ops` and `test_indirect_escape_into_bitwise_ops`.


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

https://reviews.llvm.org/D63720