[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thank you guys for investigating it!

In D69726#2671734 , @vsavchenko wrote:

> 2. The analyzer doesn't explain why it thinks that `guc_malloc` returns null 
> pointer.  I find it alarming that it might assume it for all the wrong 
> reasons.



In D69726#2673178 , @NoQ wrote:

> We should still investigate the tracking bug though, i.e. why path in 
> `guc_malloc()` isn't explained.

What I have seen back in the days is that: uninitialized variables and 
variables storing `NULL` are not attached to regions so we cannot map notes to 
their origin region and we cannot track them. That is where 
`trackExpressionValue()` tries to attach notes based on changes in the Store 
and with full of heuristics. The nature of heuristics and the fight of 
note-creation and note-suppression Reporters what you see.

If we would prioritize to massage the `trackExpressionValue()` framework, count 
me in: I have half year of pai- programming in it, but I am out of the office.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Here's a reduced repro - a file that has different behavior before and after 
the patch (sorry, not perfectly reduced, my creduce is broken again):

  // RUN: clang --analyze %s
  typedef Oid;
  typedef Pointer;
  typedef text;
  errstart(int elevel, filename, lineno, funcname, domain);
  typedef Datum;
  typedef struct {
Oid elemtype;
  } ArrayType;
  
  *guc_malloc(elevel, size) {
void *data;
data = malloc(size);
if (data == (0))
  (errstart(elevel, "c", 3298, __func__, (0))
   ? ((errcode('0') & 0x3F) < 24)
   : 0);
return (errstart(elevel, "c", 3324, __func__, (0)) ? ((errcode(((24) : 
0);
  }
  
  ParseLongOption(*string, *name, value) {
*name = guc_malloc(21, 1);
__builtin___strlcpy_chk(*name, string, 1, string[1]);
  }
  
  ProcessGUCArray(ArrayType *array, action) {
int i = 0;
{ (array)->elemtype) = 25)), "c", 7599); }
for (i = i + 1; char *)(array)) + sizeof(ArrayType)))[0];) {
  Datum d;
  char s;
  char name;
  char value;
  d = array_ref();
  s = text_to_cstring((text)((Pointer)(d)));
  ParseLongOption(s, , );
(errstart(19, "c", 7629, __func__, (0)) ? ((errcode(), errmsg(name)))
: 0);
}
  }

It's most likely some budget heuristic acting up. By slightly changing stuff 
that has shouldn't have any effect you can easily eliminate the regression or 
even reverse it (so that the warning appeared before the patch but not after 
the patch). The patch definitely changes modeling so it's possible that it 
causes budgets to be spent differently.

One particular thing i noticed about the behavior after the patch (by observing 
exploded graph topologies) is that it causes states to be //merged// more 
often. Which is expected because where previously we've created a new extent 
symbol and added a  new constraint, currently we simply re-use the existing 
symbol. This causes 10% improvement in the number of generated nodes. I also 
didn't immediately notice any unintended state splits.

I think we should indeed move on. I'm curious which specific budget do we hit 
but this patch definitely didn't introduce the root cause of the problem, only 
accidentally surfaced it. We should still investigate the tracking bug though, 
i.e. why path in `guc_malloc()` isn't explained.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D69726#2671611 , @Charusso wrote:

> In D69726#2671526 , @vsavchenko 
> wrote:
>
>> @Charusso 
>> It looks like this patch introduced a some weird false positive on 
>> PostgreSQL F16161734: report-guc.c-ParseLongOption-13-1.html 
>> 
>> I'll try to look at it myself and minimize it, but maybe you can get an idea 
>> from a full report.
>
> Could you supply us with all the divergence please? If there is only one 
> case, may we can ignore it by appending it to @NoQ's 
> `extremely-weird-bugs.txt` so people can focus on more important stuff.
>
> What I see in the bug report is that: `line 3290 - guc_malloc()` returns null 
> and it is a true positive.



1. It's the only bug in divergence.
2. The analyzer doesn't explain why it thinks that `guc_malloc` returns null 
pointer.  I find it alarming that it might assume it for all the wrong reasons.
3. It is a false positive because `guc_malloc` is called with `elevel == 
FATAL`, which causes `ereport` to fail the whole execution.

I believe that this problem is important because it is definitely a regression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D69726#2671526 , @vsavchenko wrote:

> @Charusso 
> It looks like this patch introduced a some weird false positive on PostgreSQL 
> F16161734: report-guc.c-ParseLongOption-13-1.html 
> 
> I'll try to look at it myself and minimize it, but maybe you can get an idea 
> from a full report.

Could you supply us with all the divergence please? If there is only one case, 
may we can ignore it by appending it to @NoQ's `extremely-weird-bugs.txt` so 
people can focus on more important stuff.

What I see in the bug report is that: `line 3290 - guc_malloc()` returns null 
and it is a true positive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

@Charusso 
It looks like this patch introduced a some weird false positive on PostgreSQL 
F16161734: report-guc.c-ParseLongOption-13-1.html 

I'll try to look at it myself and minimize it, but maybe you can get an idea 
from a full report.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Cool, thanks!

Debug facility NFC: 
https://reviews.llvm.org/rG89d210fe1a7a1c6cbf926df0595b6f107bc491d5
`size` -> `extent` conversion: 
https://reviews.llvm.org/rG9b3df78b4c2ab7a7063e532165492e1ffa38d401




Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:294
+
+  SmallString<128> Msg;
+  llvm::raw_svector_ostream Out(Msg);

NoQ wrote:
> 128 is too much imho :)
I am totally fine with 128, but let us use 64 then:
https://developer.apple.com/documentation/authenticationservices/asauthorizationcontrollerpresentationcontextproviding


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Charusso marked an inline comment as done.
Closed by commit rGdf64f471d1e2: [analyzer] DynamicSize: Store the dynamic size 
(authored by Charusso).

Changed prior to commit:
  https://reviews.llvm.org/D69726?vs=319940=335284#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69726

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp

Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -54,7 +54,7 @@
   int *x = new int[ext];
   clang_analyzer_explain(x); // expected-warning-re^pointer to element of type 'int' with index 0 of heap segment that starts at symbol of type 'int \*' conjured at statement 'new int \[ext\]'$
   // Sic! What gets computed is the extent of the element-region.
-  clang_analyzer_explain(clang_analyzer_getExtent(x)); // expected-warning-re^signed 32-bit integer '4'$
+  clang_analyzer_explain(clang_analyzer_getExtent(x)); // expected-warning-re^\(argument 'ext'\) \* 4$
   delete[] x;
 }
 
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -28,6 +28,7 @@
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
@@ -729,13 +730,6 @@
 // MemRegionManager methods.
 //===--===//
 
