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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:123
         state = state->BindExpr(B, LCtx, Result);
       }
 
----------------
NoQ wrote:
> 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.
Oh, I really wanted to see them in the Environment. Thanks for the clarification!


================
Comment at: clang/test/Analysis/symbol-escape.cpp:5-6
 
+// expected-no-diagnostics
+
 #include <stdint.h>
----------------
NoQ wrote:
> 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.
Lesson learned, thanks for the idea!


================
Comment at: clang/test/Analysis/symbol-escape.cpp:7
+
 #include <stdint.h>
 
----------------
NoQ wrote:
> 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.
Hm, I hope that `malloc.c` one works.


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

https://reviews.llvm.org/D63720