-static DefinedOrUnknownSVal getTypeSize(QualType Ty, ASTContext ,
-  SValBuilder ) {
-  CharUnits Size = Ctx.getTypeSizeInChars(Ty);
-  QualType SizeTy = SVB.getArrayIndexType();
-  return SVB.makeIntVal(Size.getQuantity(), SizeTy);
-}
-
 DefinedOrUnknownSVal MemRegionManager::getStaticSize(const MemRegion *MR,
  SValBuilder ) const {
   const auto *SR = cast(MR);
@@ -766,7 +760,7 @@
 if (Ty->isIncompleteType())
   return UnknownVal();
 
-return getTypeSize(Ty, Ctx, SVB);
+return getElementSize(Ty, SVB);
   }
   case MemRegion::FieldRegionKind: {
 // Force callers to deal with bitfields explicitly.
@@ -774,7 +768,7 @@
   return UnknownVal();
 
 QualType Ty = cast(SR)->getDesugaredValueType(Ctx);
-DefinedOrUnknownSVal Size = getTypeSize(Ty, Ctx, SVB);
+DefinedOrUnknownSVal Size = getElementSize(Ty, SVB);
 
 // A zero-length array at the end of a struct often stands for dynamically
 // allocated extra memory.
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -18,6 +18,7 @@
 #include "clang/Analysis/ConstructionContext.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
@@ -689,16 +690,30 @@
 
 // See if we need to conjure a heap pointer instead of
 // a regular unknown pointer.
-bool IsHeapPointer = false;
-if (const auto *CNE = dyn_cast(E))
-  if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
-// FIXME: Delegate this to evalCall in MallocChecker?
-IsHeapPointer = true;
+const auto *CNE = dyn_cast(E);
+if (CNE && CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+  R = svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count);
+  const MemRegion *MR = R.getAsRegion()->StripCasts();
+
+  // Store the extent of the allocated object(s).
+  SVal ElementCount;
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr)) {
+ElementCount = State->getSVal(SizeExpr, LCtx);
+  } else {
+ElementCount = svalBuilder.makeIntVal(1, /*IsUnsigned=*/true);
   

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

I don't see any further bugs so I think this is good to go!

I think you should separate out the NFC / debugging facility changes before 
committing.




Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:294
+
+  SmallString<128> Msg;
+  llvm::raw_svector_ostream Out(Msg);

128 is too much imho :)


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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

@NoQ, could you upstream it and move this idea forward please?


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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a reviewer: vsavchenko.
Charusso added a comment.

Hey, I am back.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44
+/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR.
+ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR,
+   const CXXNewExpr *NE,
+   const LocationContext *LCtx, SValBuilder );

Charusso wrote:
> NoQ wrote:
> > This function is probably going to be used exactly once in the whole code. 
> > There's no need to turn it into a public API.
> It is being used 3 times already, so I believe it is a cool API.
Let us remove the `setDynamicSize(CXXNewExpr)` API from now on (was not that 
cool). This function will reside inside `ExprEngine::bindReturnValue()` which 
is the second phase of evaluating operator new: 
https://reviews.llvm.org/D40560#961694



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311
+
+  QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext())
+: CE->getArg(0)->IgnoreParenImpCasts()->getType();
+

Charusso wrote:
> NoQ wrote:
> > How is this better than `getValueType()`? Are you sure you're not getting a 
> > pointer type instead in the `!TVR` case? I.e., can you test this on a 
> > non-heap `SymRegion`?
> > How is this better than getValueType()?
> Consistency. We get the static ~~size~~ extent by getting the desugared type 
> which most likely just an extra overhead.
> 
> > Are you sure you're not getting a pointer type instead in the !TVR case? 
> > I.e., can you test this on a non-heap SymRegion?
> The issue was the `var_region_simple_ptr` test: we need to use regex for 
> checking after the `}}` token where `/ 4` is the correct result. I did not 
> really get why the test pass.
Added a conjured `SymbolicRegion` test called `user_defined_new` and an 
assertion.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1388
- ->castAs()
- ->StripCasts()
- ->castAs();

NoQ wrote:
> What happened to this `StripCasts()`? I don't see it in the after-code and I 
> vaguely remember having to be very careful with it.
I have reimplemented it by a mistake with the `getSuperRegion(MR)` helper 
routine. From now I will use `StripCasts()` instead. Nice catch!



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:29
+/// Helper to bypass the top-level ElementRegion of \p MR.
+static const MemRegion *getSuperRegion(const MemRegion *MR) {
+  assert(MR);

Szelethus wrote:
> `getSuperRegionIfElementRegion`?
`getSuperRegion(MR)` was just a plain helper routine not restricted to return 
iff `MR` is an `ElementRegion`. It obtains the immediate region which we can 
measure in size, but it turns out the same idea as `MemRegion::StripCasts()`, 
so I have removed it.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:36-44
 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,
 SValBuilder ) {
+  MR = getSuperRegion(MR);
+
+  if (const DefinedOrUnknownSVal *Size = State->get(MR))
+return *Size;
+

Szelethus wrote:
> Well hold on for a second. Does this mean that we legit resolved 
> `getDynamicSize` to `getStaticSize`??? That's crazy! I've been looking around 
> in the code for how does your patch interact with our current dynamic size 
> modeling, but I guess I never really realized that there is no such a thing. 
> Odd to not even see a FIXME around.
I wanted to upstream both of the patches, but this one became dead:

[analyzer] DynamicSize: Remove 'getExtent()' from regions
Oct 29 2019, 01:43

[analyzer] DynamicSize: Store the dynamic size
Nov 1 2019, 19:57

Sorry for the inconvenience.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:80-82
+  /* FIXME: Make this work.
+  if (const auto CI = Size.getAs())
+assert(CI->getValue().isUnsigned());*/

Szelethus wrote:
> Make this work as is "make sure that checkers actually obey this 
> precondition" or does the assert just not work as-is? Could you specify this 
> comment?
This assertion was about the signedness, because I have seen something like 
`unsigned(42) == signed(42)` did not evaluate to true, and I really wanted to 
make the size equality to be true in such value-equality cases. I have added a 
test (signedness_equality) instead, which is working fine now (I hope, because 
I am not sure what was the original behavior).



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:779-780
 
   bool IsStandardGlobalOpNewFunction =
   FD->isReplaceableGlobalAllocationFunction();
 

Szelethus wrote:
> I wonder what the rationale was here. Why do we explicitly 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 319940.
Charusso marked 6 inline comments as done.
Charusso added a comment.

- Fix everything.


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

https://reviews.llvm.org/D69726

Files:
  clang/docs/analyzer/developer-docs/DebugChecks.rst
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/memory-model.cpp

Index: clang/test/Analysis/memory-model.cpp
===
--- /dev/null
+++ clang/test/Analysis/memory-model.cpp
@@ -0,0 +1,157 @@
+// RUN: %clang_analyze_cc1 -std=c++20 \
+// RUN:  -analyzer-checker=core,unix,cplusplus,debug.ExprInspection \
+// RUN:  -triple x86_64-unknown-linux-gnu \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *realloc(void *ptr, size_t size);
+void *calloc(size_t number, size_t size);
+void free(void *);
+
+struct S {
+  int f;
+};
+
+void clang_analyzer_dump(int);
+void clang_analyzer_dump(const void *);
+void clang_analyzer_dumpExtent(int);
+void clang_analyzer_dumpExtent(const void *);
+void clang_analyzer_dumpElementCount(int);
+void clang_analyzer_dumpElementCount(const void *);
+
+int clang_analyzer_getExtent(void *);
+void clang_analyzer_eval(bool);
+
+void var_simple_ref() {
+  int a = 13;
+  clang_analyzer_dump(); // expected-warning {{a}}
+  clang_analyzer_dumpExtent();   // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void var_simple_ptr(int *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning {{(extent_$1{SymRegion{reg_$0}}) / 4}}
+}
+
+void var_array() {
+  int a[] = {1, 2, 3};
+  clang_analyzer_dump(a); // expected-warning {{Element{a,0 S64b,int}}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void string() {
+  clang_analyzer_dump("foo"); // expected-warning {{Element{"foo",0 S64b,char}}}
+  clang_analyzer_dumpExtent("foo");   // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount("foo"); // expected-warning {{4 S64b}}
+}
+
+void struct_simple_ptr(S *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning {{(extent_$1{SymRegion{reg_$0}}) / 4}}
+}
+
+void field_ref(S a) {
+  clang_analyzer_dump(); // expected-warning {{a.f}}
+  clang_analyzer_dumpExtent();   // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void field_ptr(S *a) {
+  clang_analyzer_dump(>f); // expected-warning {{SymRegion{reg_$0}.f}}
+  clang_analyzer_dumpExtent(>f);   // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(>f); // expected-warning {{1 S64b}}
+}
+
+void symbolic_array() {
+  int *a = new int[3];
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] a;
+}
+
+void symbolic_placement_new() {
+  char *buf = new char[sizeof(int) * 3];
+  int *a = new (buf) int(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] buf;
+}
+
+void symbolic_malloc() {
+  int *a = (int *)malloc(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  free(a);
+}
+
+void symbolic_alloca() {
+  int *a = (int *)alloca(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D69726#2471657 , @vabridgers wrote:

> I was referred to this patch from https://reviews.llvm.org/D86743. I pulled 
> this patch under review, brought it up to date and pushed to github at 
> https://github.com/vabridgers/llvm-project-dev.git, branch: vla-fam-fixes. 
> Everything seems ok on this branch (LITs pass,  reproducers from 
> https://bugs.llvm.org/show_bug.cgi?id=47272 and 
> https://bugs.llvm.org/show_bug.cgi?id=28450 no longer crash). I can continue 
> and push a change to Phabricator for review, or @charusso and/or @balazske 
> could finish this? I didn't want to just push an update without asking first 
> :/  Cheers!

Hi Vince and Artem,

I agree that this is an important patch. Unfortunately it got stuck. I see two 
possible solutions:

1. Vince commandeers this patch. I am not sure about the policy of 
commandeering a patch and that might be too intrusive.
2. Vince opens a new patch (a copy of this) and gives credit to @Charusso when 
he commits. I prefer this.


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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added subscribers: balazske, vabridgers.
vabridgers added a comment.

I was referred to this patch from https://reviews.llvm.org/D86743. I pulled 
this patch under review, brought it up to date and pushed to github at 
https://github.com/vabridgers/llvm-project-dev.git, branch: vla-fam-fixes. 
Everything seems ok on this branch (LITs pass,  reproducers from 
https://bugs.llvm.org/show_bug.cgi?id=47272 and 
https://bugs.llvm.org/show_bug.cgi?id=28450 no longer crash). I can continue 
and push a change to Phabricator for review, or @charusso and/or @balazske 
could finish this? I didn't want to just push an update without asking first :/ 
 Cheers!


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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thank you for reminding me of this patch. I still think it's a pretty important 
patch and i'm interested in getting it landed.




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1388
- ->castAs()
- ->StripCasts()
- ->castAs();

What happened to this `StripCasts()`? I don't see it in the after-code and I 
vaguely remember having to be very careful with it.


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

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

While this patch spans across a lot of files, it is actually rather 
straightforward. I'm stunned to see that we never really had a proper dynamic 
size support, especially that this patch has been out there for a good long 
time :) Oh well, better learn that now than never.

There sure is some rebasing nightmare waiting for you in `VLASizeChecker`, and 
I honestly lack the confidence to accept a patch with so much SVal-smithing 
going on. With that said, I do like the overall direction.




Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:29
+/// Helper to bypass the top-level ElementRegion of \p MR.
+static const MemRegion *getSuperRegion(const MemRegion *MR) {
+  assert(MR);

`getSuperRegionIfElementRegion`?



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:36-44
 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,
 SValBuilder ) {
+  MR = getSuperRegion(MR);
+
+  if (const DefinedOrUnknownSVal *Size = State->get(MR))
+return *Size;
+

Well hold on for a second. Does this mean that we legit resolved 
`getDynamicSize` to `getStaticSize`??? That's crazy! I've been looking around 
in the code for how does your patch interact with our current dynamic size 
modeling, but I guess I never really realized that there is no such a thing. 
Odd to not even see a FIXME around.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:80-82
+  /* FIXME: Make this work.
+  if (const auto CI = Size.getAs())
+assert(CI->getValue().isUnsigned());*/

Make this work as is "make sure that checkers actually obey this precondition" 
or does the assert just not work as-is? Could you specify this comment?



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:779-780
 
   bool IsStandardGlobalOpNewFunction =
   FD->isReplaceableGlobalAllocationFunction();
 

I wonder what the rationale was here. Why do we explicitly avoid alignment new?



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:799-800
+} else {
   symVal = svalBuilder.conjureSymbolVal(nullptr, CNE, LCtx, CNE->getType(),
 blockCount);
+}

Regardless of whether this is heap allocated or not, we can set a dynamic size 
here, correct? Like, all the align new expressions are standard, but they 
aren't replaceable (as documented in `isReplaceableGlobalAllocationFunction()`).


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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 254051.
Charusso added a comment.

- Remove the last gymnastic.
- Rebase.


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

https://reviews.llvm.org/D69726

Files:
  clang/docs/analyzer/developer-docs/DebugChecks.rst
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/memory-model.cpp

Index: clang/test/Analysis/memory-model.cpp
===
--- /dev/null
+++ clang/test/Analysis/memory-model.cpp
@@ -0,0 +1,114 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,cplusplus,debug.ExprInspection \
+// RUN:  -triple x86_64-unknown-linux-gnu \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *realloc(void *ptr, size_t size);
+void *calloc(size_t number, size_t size);
+void free(void *);
+
+struct S { int f; };
+
+void clang_analyzer_dump(int);
+void clang_analyzer_dump(const void *);
+void clang_analyzer_dumpExtent(int);
+void clang_analyzer_dumpExtent(const void *);
+void clang_analyzer_dumpElementCount(int);
+void clang_analyzer_dumpElementCount(const void *);
+
+void var_simple_ref() {
+  int a = 13;
+  clang_analyzer_dump(); // expected-warning {{a}}
+  clang_analyzer_dumpExtent(); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void var_simple_ptr(int *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning-re ^\(extent_\$1\{SymRegion\{reg_\$0\}\}\) / 4$
+}
+
+void var_array() {
+  int a[] = {1, 2, 3};
+  clang_analyzer_dump(a); // expected-warning {{Element{a,0 S64b,int}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void string() {
+  clang_analyzer_dump("foo"); // expected-warning {{Element{"foo",0 S64b,char}}}
+  clang_analyzer_dumpExtent("foo"); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount("foo"); // expected-warning {{4 S64b}}
+}
+
+void struct_simple_ptr(S *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning-re ^\(extent_\$1\{SymRegion\{reg_\$0\}\}\) / 4$
+}
+
+void field_ref(S a) {
+  clang_analyzer_dump(); // expected-warning {{a.f}}
+  clang_analyzer_dumpExtent(); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void field_ptr(S *a) {
+  clang_analyzer_dump(>f); // expected-warning {{SymRegion{reg_$0}.f}}
+  clang_analyzer_dumpExtent(>f); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(>f); // expected-warning {{1 S64b}}
+}
+
+void symbolic_array() {
+  int *a = new int[3];
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] a;
+}
+
+void symbolic_placement_new() {
+  char *buf = new char[sizeof(int) * 3];
+  int *a = new (buf) int(13);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] buf;
+}
+
+void symbolic_malloc() {
+  int *a = (int *)malloc(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  free(a);
+}
+
+void symbolic_alloca() {
+  int *a = (int *)alloca(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void symbolic_complex() {
+  int *a = (int *)malloc(4);
+  clang_analyzer_dumpExtent(a); // expected-warning {{4 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{1 S64b}}
+
+  int *b = (int *)realloc(a, sizeof(int) * 2);
+  

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 253394.
Charusso marked 19 inline comments as done.
Charusso edited the summary of this revision.
Charusso added a comment.
Herald added a subscriber: ASDenysPetrov.

- Fix `VLASizeChecker`'s multi-dimensional array early return.
- So that fix the regression in test `misc-ps-region-store.m`.
- Fix tests that need regex.
- Add documentation about `dumpExtent`, `dumpElementCount`.


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

https://reviews.llvm.org/D69726

Files:
  clang/docs/analyzer/developer-docs/DebugChecks.rst
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/memory-model.cpp

Index: clang/test/Analysis/memory-model.cpp
===
--- /dev/null
+++ clang/test/Analysis/memory-model.cpp
@@ -0,0 +1,114 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,cplusplus,debug.ExprInspection \
+// RUN:  -triple x86_64-unknown-linux-gnu \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *realloc(void *ptr, size_t size);
+void *calloc(size_t number, size_t size);
+void free(void *);
+
+struct S { int f; };
+
+void clang_analyzer_dump(int);
+void clang_analyzer_dump(const void *);
+void clang_analyzer_dumpExtent(int);
+void clang_analyzer_dumpExtent(const void *);
+void clang_analyzer_dumpElementCount(int);
+void clang_analyzer_dumpElementCount(const void *);
+
+void var_simple_ref() {
+  int a = 13;
+  clang_analyzer_dump(); // expected-warning {{a}}
+  clang_analyzer_dumpExtent(); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void var_simple_ptr(int *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning-re ^\(extent_\$1\{SymRegion\{reg_\$0\}\}\) / 4$
+}
+
+void var_array() {
+  int a[] = {1, 2, 3};
+  clang_analyzer_dump(a); // expected-warning {{Element{a,0 S64b,int}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void string() {
+  clang_analyzer_dump("foo"); // expected-warning {{Element{"foo",0 S64b,char}}}
+  clang_analyzer_dumpExtent("foo"); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount("foo"); // expected-warning {{4 S64b}}
+}
+
+void struct_simple_ptr(S *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning-re ^\(extent_\$1\{SymRegion\{reg_\$0\}\}\) / 4$
+}
+
+void field_ref(S a) {
+  clang_analyzer_dump(); // expected-warning {{a.f}}
+  clang_analyzer_dumpExtent(); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void field_ptr(S *a) {
+  clang_analyzer_dump(>f); // expected-warning {{SymRegion{reg_$0}.f}}
+  clang_analyzer_dumpExtent(>f); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(>f); // expected-warning {{1 S64b}}
+}
+
+void symbolic_array() {
+  int *a = new int[3];
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] a;
+}
+
+void symbolic_placement_new() {
+  char *buf = new char[sizeof(int) * 3];
+  int *a = new (buf) int(13);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] buf;
+}
+
+void symbolic_malloc() {
+  int *a = (int *)malloc(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  free(a);
+}
+
+void symbolic_alloca() {
+  int *a = (int *)alloca(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 U64b}}
+  

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:25
 
-/// Get the stored dynamic size for the region \p MR.
+/// \returns The stored dynamic size for the region \p MR.
 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,

NoQ wrote:
> Naming: I believe we should keep using the word "Extent". There's no need to 
> introduce a new term for the existing concept; it makes it harder to explain 
> on the mailing list :) Let's make a follow-up patch to change the naming back 
> (so that not to screw the review).
Since then I have changed my mind when I have read about Environment and Store 
in a book. Sure, next up.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44
+/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR.
+ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR,
+   const CXXNewExpr *NE,
+   const LocationContext *LCtx, SValBuilder );

NoQ wrote:
> This function is probably going to be used exactly once in the whole code. 
> There's no need to turn it into a public API.
It is being used 3 times already, so I believe it is a cool API.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:99
+  .Case("clang_analyzer_region", 
::analyzerRegion)
+  .Case("clang_analyzer_size", 
::analyzerDumpSize)
+  .Case("clang_analyzer_elementCount",

NoQ wrote:
> `clang_analyzer_dump_extent()`? Or just 
> `clang_analyzer_dump(clang_analyzer_getExtent())`.
I like the shorter version, but of course I have seen the longer version.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:283
+  llvm::raw_svector_ostream Out(Msg);
+  Out << MR;
+  reportBug(Out.str(), C);

NoQ wrote:
> So, how is it different from the existing `clang_analyzer_dump()`?
I wanted to make it for the derived regions, but then I have realized I am lazy 
to dig into the buggier parts of the Analyzer. Removed.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311
+
+  QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext())
+: CE->getArg(0)->IgnoreParenImpCasts()->getType();
+

NoQ wrote:
> How is this better than `getValueType()`? Are you sure you're not getting a 
> pointer type instead in the `!TVR` case? I.e., can you test this on a 
> non-heap `SymRegion`?
> How is this better than getValueType()?
Consistency. We get the static ~~size~~ extent by getting the desugared type 
which most likely just an extra overhead.

> Are you sure you're not getting a pointer type instead in the !TVR case? 
> I.e., can you test this on a non-heap SymRegion?
The issue was the `var_region_simple_ptr` test: we need to use regex for 
checking after the `}}` token where `/ 4` is the correct result. I did not 
really get why the test pass.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+if (CI->getValue().isUnsigned())
+  Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+

NoQ wrote:
> Charusso wrote:
> > That one is interesting. Some of the checkers / SValBuilder(?) generate 
> > unsigned integers which should not happen, I believe. May we need a FIXME 
> > and an assertion about signedness. What do you think?
> `SymbolExtent` has type `size_t` and both `malloc` and `operator new` accept 
> `size_t` as a parameter. Therefore everything needs to be //unsigned// and we 
> need to assert this.
> 
> That said, array indexes are //signed// (as per implementation of 
> `ElementRegion`).
It was a premature optimization for consistency. I will leave the signedness as 
is, FIXME added.



Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+tmp2[x] = am; // expected-warning \
+{{Access out-of-bound array element (buffer overflow)}}
   }

NoQ wrote:
> Charusso wrote:
> > That is the single regression which I do not get.
> Well, please debug. Like, look at the full report, dump egraph, see what 
> changed. Try to `creduce` the example further under the condition "behavior 
> changed with the patch", maybe that'll clear something up (though if it's a 
> true positive after creduce it doesn't guarantee that it's a true positive 
> before creduce).
Given that I have no access for `rdar` I did my hand-reduce and the solution is 
the following:
```lang=c
// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 \
// RUN:  -analyzer-checker=core,alpha.security.ArrayBound \
// RUN:  -verify %s

static void RDar8424269_B(int unknown) {
  unsigned char tmp2t[1][unknown];
  tmp2t[1][13] = 0; // expected-warning \
{{Access out-of-bound array element (buffer 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:25
 
-/// Get the stored dynamic size for the region \p MR.
+/// \returns The stored dynamic size for the region \p MR.
 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,

Naming: I believe we should keep using the word "Extent". There's no need to 
introduce a new term for the existing concept; it makes it harder to explain on 
the mailing list :) Let's make a follow-up patch to change the naming back (so 
that not to screw the review).



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44
+/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR.
+ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR,
+   const CXXNewExpr *NE,
+   const LocationContext *LCtx, SValBuilder );

This function is probably going to be used exactly once in the whole code. 
There's no need to turn it into a public API.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:99
+  .Case("clang_analyzer_region", 
::analyzerRegion)
+  .Case("clang_analyzer_size", 
::analyzerDumpSize)
+  .Case("clang_analyzer_elementCount",

`clang_analyzer_dump_extent()`? Or just 
`clang_analyzer_dump(clang_analyzer_getExtent())`.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:100
+  .Case("clang_analyzer_size", 
::analyzerDumpSize)
+  .Case("clang_analyzer_elementCount",
+::analyzerDumpElementCount)

`clang_analyzer_dumpElementCount()`. We need subjects in our sentences because 
they tend to vary quite a bit here and therefore cannot be implied (dump, 
return, express, ...).



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:183-185
+  const Expr *Arg = nullptr;
+  if (!(Arg = getArgExpr(CE, C)))
+return nullptr;

```lang=c++
  const Expr *Arg = getArgExpr(CE, C);
  if (!Arg)
return nullptr;
```

No need for gymnastics >.>



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:283
+  llvm::raw_svector_ostream Out(Msg);
+  Out << MR;
+  reportBug(Out.str(), C);

So, how is it different from the existing `clang_analyzer_dump()`?



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:289-291
+  const MemRegion *MR = nullptr;
+  if (!(MR = getArgRegion(CE, C)))
+return;

```lang=c++
  const MemRegion *MR = getArgRegion(CE, C);
  if (!MR)
return;
```
... and so on.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311
+
+  QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext())
+: CE->getArg(0)->IgnoreParenImpCasts()->getType();
+

How is this better than `getValueType()`? Are you sure you're not getting a 
pointer type instead in the `!TVR` case? I.e., can you test this on a non-heap 
`SymRegion`?



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+if (CI->getValue().isUnsigned())
+  Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+

Charusso wrote:
> That one is interesting. Some of the checkers / SValBuilder(?) generate 
> unsigned integers which should not happen, I believe. May we need a FIXME and 
> an assertion about signedness. What do you think?
`SymbolExtent` has type `size_t` and both `malloc` and `operator new` accept 
`size_t` as a parameter. Therefore everything needs to be //unsigned// and we 
need to assert this.

That said, array indexes are //signed// (as per implementation of 
`ElementRegion`).



Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+tmp2[x] = am; // expected-warning \
+{{Access out-of-bound array element (buffer overflow)}}
   }

Charusso wrote:
> That is the single regression which I do not get.
Well, please debug. Like, look at the full report, dump egraph, see what 
changed. Try to `creduce` the example further under the condition "behavior 
changed with the patch", maybe that'll clear something up (though if it's a 
true positive after creduce it doesn't guarantee that it's a true positive 
before creduce).


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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 248097.
Charusso marked 4 inline comments as done.
Charusso edited the summary of this revision.
Charusso added a comment.

- Set the size properly.
- Add new debug.ExprInspection patterns: region, size, element count.
- `clang-format -i ExprInspectionChecker.cpp`.
- Having no idea what is the single regression in tests.


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

https://reviews.llvm.org/D69726

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/memory-model.cpp
  clang/test/Analysis/misc-ps-region-store.m

Index: clang/test/Analysis/misc-ps-region-store.m
===
--- clang/test/Analysis/misc-ps-region-store.m
+++ clang/test/Analysis/misc-ps-region-store.m
@@ -1186,7 +1186,8 @@
 for (y = 0; y < b_h; y++) {
   for (x = 0; x < b_w + 1; x++) {
 int am = 0;
-tmp2[x] = am;
+tmp2[x] = am; // expected-warning \
+{{Access out-of-bound array element (buffer overflow)}}
   }
 }
   }
Index: clang/test/Analysis/memory-model.cpp
===
--- /dev/null
+++ clang/test/Analysis/memory-model.cpp
@@ -0,0 +1,108 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,cplusplus,debug.ExprInspection \
+// RUN:  -triple x86_64-unknown-linux-gnu \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *realloc(void *ptr, size_t size);
+void *calloc(size_t number, size_t size);
+void free(void *);
+
+struct S { int f; };
+
+void clang_analyzer_region(int);
+void clang_analyzer_region(const void *);
+void clang_analyzer_size(int);
+void clang_analyzer_size(const void *);
+void clang_analyzer_elementCount(int);
+void clang_analyzer_elementCount(const void *);
+
+void var_region_simple_ref() {
+  int a = 13;
+  clang_analyzer_region(); // expected-warning {{a}}
+  clang_analyzer_size(); // expected-warning {{4 S64b}}
+  clang_analyzer_elementCount(); // expected-warning {{1 S64b}}
+}
+
+void var_region_simple_ptr(int *a) {
+  clang_analyzer_region(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_size(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_elementCount(a); // expected-warning {{(extent_$1{SymRegion{reg_$0}}) / 8}}
+}
+
+void var_region_array() {
+  int a[] = {1, 2, 3};
+  clang_analyzer_region(a); // expected-warning {{Element{a,0 S64b,int}}}
+  clang_analyzer_size(a); // expected-warning {{12 S64b}}
+  clang_analyzer_elementCount(a); // expected-warning {{3 S64b}}
+}
+
+void string_region() {
+  clang_analyzer_region("foo"); // expected-warning {{Element{"foo",0 S64b,char}}}
+  clang_analyzer_size("foo"); // expected-warning {{4 S64b}}
+  clang_analyzer_elementCount("foo"); // expected-warning {{4 S64b}}
+}
+
+void field_region_ref(S a) {
+  clang_analyzer_region(); // expected-warning {{a.f}}
+  clang_analyzer_size(); // expected-warning {{4 S64b}}
+  clang_analyzer_elementCount(); // expected-warning {{1 S64b}}
+}
+
+void field_region_ptr(S *a) {
+  clang_analyzer_region(>f); // expected-warning {{SymRegion{reg_$0}.f}}
+  clang_analyzer_size(>f); // expected-warning {{4 S64b}}
+  clang_analyzer_elementCount(>f); // expected-warning {{1 S64b}}
+}
+
+void symbolic_array() {
+  int *a = new int[3];
+  clang_analyzer_region(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_size(a); // expected-warning {{12 S64b}}
+  clang_analyzer_elementCount(a); // expected-warning {{3 S64b}}
+  delete[] a;
+}
+
+void symbolic_placement_new() {
+  char *buf = new char[sizeof(int) * 3];
+  int *a = new (buf) int(13);
+  clang_analyzer_region(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_size(a); // expected-warning {{12 S64b}}
+  clang_analyzer_elementCount(a); // expected-warning {{3 S64b}}
+  delete[] buf;
+}
+
+void symbolic_malloc() {
+  int *a = (int *)malloc(12);
+  clang_analyzer_region(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_size(a); // expected-warning {{12 S64b}}
+  clang_analyzer_elementCount(a); // expected-warning {{3 S64b}}
+  free(a);
+}
+
+void symbolic_alloca() {
+  int *a = (int *)alloca(12);
+  clang_analyzer_region(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_size(a); // 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+if (CI->getValue().isUnsigned())
+  Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+

That one is interesting. Some of the checkers / SValBuilder(?) generate 
unsigned integers which should not happen, I believe. May we need a FIXME and 
an assertion about signedness. What do you think?



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:698
+  DefinedOrUnknownSVal Size = UnknownVal();
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr))
+Size = State->getSVal(SizeExpr, LCtx).castAs();

NoQ wrote:
> Same. I guess we should add a test for both cases, given that nothing failed 
> when we screwed up the extent.
Well, it was equally wrong everywhere, so that it works... I have noticed it 
like 5 months ago, but I was lazy to fix.



Comment at: clang/test/Analysis/explain-svals.cpp:52
   // Sic! What gets computed is the extent of the element-region.
-  clang_analyzer_explain(clang_analyzer_getExtent(x)); // 
expected-warning-re^signed 32-bit integer '4'$
+  clang_analyzer_explain(clang_analyzer_getExtent(x)); // 
expected-warning-re^\(argument 'ext'\) \* 4$
   delete[] x;

Yea, that is the fact: The size is the size of the parameter, which is unknown.



Comment at: clang/test/Analysis/memory-model.cpp:108
+  free(c);
+}

Here I wanted to put more, but I am not that cool with other MemRegions. Some 
wise words about the followups of this test file:
> Some day
- The 11 years old comment in ExprEngine.cpp: 
https://github.com/llvm/llvm-project/blame/master/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp#L237



Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+tmp2[x] = am; // expected-warning \
+{{Access out-of-bound array element (buffer overflow)}}
   }

That is the single regression which I do not get.


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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.
Herald added subscribers: martong, steakhal.

Hmm, has this patch not landed yet? I was so excited to finally have 
https://bugs.llvm.org/show_bug.cgi?id=28450 fixed :)




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:767
+  DefinedOrUnknownSVal Size = UnknownVal();
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr))
+Size = State->getSVal(SizeExpr, LCtx).castAs();

This looks like an object count, we'll need to convert it to size in bytes 
because that's what extent is.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:698
+  DefinedOrUnknownSVal Size = UnknownVal();
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr))
+Size = State->getSVal(SizeExpr, LCtx).castAs();

Same. I guess we should add a test for both cases, given that nothing failed 
when we screwed up the extent.


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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 241749.
Charusso edited the summary of this revision.
Charusso added a comment.

- Let us reuse this patch.
- Remove the expression storing feature.
- Only store known sizes.


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

https://reviews.llvm.org/D69726

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -10,15 +10,16 @@
 //
 //===--===//
 
-#include "clang/AST/Decl.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "PrettyStackTraceLocationContext.h"
 #include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/Analysis/Analyses/LiveVariables.h"
 #include "clang/Analysis/ConstructionContext.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -690,16 +691,18 @@
 
 // See if we need to conjure a heap pointer instead of
 // a regular unknown pointer.
-bool IsHeapPointer = false;
-if (const auto *CNE = dyn_cast(E))
-  if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
-// FIXME: Delegate this to evalCall in MallocChecker?
-IsHeapPointer = true;
-  }
-
-R = IsHeapPointer ? svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count)
-  : svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy,
- Count);
+const auto *CNE = dyn_cast(E);
+if (CNE && CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+  // FIXME: Delegate this to evalCall in MallocChecker?
+  DefinedOrUnknownSVal Size = UnknownVal();
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr))
+Size = State->getSVal(SizeExpr, LCtx).castAs();
+
+  R = svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count);
+  State = setDynamicSize(State, R.getAsRegion(), Size);
+} else {
+  R = svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+}
   }
   return State->BindExpr(E, LCtx, R);
 }
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -10,15 +10,16 @@
 //
 //===--===//
 
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
-#include "clang/Analysis/ConstructionContext.h"
 #include "clang/AST/DeclCXX.h"
-#include "clang/AST/StmtCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/AST/StmtCXX.h"
+#include "clang/Analysis/ConstructionContext.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 
 using namespace clang;
 using namespace ento;
@@ -761,11 +762,17 @@
   // heap. We realize this is an approximation that might not correctly model
   // a custom global allocator.
   if (symVal.isUnknown()) {
-if (IsStandardGlobalOpNewFunction)
+if (IsStandardGlobalOpNewFunction) {
+  DefinedOrUnknownSVal Size = UnknownVal();
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr))
+Size = State->getSVal(SizeExpr, LCtx).castAs();
+
   symVal = svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount);
-else
+  State = setDynamicSize(State, symVal.getAsRegion(), Size);
+} else {
   symVal = svalBuilder.conjureSymbolVal(nullptr, CNE, LCtx, CNE->getType(),
 blockCount);
+}
   }
 
   CallEventManager  = getStateManager().getCallEventManager();
Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
===
--- 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso abandoned this revision.
Charusso marked an inline comment as done.
Charusso added a comment.

Let us say, we do not need this patch. In case we would need it, I will reopen.


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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:29-31
+/// \returns The stored dynamic size expression for the region \p MR.
+const Expr *getDynamicSizeExpr(ProgramStateRef State, const MemRegion *MR);
+

Charusso wrote:
> NoQ wrote:
> > Why do we need this?
> I think as the checkers are growing and we push more-and-more allocation 
> modeling so that at some point the Git's 8-parameter allocator's size 
> expression could be retrieved so easily. This is the full arsenal of my 
> buffer-overflow checker's needs, so I have pushed it here. Also it made a 
> meaning to have a helper-class with two fields (one would be lame).
>>! In D68725#1722136, @NoQ wrote:
> any path-sensitive checker for which such region is "interesting" would have 
> to implement a bug visitor to display the allocation site. Such visitor 
> automatically knows the location of the `alloca()` expression and can emit 
> the necessary fixits.


>>! In D69813#inline-628182, @NoQ wrote:
> Again, you will have to highlight the allocation site with a note. Therefore 
> you will have to write a bug visitor that traverses the size expression at 
> some point (or, equivalently, a note tag when the size expression is 
> evaluated). Therefore you don't need to store the expression in the program 
> state.


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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 3 inline comments as done.
Charusso added a comment.

In D69726#1732334 , @Szelethus wrote:

> Changes to `MallocChecker` really highlight the positive effects of this 
> patch. Nice!


Thanks!




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:451
   static ProgramStateRef MallocMemAux(CheckerContext , const CallExpr *CE,
-  const Expr *SizeEx, SVal Init,
+  const Expr *SizeExpr, SVal Init,
   ProgramStateRef State,

Szelethus wrote:
> Is it possible to merge these parameters into a single `DynamicSizeInfo` 
> object?
I want to do the opposite so I want to hide such constructors with the global 
getters and setters to make it easier to use. It turns out you only want to 
obtain one of its fields at a time, so that it is a bad idea to give you the 
entire object with multiple metadata. In my mind they will be in a `dynamic` 
namespace, and we could write that: `dynamic::getSize(State, MR)` or 
`dynamic::getType(State, MR)`.

I like the idea of creating `Contexts` to store metadata, but this is not the 
use case of the dynamic information, I think. The `Dynamic*Info` is good for 
being stored in a map, and other than that it is just boilerplate to obtain the 
object and call one of its fields which I do not like anymore.


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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Changes to `MallocChecker` really highlight the positive effects of this patch. 
Nice!




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:451
   static ProgramStateRef MallocMemAux(CheckerContext , const CallExpr *CE,
-  const Expr *SizeEx, SVal Init,
+  const Expr *SizeExpr, SVal Init,
   ProgramStateRef State,

Is it possible to merge these parameters into a single `DynamicSizeInfo` object?



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1167
   // elements with zeros in the last one.
-  State = MallocMemAux(C, CE, UnknownVal(), UnknownVal(), State,
-   AF_IfNameIndex);
+  State = MallocMemAux(C, CE, /*SizeExpr=*/nullptr, UnknownVal(),
+   UnknownVal(), State, AF_IfNameIndex);

and here?


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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 227534.
Charusso marked 6 inline comments as done.
Charusso added a comment.

- Fix.


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

https://reviews.llvm.org/D69726

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -10,7 +10,6 @@
 //
 //===--===//
 
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "PrettyStackTraceLocationContext.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclCXX.h"
@@ -18,6 +17,8 @@
 #include "clang/Analysis/ConstructionContext.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -653,16 +654,21 @@
 
 // See if we need to conjure a heap pointer instead of
 // a regular unknown pointer.
-bool IsHeapPointer = false;
-if (const auto *CNE = dyn_cast(E))
-  if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
-// FIXME: Delegate this to evalCall in MallocChecker?
-IsHeapPointer = true;
+const auto *CNE = dyn_cast(E);
+if (CNE && CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+  // FIXME: Delegate this to evalCall in MallocChecker?
+  DefinedOrUnknownSVal Size = UnknownVal();
+  const Expr *SizeExpr = nullptr;
+
+  if ((SizeExpr = CNE->getArraySize().getValueOr(nullptr))) {
+Size = State->getSVal(SizeExpr, LCtx).castAs();
   }
 
-R = IsHeapPointer ? svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count)
-  : svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy,
- Count);
+  R = svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count);
+  State = setDynamicSize(State, R.getAsRegion(), Size, SizeExpr);
+} else {
+  R = svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+}
   }
   return State->BindExpr(E, LCtx, R);
 }
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -10,15 +10,16 @@
 //
 //===--===//
 
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
-#include "clang/Analysis/ConstructionContext.h"
 #include "clang/AST/DeclCXX.h"
-#include "clang/AST/StmtCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/AST/StmtCXX.h"
+#include "clang/Analysis/ConstructionContext.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 
 using namespace clang;
 using namespace ento;
@@ -761,11 +762,20 @@
   // heap. We realize this is an approximation that might not correctly model
   // a custom global allocator.
   if (symVal.isUnknown()) {
-if (IsStandardGlobalOpNewFunction)
+if (IsStandardGlobalOpNewFunction) {
+  DefinedOrUnknownSVal Size = UnknownVal();
+  const Expr *SizeExpr = nullptr;
+
+  if ((SizeExpr = CNE->getArraySize().getValueOr(nullptr))) {
+Size = State->getSVal(SizeExpr, LCtx).castAs();
+  }
+
   symVal = svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount);
-else
+  State = setDynamicSize(State, symVal.getAsRegion(), Size, SizeExpr);
+} else {
   symVal = svalBuilder.conjureSymbolVal(nullptr, CNE, LCtx, CNE->getType(),
 blockCount);
+}
   }
 
   CallEventManager  = getStateManager().getCallEventManager();
Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
===
--- 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:29-31
+/// \returns The stored dynamic size expression for the region \p MR.
+const Expr *getDynamicSizeExpr(ProgramStateRef State, const MemRegion *MR);
+

NoQ wrote:
> Why do we need this?
I think as the checkers are growing and we push more-and-more allocation 
modeling so that at some point the Git's 8-parameter allocator's size 
expression could be retrieved so easily. This is the full arsenal of my 
buffer-overflow checker's needs, so I have pushed it here. Also it made a 
meaning to have a helper-class with two fields (one would be lame).



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1416
+   SizeInBytes.castAs(),
+   NE->getArraySize().getValueOr(nullptr));
   }

My problem was only that. It partially repeats the 
`ExprEngine::bindReturnValue()`, which is a wonky design. I will look into that 
later.



Comment at: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:180
-  // Assume should not fail at this point.
-  assert(state);
 

NoQ wrote:
> This gets rid of the assertion failure in 
> https://bugs.llvm.org/show_bug.cgi?id=28450 by implementing my suggestion 
> (2). Yay.
Cool!


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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:29-31
+/// \returns The stored dynamic size expression for the region \p MR.
+const Expr *getDynamicSizeExpr(ProgramStateRef State, const MemRegion *MR);
+

Why do we need this?



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1073
+
+setDynamicSize(State, BR, *SizeNL, Size);
 

Charusso wrote:
> That dual assumption made changes in the test files, and there is no other 
> dual assumption.
Wait, this code should not have been changed. It's not an allocation site, we 
don't receive any new information about the size of the region here.



Comment at: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:180
-  // Assume should not fail at this point.
-  assert(state);
 

This gets rid of the assertion failure in 
https://bugs.llvm.org/show_bug.cgi?id=28450 by implementing my suggestion (2). 
Yay.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked an inline comment as done.
Charusso added a comment.

I do not want to reverse engineer the `MallocChecker` to obtain the size on 
call basis. The current model without D68725  
makes it enough difficult to obtain the size even with this generic map, but it 
is working fine.




Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1073
+
+setDynamicSize(State, BR, *SizeNL, Size);
 

That dual assumption made changes in the test files, and there is no other dual 
assumption.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso created this revision.
Charusso added a reviewer: NoQ.
Charusso added a project: clang.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

This patch introduces a way to store the symbolic size and its expression.


Repository:
  rC Clang

https://reviews.llvm.org/D69726

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/null-deref-ps-region.c
  clang/test/Analysis/string.c

Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -1274,7 +1274,7 @@
 void memset5_char_malloc_overflow_null() {
   char *str = (char *)malloc(10 * sizeof(char));
   memset(str, '\0', 12);
-  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{TRUE}}
   free(str);
 }
 #endif
@@ -1362,7 +1362,7 @@
   char *str = (char *)malloc(10 * sizeof(int));
   int *array = (int *)str;
   memset(array, 0, 5 * sizeof(int));
-  clang_analyzer_eval(str[10] == '\0');// expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(str[10] == '\0');// expected-warning{{TRUE}}
   clang_analyzer_eval(strlen((char *)array) == 0); // expected-warning{{TRUE}}
   clang_analyzer_eval(strlen(str) == 0);   // expected-warning{{TRUE}}
   free(str);
@@ -1380,7 +1380,7 @@
 int memset21_scalar() {
   int *x = malloc(sizeof(int));
   memset(x, 0, 1);
-  int num = 1 / *x;
+  int num = 1 / *x; // expected-warning{{Division by zero}}
   free(x);
   return num;
 }
@@ -1476,7 +1476,7 @@
 #endif
   clang_analyzer_eval(privkey[0] == '\0');
 #ifdef SUPPRESS_OUT_OF_BOUND
-  // expected-warning@-2 {{UNKNOWN}}
+  // expected-warning@-2 {{TRUE}}
 #endif
   free(privkey);
 }
Index: clang/test/Analysis/null-deref-ps-region.c
===
--- clang/test/Analysis/null-deref-ps-region.c
+++ clang/test/Analysis/null-deref-ps-region.c
@@ -33,7 +33,7 @@
 void bar() {
   int *x = malloc(sizeof(int));
   memset(x, 0, 1);
-  int n = 1 / *x; // no-warning
+  int n = 1 / *x; // expected-warning {{Division by zero}}
   free(x);
 }
 
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -10,7 +10,6 @@
 //
 //===--===//
 
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "PrettyStackTraceLocationContext.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclCXX.h"
@@ -18,6 +17,8 @@
 #include "clang/Analysis/ConstructionContext.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -653,16 +654,21 @@
 
 // See if we need to conjure a heap pointer instead of
 // a regular unknown pointer.
-bool IsHeapPointer = false;
-if (const auto *CNE = dyn_cast(E))
-  if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
-// FIXME: Delegate this to evalCall in MallocChecker?
-IsHeapPointer = true;
+const auto *CNE = dyn_cast(E);
+if (CNE && CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+  // FIXME: Delegate this to evalCall in MallocChecker?
+  DefinedOrUnknownSVal Size = UnknownVal();
+  const Expr *SizeExpr = nullptr;
+
+  if ((SizeExpr = CNE->getArraySize().getValueOr(nullptr))) {
+Size = State->getSVal(SizeExpr, LCtx).castAs();
   }
 
-R = IsHeapPointer ? svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count)
-  : svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy,
- Count);
+  R = svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count);
+  State = setDynamicSize(State, R.getAsRegion(), Size, SizeExpr);
+} else {
+  R = svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+}
   }
   return State->BindExpr(E, LCtx, R);
 }
Index: