[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs();
+  if (!SizeOfTargetCI)

xazax.hun wrote:
> Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as `ConcreteInt`s, 
> I think you should rather use `evalBinOp` to compare them. That method is 
> more future proof as if we cannot constraint these values down to a single 
> integer but we still have some information about them a sufficiently smart 
> solver could prove the relationship between the symbolic values.
I am not sure if `evalBinOp` is that useful here, because we need the concrete 
values anyway when we issue the diagnostics. We'd like to present the concrete 
sizes in bytes.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:32
+  const MemRegion *BaseRegion = MRegion->getBaseRegion();
+  assert(BaseRegion == Offset.getRegion());
+

NoQ wrote:
> martong wrote:
> > This assertion fails on real code quite often (e.g. on LLVM/Clang code). I 
> > don't really understand why. @NoQ what is you understanding on this? 
> > Perhaps I could try to reduce a case from the real code to see an example.
> `RegionOffset` cannot really represent symbolic offsets :( You should be able 
> to re-add the assertion after checking for symbolic offsets.
> 
> And, yeah, you can always easily reduce any crash with `creduce`.
I think protecting with `if (Offset.hasSymbolicOffset())` is just fine, instead 
of the assertion.



Comment at: clang/test/Analysis/placement-new.cpp:6
+// RUN:   -analyzer-output=text -verify \
+// RUN:   -triple x86_64-unknown-linux-gnu
+

NoQ wrote:
> Wow, somebody actually remembers to add a target triple for tests that depend 
> on the target triple! My respect, sir.
Thanks! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 237059.
martong marked 11 inline comments as done.
martong added a comment.

- Add documentation to checkers.rst
- Pass CheckerContext as last param
- Use BugType and default member initializer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new-user-defined.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- /dev/null
+++ clang/test/Analysis/placement-new.cpp
@@ -0,0 +1,141 @@
+// RUN: %clang_analyze_cc1 -std=c++11 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus.NewDelete \
+// RUN:   -analyzer-checker=cplusplus.PlacementNew \
+// RUN:   -analyzer-output=text -verify \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  long *lp = ::new (&s) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)lp;
+}
+
+namespace testArrayNew {
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  char *buf = ::new (&s) char[8]; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)buf;
+}
+} // namespace testArrayNew
+
+namespace testBufferInOtherFun {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  short buf; // expected-note {{'buf' declared without an initial value}}
+  f(&buf);   // expected-note 2 {{}}
+}
+} // namespace testBufferInOtherFun
+
+namespace testArrayBuffer {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  char buf[2]; // expected-note {{'buf' initialized here}}
+  f(&buf); // expected-note 2 {{}}
+}
+} // namespace testArrayBuffer
+
+namespace testGlobalPtrAsPlace {
+void *gptr = nullptr;
+short gs;
+void f() {
+  gptr = &gs; // expected-note {{Value assigned to 'gptr'}}
+}
+void g() {
+  f();  // expected-note 2 {{}}
+  long *lp = ::new (gptr) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testGlobalPtrAsPlace
+
+namespace testRvalue {
+short gs;
+void *f() {
+  return &gs;
+}
+void g() {
+  long *lp = ::new (f()) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testRvalue
+
+namespace testNoWarning {
+void *f();
+void g() {
+  long *lp = ::new (f()) long;
+  (void)lp;
+}
+} // namespace testNoWarning
+
+namespace testPtrToArrayAsPlace {
+void f() {
+  //char *st = new char [8];
+  char buf[3];// expected-note {{'buf' initialized here}}
+  void *st = buf; // expected-note {{'st' initialized here}}
+  long *lp = ::new (st) long; // expected-warning{{Storage provided to placement new is only 3 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayAsPlace
+
+namespace testPtrToArrayWithOffsetAsPlace {
+void f() {
+  int buf[3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf + 2) long; // expected-warning{{Storage provided to placement new is only 4 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayWithOffsetAsPlace
+
+namespace testHeapAllocatedBuffer {
+void g2() {
+  char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testHeapAllocatedBuffer
+
+namespace testMultiDimensionalArray {
+void f() {
+  char buf[2][3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 6 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D71612#1790045 , @NoQ wrote:

> I wonder if this checker will find any misuses of placement into 
> `llvm::TrailingObjects`.


I've evaluated this checker on LLVM/Clang source and it did not find any 
results, though there are many placement new calls there. I think this is good, 
because there are no false positives.

  [INFO 2019-12-21 00:15] -  Summary 
  [INFO 2019-12-21 00:15] - Successfully analyzed
  [INFO 2019-12-21 00:15] -   clangsa: 2975
  [INFO 2019-12-21 00:15] - Failed to analyze
  [INFO 2019-12-21 00:15] -   clangsa: 2

The two failures are caused by non existing `.inc` files, e.g.:

  
/mnt/ssd/egbomrt/WORK/llvm1/git/llvm-project/llvm/unittests/Option/OptionParsingTest.cpp:23:10:
 fatal error: 'Opts.inc' file not found
  #include "Opts.inc"
   ^~
  1 error generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

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

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs();
+  if (!SizeOfTargetCI)

martong wrote:
> xazax.hun wrote:
> > Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as 
> > `ConcreteInt`s, I think you should rather use `evalBinOp` to compare them. 
> > That method is more future proof as if we cannot constraint these values 
> > down to a single integer but we still have some information about them a 
> > sufficiently smart solver could prove the relationship between the symbolic 
> > values.
> I am not sure if `evalBinOp` is that useful here, because we need the 
> concrete values anyway when we issue the diagnostics. We'd like to present 
> the concrete sizes in bytes.
The reason why evalbinop might be useful because we might have symbolic sizes:
```
void f(int a) {
 char *buffer = new char[a];
}
```

So in the code snippet above you cannot get a concrete integer for the size of 
the buffer. But in case we already have some constraints about the value of 
`a`, the constraint solver might be able to tell if we are sure that the type 
will not fit into the buffer. I can imagine that this scenario is relatively 
rare, but I think we need relatively little code to support this. 

So you could potentially warn when:
```
void f(int a) {
  char *buffer = new char[a];
  if (a > 3)
return;
  int *p = new (buffer) int;
}
```

I know, this is silly code, but we might not know if there are reasonable code 
that has similar patterns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs();
+  if (!SizeOfTargetCI)

xazax.hun wrote:
> martong wrote:
> > xazax.hun wrote:
> > > Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as 
> > > `ConcreteInt`s, I think you should rather use `evalBinOp` to compare 
> > > them. That method is more future proof as if we cannot constraint these 
> > > values down to a single integer but we still have some information about 
> > > them a sufficiently smart solver could prove the relationship between the 
> > > symbolic values.
> > I am not sure if `evalBinOp` is that useful here, because we need the 
> > concrete values anyway when we issue the diagnostics. We'd like to present 
> > the concrete sizes in bytes.
> The reason why evalbinop might be useful because we might have symbolic sizes:
> ```
> void f(int a) {
>  char *buffer = new char[a];
> }
> ```
> 
> So in the code snippet above you cannot get a concrete integer for the size 
> of the buffer. But in case we already have some constraints about the value 
> of `a`, the constraint solver might be able to tell if we are sure that the 
> type will not fit into the buffer. I can imagine that this scenario is 
> relatively rare, but I think we need relatively little code to support this. 
> 
> So you could potentially warn when:
> ```
> void f(int a) {
>   char *buffer = new char[a];
>   if (a > 3)
> return;
>   int *p = new (buffer) int;
> }
> ```
> 
> I know, this is silly code, but we might not know if there are reasonable 
> code that has similar patterns.
For this sort of stuff i'd strongly recommend
1. explaining the range constraints for the buffer size in the warning message 
and
2. making a bug visitor that'll explain how these constraints change across the 
path.

I.e., "Assuming that 'a' is less than or equal to 3" => "Buffer size is 
constrained to [0, 3]" => "Storage provided to placement new is //at most// 3 
bytes, whereas the allocated type requires 4 bytes".

The same applies to our alpha array bound checkers. We really need this stuff 
explained in them. Without such facilities i'd rather stick to concrete values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

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

xazax.hun accepted this revision.
xazax.hun added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs();
+  if (!SizeOfTargetCI)

NoQ wrote:
> xazax.hun wrote:
> > martong wrote:
> > > xazax.hun wrote:
> > > > Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as 
> > > > `ConcreteInt`s, I think you should rather use `evalBinOp` to compare 
> > > > them. That method is more future proof as if we cannot constraint these 
> > > > values down to a single integer but we still have some information 
> > > > about them a sufficiently smart solver could prove the relationship 
> > > > between the symbolic values.
> > > I am not sure if `evalBinOp` is that useful here, because we need the 
> > > concrete values anyway when we issue the diagnostics. We'd like to 
> > > present the concrete sizes in bytes.
> > The reason why evalbinop might be useful because we might have symbolic 
> > sizes:
> > ```
> > void f(int a) {
> >  char *buffer = new char[a];
> > }
> > ```
> > 
> > So in the code snippet above you cannot get a concrete integer for the size 
> > of the buffer. But in case we already have some constraints about the value 
> > of `a`, the constraint solver might be able to tell if we are sure that the 
> > type will not fit into the buffer. I can imagine that this scenario is 
> > relatively rare, but I think we need relatively little code to support 
> > this. 
> > 
> > So you could potentially warn when:
> > ```
> > void f(int a) {
> >   char *buffer = new char[a];
> >   if (a > 3)
> > return;
> >   int *p = new (buffer) int;
> > }
> > ```
> > 
> > I know, this is silly code, but we might not know if there are reasonable 
> > code that has similar patterns.
> For this sort of stuff i'd strongly recommend
> 1. explaining the range constraints for the buffer size in the warning 
> message and
> 2. making a bug visitor that'll explain how these constraints change across 
> the path.
> 
> I.e., "Assuming that 'a' is less than or equal to 3" => "Buffer size is 
> constrained to [0, 3]" => "Storage provided to placement new is //at most// 3 
> bytes, whereas the allocated type requires 4 bytes".
> 
> The same applies to our alpha array bound checkers. We really need this stuff 
> explained in them. Without such facilities i'd rather stick to concrete 
> values.
This makes sense. How about committing this only supporting concrete values and 
introduce the visitor/symbolic support in a follow-up? (If Gabor Marton is 
motivated to implement it :) I am also ok if this will not get implemented for 
now.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D71612#1812036 , @martong wrote:

> In D71612#1790045 , @NoQ wrote:
>
> > I wonder if this checker will find any misuses of placement into 
> > `llvm::TrailingObjects`.
>
>
> I've evaluated this checker on LLVM/Clang source and it did not find any 
> results, though there are many placement new calls there. I think this is 
> good, because there are no false positives.


In such cases it's usually a good idea to verify that the checker works 
correctly by artificially injecting a bug into the codebase. If the bug is not 
found, the checker is probably not working. If the bug is found, change it to 
be more and more realistic, so that to see what limitations does the checker 
have in terms of false negatives. Monitor analyzer stats closely (max-nodes 
limits, block count limits, inlining limits) in order to see what exactly goes 
wrong (or debug on the Exploded Graph as usual, depending on how it goes 
wrong). This exercise often uncovers interesting omissions :)

Can we enable the check by default then? What pieces are missing? Like, the 
symbolic extent/offset case is not a must. I think we have everything except 
maybe some deeper testing. I'd rather spend some time on the above exercise, 
but then enable by default if it goes well.

> The two failures are caused by non existing `.inc` files, e.g.:
> 
>   
> /mnt/ssd/egbomrt/WORK/llvm1/git/llvm-project/llvm/unittests/Option/OptionParsingTest.cpp:23:10:
>  fatal error: 'Opts.inc' file not found
>   #include "Opts.inc"
>^~
>   1 error generated.

I've seen these errors with scan-build-py as well. It looks like LLVM's build 
system cannot be correctly reduced to a compilation database (though it's just 
one place - maybe it can be easily fixed).




Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs();
+  if (!SizeOfTargetCI)

xazax.hun wrote:
> NoQ wrote:
> > xazax.hun wrote:
> > > martong wrote:
> > > > xazax.hun wrote:
> > > > > Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as 
> > > > > `ConcreteInt`s, I think you should rather use `evalBinOp` to compare 
> > > > > them. That method is more future proof as if we cannot constraint 
> > > > > these values down to a single integer but we still have some 
> > > > > information about them a sufficiently smart solver could prove the 
> > > > > relationship between the symbolic values.
> > > > I am not sure if `evalBinOp` is that useful here, because we need the 
> > > > concrete values anyway when we issue the diagnostics. We'd like to 
> > > > present the concrete sizes in bytes.
> > > The reason why evalbinop might be useful because we might have symbolic 
> > > sizes:
> > > ```
> > > void f(int a) {
> > >  char *buffer = new char[a];
> > > }
> > > ```
> > > 
> > > So in the code snippet above you cannot get a concrete integer for the 
> > > size of the buffer. But in case we already have some constraints about 
> > > the value of `a`, the constraint solver might be able to tell if we are 
> > > sure that the type will not fit into the buffer. I can imagine that this 
> > > scenario is relatively rare, but I think we need relatively little code 
> > > to support this. 
> > > 
> > > So you could potentially warn when:
> > > ```
> > > void f(int a) {
> > >   char *buffer = new char[a];
> > >   if (a > 3)
> > > return;
> > >   int *p = new (buffer) int;
> > > }
> > > ```
> > > 
> > > I know, this is silly code, but we might not know if there are reasonable 
> > > code that has similar patterns.
> > For this sort of stuff i'd strongly recommend
> > 1. explaining the range constraints for the buffer size in the warning 
> > message and
> > 2. making a bug visitor that'll explain how these constraints change across 
> > the path.
> > 
> > I.e., "Assuming that 'a' is less than or equal to 3" => "Buffer size is 
> > constrained to [0, 3]" => "Storage provided to placement new is //at most// 
> > 3 bytes, whereas the allocated type requires 4 bytes".
> > 
> > The same applies to our alpha array bound checkers. We really need this 
> > stuff explained in them. Without such facilities i'd rather stick to 
> > concrete values.
> This makes sense. How about committing this only supporting concrete values 
> and introduce the visitor/symbolic support in a follow-up? (If Gabor Marton 
> is motivated to implement it :) I am also ok if this will not get implemented 
> for now.)
Technically, there's also a chance of the object size being symbolic (i.e., 
"placement `new[]`"), however placement `new[]` is already very weird due to 
the requirement to store the "allocated" size within the storage together with 
the actual buffer (IIRC the size of such header is implementation-defined and 
the operator almost always returns an 

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 237290.
martong added a comment.

- Declare ElementCountNL in if stmt
- Use makeArrayIndex instead makeIntVal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new-user-defined.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- /dev/null
+++ clang/test/Analysis/placement-new.cpp
@@ -0,0 +1,141 @@
+// RUN: %clang_analyze_cc1 -std=c++11 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus.NewDelete \
+// RUN:   -analyzer-checker=cplusplus.PlacementNew \
+// RUN:   -analyzer-output=text -verify \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  long *lp = ::new (&s) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)lp;
+}
+
+namespace testArrayNew {
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  char *buf = ::new (&s) char[8]; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)buf;
+}
+} // namespace testArrayNew
+
+namespace testBufferInOtherFun {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  short buf; // expected-note {{'buf' declared without an initial value}}
+  f(&buf);   // expected-note 2 {{}}
+}
+} // namespace testBufferInOtherFun
+
+namespace testArrayBuffer {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  char buf[2]; // expected-note {{'buf' initialized here}}
+  f(&buf); // expected-note 2 {{}}
+}
+} // namespace testArrayBuffer
+
+namespace testGlobalPtrAsPlace {
+void *gptr = nullptr;
+short gs;
+void f() {
+  gptr = &gs; // expected-note {{Value assigned to 'gptr'}}
+}
+void g() {
+  f();  // expected-note 2 {{}}
+  long *lp = ::new (gptr) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testGlobalPtrAsPlace
+
+namespace testRvalue {
+short gs;
+void *f() {
+  return &gs;
+}
+void g() {
+  long *lp = ::new (f()) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testRvalue
+
+namespace testNoWarning {
+void *f();
+void g() {
+  long *lp = ::new (f()) long;
+  (void)lp;
+}
+} // namespace testNoWarning
+
+namespace testPtrToArrayAsPlace {
+void f() {
+  //char *st = new char [8];
+  char buf[3];// expected-note {{'buf' initialized here}}
+  void *st = buf; // expected-note {{'st' initialized here}}
+  long *lp = ::new (st) long; // expected-warning{{Storage provided to placement new is only 3 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayAsPlace
+
+namespace testPtrToArrayWithOffsetAsPlace {
+void f() {
+  int buf[3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf + 2) long; // expected-warning{{Storage provided to placement new is only 4 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayWithOffsetAsPlace
+
+namespace testHeapAllocatedBuffer {
+void g2() {
+  char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testHeapAllocatedBuffer
+
+namespace testMultiDimensionalArray {
+void f() {
+  char buf[2][3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 6 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testMultiDimensionalArray
+
+namespace testMultiDimensionalArray2 {
+

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong marked 2 inline comments as done.
martong added a comment.

> Can we enable the check by default then? What pieces are missing? Like, the 
> symbolic extent/offset case is not a must. I think we have everything except 
> maybe some deeper testing. I'd rather spend some time on the above exercise, 
> but then enable by default if it goes well.

I am going to execute the analysis on LLVM/Clang as you suggested, so then 
we'll see what the experiment will bring us. And then we could enable it by 
default I think. BTW how can we do that? I could not find any default 
enablement related config in Checkers.td ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D71612#1813937 , @martong wrote:

> I am going to execute the analysis on LLVM/Clang as you suggested, so then 
> we'll see what the experiment will bring us. And then we could enable it by 
> default I think. BTW how can we do that? I could not find any default 
> enablement related config in Checkers.td ...


Enabling checkers by default is controlled by the messy code in 
`RenderAnalyzerOptions()` in the Driver.

Wait, you already made it on-by-default. Checkers that are currently under 
development go into the `alpha` package.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5e7beb0a4146: [analyzer] Add PlacementNewChecker (authored 
by martong).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new-user-defined.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- /dev/null
+++ clang/test/Analysis/placement-new.cpp
@@ -0,0 +1,141 @@
+// RUN: %clang_analyze_cc1 -std=c++11 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus.NewDelete \
+// RUN:   -analyzer-checker=cplusplus.PlacementNew \
+// RUN:   -analyzer-output=text -verify \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  long *lp = ::new (&s) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)lp;
+}
+
+namespace testArrayNew {
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  char *buf = ::new (&s) char[8]; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)buf;
+}
+} // namespace testArrayNew
+
+namespace testBufferInOtherFun {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  short buf; // expected-note {{'buf' declared without an initial value}}
+  f(&buf);   // expected-note 2 {{}}
+}
+} // namespace testBufferInOtherFun
+
+namespace testArrayBuffer {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  char buf[2]; // expected-note {{'buf' initialized here}}
+  f(&buf); // expected-note 2 {{}}
+}
+} // namespace testArrayBuffer
+
+namespace testGlobalPtrAsPlace {
+void *gptr = nullptr;
+short gs;
+void f() {
+  gptr = &gs; // expected-note {{Value assigned to 'gptr'}}
+}
+void g() {
+  f();  // expected-note 2 {{}}
+  long *lp = ::new (gptr) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testGlobalPtrAsPlace
+
+namespace testRvalue {
+short gs;
+void *f() {
+  return &gs;
+}
+void g() {
+  long *lp = ::new (f()) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testRvalue
+
+namespace testNoWarning {
+void *f();
+void g() {
+  long *lp = ::new (f()) long;
+  (void)lp;
+}
+} // namespace testNoWarning
+
+namespace testPtrToArrayAsPlace {
+void f() {
+  //char *st = new char [8];
+  char buf[3];// expected-note {{'buf' initialized here}}
+  void *st = buf; // expected-note {{'st' initialized here}}
+  long *lp = ::new (st) long; // expected-warning{{Storage provided to placement new is only 3 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayAsPlace
+
+namespace testPtrToArrayWithOffsetAsPlace {
+void f() {
+  int buf[3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf + 2) long; // expected-warning{{Storage provided to placement new is only 4 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayWithOffsetAsPlace
+
+namespace testHeapAllocatedBuffer {
+void g2() {
+  char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testHeapAllocatedBuffer
+
+namespace testMultiDimensionalArray {
+void f() {
+  char buf[2][3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 6 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testMultiDimensionalArray
+
+namespace testMultiDime

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D71612#1814225 , @NoQ wrote:

> In D71612#1813937 , @martong wrote:
>
> > I am going to execute the analysis on LLVM/Clang as you suggested, so then 
> > we'll see what the experiment will bring us. And then we could enable it by 
> > default I think. BTW how can we do that? I could not find any default 
> > enablement related config in Checkers.td ...
>
>
> Enabling checkers by default is controlled by the messy code in 
> `RenderAnalyzerOptions()` in the Driver.
>
> Wait, you already made it on-by-default. Checkers that are currently under 
> development go into the `alpha` package.


Ups, I am sorry. Now I am creating another commit which moves it to alpha. (So 
then, If I understand it correctly then the `cplusplus` is enabled by default, 
but `alpha` is not.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D71612#1814410 , @martong wrote:

> Ups, I am sorry. Now I am creating another commit which moves it to alpha. 
> (So then, If I understand it correctly then the `cplusplus` is enabled by 
> default, but `alpha` is not.)


Aha, yup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Related commit that moves to alpha:
https://github.com/llvm/llvm-project/commit/13ec473b9d4bd4f7a558272932b7c0806171c666


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D71612#1812476 , @NoQ wrote:

> In D71612#1812036 , @martong wrote:
>
> > In D71612#1790045 , @NoQ wrote:
> >
> > > I wonder if this checker will find any misuses of placement into 
> > > `llvm::TrailingObjects`.
> >
> >
> > I've evaluated this checker on LLVM/Clang source and it did not find any 
> > results, though there are many placement new calls there. I think this is 
> > good, because there are no false positives.
>
>
> In such cases it's usually a good idea to verify that the checker works 
> correctly by artificially injecting a bug into the codebase. If the bug is 
> not found, the checker is probably not working. If the bug is found, change 
> it to be more and more realistic, so that to see what limitations does the 
> checker have in terms of false negatives. Monitor analyzer stats closely 
> (max-nodes limits, block count limits, inlining limits) in order to see what 
> exactly goes wrong (or debug on the Exploded Graph as usual, depending on how 
> it goes wrong). This exercise often uncovers interesting omissions :)
>
> Can we enable the check by default then? What pieces are missing? Like, the 
> symbolic extent/offset case is not a must. I think we have everything except 
> maybe some deeper testing. I'd rather spend some time on the above exercise, 
> but then enable by default if it goes well.


Artem, I've made some more experiments with this checker. I searched for 
default placement new call expressions in the LLVM codebase and slightly 
modified the checker's code to log if we have concrete values for both sizes 
(target and place).

  @@ -95,6 +96,15 @@ void PlacementNewChecker::checkPreStmt(const CXXNewExpr 
*NE,
 if (!SizeOfPlaceCI)
   return;
  
  +  //auto& SM = C.getASTContext().getSourceManager();
  +  //NE->dump();
  +  //NE->getBeginLoc().dump(SM);
  +  //NE->getOperatorNew()->dump();
  +  //SizeOfTarget.dump();
  +  //llvm::errs() << "\n";
  +  //SizeOfPlace.dump();
  +  //llvm::errs() << "\n";
  +

This way I could pick one specific file for analysis: DeclBase.cpp.
Based on the logs, I modified two LLVM headers, thus introducing bugs, which 
should be found by this checker:

  diff --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
  index ed25b2cd89f..3a2c2df383d 100644
  --- a/llvm/include/llvm/ADT/APFloat.h
  +++ b/llvm/include/llvm/ADT/APFloat.h
  @@ -744,11 +744,11 @@ class APFloat : public APFloatBase {
  
   Storage(Storage &&RHS) {
 if (usesLayout(*RHS.semantics)) {
  -new (this) IEEEFloat(std::move(RHS.IEEE));
  +new ((char*)this+9) IEEEFloat(std::move(RHS.IEEE));
   return;
 }
 if (usesLayout(*RHS.semantics)) {
  -new (this) DoubleAPFloat(std::move(RHS.Double));
  +new ((char*)this+9) DoubleAPFloat(std::move(RHS.Double));
   return;
 }
 llvm_unreachable("Unexpected semantics");
  diff --git a/llvm/include/llvm/ADT/DenseMap.h 
b/llvm/include/llvm/ADT/DenseMap.h
  index 148d319c860..d0b23ed531b 100644
  --- a/llvm/include/llvm/ADT/DenseMap.h
  +++ b/llvm/include/llvm/ADT/DenseMap.h
  @@ -1024,8 +1024,8 @@ public:
   !KeyInfoT::isEqual(P->getFirst(), TombstoneKey)) {
 assert(size_t(TmpEnd - TmpBegin) < InlineBuckets &&
"Too many inline buckets!");
  -  ::new (&TmpEnd->getFirst()) KeyT(std::move(P->getFirst()));
  -  ::new (&TmpEnd->getSecond()) ValueT(std::move(P->getSecond()));
  +  ::new ((char*)(&TmpEnd->getFirst())+1) 
KeyT(std::move(P->getFirst()));
  +  ::new ((char*)(&TmpEnd->getSecond())+1) 
ValueT(std::move(P->getSecond()));
 ++TmpEnd;
 P->getSecond().~ValueT();
   }

And the results were what we expect, the checker did find the bugs:

  ) CodeChecker parse --print-steps reports
  Found no defects in DeclBase.cpp
  [LOW] /usr/include/c++/7/bits/atomic_base.h:188:20: Value stored to '__b' 
during its initialization is never read [deadcode.DeadStores]
memory_order __b = __m & __memory_order_mask;
 ^
Report hash: 311a73855b3f4477ee6a4d02482e7c09
Steps:
  1, atomic_base.h:188:20: Value stored to '__b' during its initialization 
is never read
  
  [LOW] /usr/include/c++/7/bits/atomic_base.h:199:20: Value stored to '__b' 
during its initialization is never read [deadcode.DeadStores]
memory_order __b = __m & __memory_order_mask;
 ^
Report hash: 890f61293b984671c6bf407dbbf4352a
Steps:
  1, atomic_base.h:199:20: Value stored to '__b' during its initialization 
is never read
  
  Found 2 defect(s) in atomic_base.h
  
  [UNSPECIFIED] 
/home/egbomrt/WORK/llvm3/git/llvm-project/llvm/include/llvm/ADT/DenseMap.h:1027:11:
 Storage provided to placement new is only 7 bytes, whereas the allocated type 
requires 8 bytes

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This looks fantastic. Let's enable by it default!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

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

This checker verifies if default placement new is provided with pointers
to sufficient storage capacity.

Noncompliant Code Example:

  #include 
  void f() {
short s;
long *lp = ::new (&s) long;
  }

Based on SEI CERT rule MEM54-CPP
https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM54-CPP.+Provide+placement+new+with+properly+aligned+pointers+to+sufficient+storage+capacity
This patch does not implement checking of the alignment.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71612

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new-user-defined.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- /dev/null
+++ clang/test/Analysis/placement-new.cpp
@@ -0,0 +1,94 @@
+// RUN: %clang_analyze_cc1 -std=c++11 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus.NewDelete \
+// RUN:   -analyzer-checker=cplusplus.PlacementNew \
+// RUN:   -analyzer-output=text -verify \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void f() {
+  short s; // expected-note-re {{'s' declared{{.*
+  long *lp = ::new (&s) long; // expected-warning{{Argument of default placement new provides storage capacity of 2 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 3 {{}}
+  (void)lp;
+}
+
+namespace testArrayNew {
+void f() {
+  short s; // expected-note-re {{'s' declared{{.*
+  char *buf = ::new (&s) char[8]; // expected-warning{{Argument of default placement new provides storage capacity of 2 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 3 {{}}
+  (void)buf;
+}
+} // namespace testArrayNew
+
+namespace testBufferInOtherFun {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Argument of default placement new provides storage capacity of 2 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  short buf; // expected-note-re {{'buf' declared{{.*
+  f(&buf); // expected-note 2 {{}}
+}
+} // namespace testBufferInOtherFun
+
+namespace testArrayBuffer {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Argument of default placement new provides storage capacity of 2 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  char buf[2]; // expected-note-re {{'buf' initialized{{.*
+  f(&buf); // expected-note 2 {{}}
+}
+} // namespace testArrayBuffer
+
+namespace testGlobalPtrAsPlace {
+void *gptr = nullptr;
+short gs;
+void f() {
+  gptr = &gs; // expected-note {{Value assigned to 'gptr'}}
+}
+void g() {
+  f(); // expected-note 2 {{}}
+  long *lp = ::new (gptr) long; // expected-warning{{Argument of default placement new provides storage capacity of 2 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testGlobalPtrAsPlace
+
+namespace testRvalue {
+short gs;
+void *f() {
+  return &gs;
+}
+void g() {
+  long *lp = ::new (f()) long; // expected-warning{{Argument of default placement new provides storage capacity of 2 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testRvalue
+
+namespace testNoWarning {
+void *f();
+void g() {
+  long *lp = ::new (f()) long;
+  (void)lp;
+}
+} // namespace testNoWarning
+
+namespace testPtrToArrayAsPlace {
+void f() {
+  //char *st = new char [8];
+  char buf[3]; // expected-note-re {{'buf' initialized{{.*
+  void *st = buf; // expected-note-re {{'st' initialized{{.*
+  long *lp = ::new (st) long; // expected-warning{{Argument of default placement new provides storage capacity of 3 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayAsPlace
+
+namespace testHeapAllocatedBuffer {
+void g2() {
+  char *buf = new char[2]; // expected-note-re {{'buf' initialized{{.*
+  long *lp = ::new (buf) long; // expected-warning{{Argument of default placement new provides storage capacity of 2 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testHeapAllocatedBuffer
Index: clang/test/Analysis/placement-new-user-defined.cpp
===
--- /dev/nul

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Are buffers otherwise modelled by the Analyser, such as results of the `malloc` 
family and `alloca` intentionally not matched, or are there some tests missing 
regarding this?




Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:115-117
+  llvm::formatv("Argument of default placement new provides storage "
+"capacity of {0} bytes, but the allocated type "
+"requires storage capacity of {1} bytes",

This message might be repeating phrases too much, and seems long. Also, I would 
expect things like //default placement new// or //argument of placement new// 
to be confusing. Not every person running Clang SA knows the nitty-gritty of 
the standard by heart...

More nitpicking: even the "default" (what does this even mean, again?) 
placement new takes **two** arguments, albeit written in a weird grammar, so 
there is no "argument of" by the looks of it. I really think this is confusing.

Something more concise, simpler, still getting the meaning across:

> Storage provided to placement new is only `N` bytes, whereas allocating a `T` 
> requires `M` bytes



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

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

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:22
+ProgramStateRef State) const;
+  mutable std::unique_ptr BT_Placement;
+};

I think now it is safe to have the bugtype by value and use member 
initialization.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:35
+  //   long *lp = ::new (buf) long;
+  if (const auto *TVRegion = dyn_cast(MRegion))
+// FIXME Handle offset other than 0. E.g.:

Add braces here.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:40
+if (const auto *ERegion = dyn_cast(TVRegion)) {
+  const auto *SRegion = cast(ERegion->getSuperRegion());
+  DefinedOrUnknownSVal Extent = SRegion->getExtent(svalBuilder);

Hmm, I think this logic might need some more testing. Could you add some tests 
with multi dimensional arrays?



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:44
+  // This is modelled by the MallocChecker.
+  const llvm::APSInt *ExtentInt = svalBuilder.getKnownValue(State, Extent);
+  if (!ExtentInt)

This query will only check if you know the exact value of the target of 
placement you.

What you actually care about if the size is at least as big as the placed 
object.

So instead of getting a known value I think it might be a better idea to 
evaluate a less than or equal operator with `evalBinOp`.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:64
+CheckerContext &C, const CXXNewExpr *NE, ProgramStateRef State) const {
+  if (!State)
+return SVal();

When do you see a null state?



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:72
+  } else {
+ElementCount = svalBuilder.makeIntVal(1, true);
+  }

Probably you could just return the size without building a more complex 
symbolic expression in this case? I.e. just put the right value in the 
makeIntVal.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:79
+
+  if (ElementCount.getAs()) {
+// size in Bytes = ElementCount*TypeSize

You could:
```
Optional NL = ElementCount.getAs();
```

And later you could replace the `castAs` with a deref of the optional.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:81
+// size in Bytes = ElementCount*TypeSize
+SVal SizeInBytes = svalBuilder.evalBinOpNN(
+State, BO_Mul, ElementCount.castAs(),

Prefer to use `evalBinOp` over `evalBinOpNN`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:40
+if (const auto *ERegion = dyn_cast(TVRegion)) {
+  const auto *SRegion = cast(ERegion->getSuperRegion());
+  DefinedOrUnknownSVal Extent = SRegion->getExtent(svalBuilder);

xazax.hun wrote:
> Hmm, I think this logic might need some more testing. Could you add some 
> tests with multi dimensional arrays?
Yeah, this code is scary because at this point literally nobody knows when 
exactly do we an have element region wrapping the pointer 
(https://bugs.llvm.org/show_bug.cgi?id=43364).

`MemRegion` represents a segment of memory, whereas `loc::MemRegionVal` 
represents the point that is the left-hand side of that segment. I recommend 
using `C.getSVal(Place).getAsRegion()` only as a reference to that point, not 
the whole segment. Then you could decompose the region into a base region and 
an offset (i.e., `MemRegion::getAsOffset()`), and subtract the offset from the 
base region's extent to see how much space is there in the region.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:65
+  if (!State)
+return SVal();
+  SValBuilder &svalBuilder = C.getSValBuilder();

That produces an `UndefinedVal`. I think you'd much rather have `UnknownVal`.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:115-117
+  llvm::formatv("Argument of default placement new provides storage "
+"capacity of {0} bytes, but the allocated type "
+"requires storage capacity of {1} bytes",

whisperity wrote:
> This message might be repeating phrases too much, and seems long. Also, I 
> would expect things like //default placement new// or //argument of placement 
> new// to be confusing. Not every person running Clang SA knows the 
> nitty-gritty of the standard by heart...
> 
> More nitpicking: even the "default" (what does this even mean, again?) 
> placement new takes **two** arguments, albeit written in a weird grammar, so 
> there is no "argument of" by the looks of it. I really think this is 
> confusing.
> 
> Something more concise, simpler, still getting the meaning across:
> 
> > Storage provided to placement new is only `N` bytes, whereas allocating a 
> > `T` requires `M` bytes
> 
Having long messages is usually not a problem for us (instead, we'd much rather 
have full sentences properly explaining what's going on), but i agree that your 
text is much neater and on point.



Comment at: clang/test/Analysis/placement-new.cpp:6
+// RUN:   -analyzer-output=text -verify \
+// RUN:   -triple x86_64-unknown-linux-gnu
+

Wow, somebody actually remembers to add a target triple for tests that depend 
on the target triple! My respect, sir.



Comment at: clang/test/Analysis/placement-new.cpp:11
+void f() {
+  short s; // expected-note-re {{'s' declared{{.*
+  long *lp = ::new (&s) long; // expected-warning{{Argument of default 
placement new provides storage capacity of 2 bytes, but the allocated type 
requires storage capacity of 8 bytes}} expected-note 3 {{}}

I'm legit curious what's hidden behind the regex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I wonder if this checker will find any misuses of placement into 
`llvm::TrailingObjects`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong marked 22 inline comments as done.
martong added a comment.

Thank you guys for the assiduous review!




Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:22
+ProgramStateRef State) const;
+  mutable std::unique_ptr BT_Placement;
+};

xazax.hun wrote:
> I think now it is safe to have the bugtype by value and use member 
> initialization.
Ok, I've made it to be a simple member. Also could remove the `mutable` 
specifier.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:40
+if (const auto *ERegion = dyn_cast(TVRegion)) {
+  const auto *SRegion = cast(ERegion->getSuperRegion());
+  DefinedOrUnknownSVal Extent = SRegion->getExtent(svalBuilder);

NoQ wrote:
> xazax.hun wrote:
> > Hmm, I think this logic might need some more testing. Could you add some 
> > tests with multi dimensional arrays?
> Yeah, this code is scary because at this point literally nobody knows when 
> exactly do we an have element region wrapping the pointer 
> (https://bugs.llvm.org/show_bug.cgi?id=43364).
> 
> `MemRegion` represents a segment of memory, whereas `loc::MemRegionVal` 
> represents the point that is the left-hand side of that segment. I recommend 
> using `C.getSVal(Place).getAsRegion()` only as a reference to that point, not 
> the whole segment. Then you could decompose the region into a base region and 
> an offset (i.e., `MemRegion::getAsOffset()`), and subtract the offset from 
> the base region's extent to see how much space is there in the region.
Thanks! I've done the decomposition of Place to base region and offset. This 
way handling of multi-dimensional arrays is also looking good.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:44
+  // This is modelled by the MallocChecker.
+  const llvm::APSInt *ExtentInt = svalBuilder.getKnownValue(State, Extent);
+  if (!ExtentInt)

xazax.hun wrote:
> This query will only check if you know the exact value of the target of 
> placement you.
> 
> What you actually care about if the size is at least as big as the placed 
> object.
> 
> So instead of getting a known value I think it might be a better idea to 
> evaluate a less than or equal operator with `evalBinOp`.
I removed this hunk, because it is no longer needed once we work with the base 
region and with the offset.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:64
+CheckerContext &C, const CXXNewExpr *NE, ProgramStateRef State) const {
+  if (!State)
+return SVal();

xazax.hun wrote:
> When do you see a null state?
Ok, I removed it.
(I think I got the idea of checking the state from the MallocChecker ... but I 
accept that it can never be null in my case).




Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:65
+  if (!State)
+return SVal();
+  SValBuilder &svalBuilder = C.getSValBuilder();

NoQ wrote:
> That produces an `UndefinedVal`. I think you'd much rather have `UnknownVal`.
I removed the check and with it the return, since @xazax.hun suggests that the 
state can never be null.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:72
+  } else {
+ElementCount = svalBuilder.makeIntVal(1, true);
+  }

xazax.hun wrote:
> Probably you could just return the size without building a more complex 
> symbolic expression in this case? I.e. just put the right value in the 
> makeIntVal.
Ok, I changed that to return directly with the size of the type.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:79
+
+  if (ElementCount.getAs()) {
+// size in Bytes = ElementCount*TypeSize

xazax.hun wrote:
> You could:
> ```
> Optional NL = ElementCount.getAs();
> ```
> 
> And later you could replace the `castAs` with a deref of the optional.
Ok, I've done that.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:115-117
+  llvm::formatv("Argument of default placement new provides storage "
+"capacity of {0} bytes, but the allocated type "
+"requires storage capacity of {1} bytes",

NoQ wrote:
> whisperity wrote:
> > This message might be repeating phrases too much, and seems long. Also, I 
> > would expect things like //default placement new// or //argument of 
> > placement new// to be confusing. Not every person running Clang SA knows 
> > the nitty-gritty of the standard by heart...
> > 
> > More nitpicking: even the "default" (what does this even mean, again?) 
> > placement new takes **two** arguments, albeit written in a weird grammar, 
> > so there is no "argument of" by the looks of it. I really think this is 
> > confusing.
> > 
> > Something more concise, simpler, still getting the mea

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 234857.
martong marked 8 inline comments as done.
martong added a comment.

- Bugtype by value
- Decompose Place to base region and offset
- Add test for type hierarchy
- Remove State check
- Return directly with the size of the type in case of non-array new
- Use Optional for ElementCount
- Update warning message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new-user-defined.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- /dev/null
+++ clang/test/Analysis/placement-new.cpp
@@ -0,0 +1,141 @@
+// RUN: %clang_analyze_cc1 -std=c++11 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus.NewDelete \
+// RUN:   -analyzer-checker=cplusplus.PlacementNew \
+// RUN:   -analyzer-output=text -verify \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  long *lp = ::new (&s) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)lp;
+}
+
+namespace testArrayNew {
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  char *buf = ::new (&s) char[8]; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)buf;
+}
+} // namespace testArrayNew
+
+namespace testBufferInOtherFun {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  short buf; // expected-note {{'buf' declared without an initial value}}
+  f(&buf);   // expected-note 2 {{}}
+}
+} // namespace testBufferInOtherFun
+
+namespace testArrayBuffer {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  char buf[2]; // expected-note {{'buf' initialized here}}
+  f(&buf); // expected-note 2 {{}}
+}
+} // namespace testArrayBuffer
+
+namespace testGlobalPtrAsPlace {
+void *gptr = nullptr;
+short gs;
+void f() {
+  gptr = &gs; // expected-note {{Value assigned to 'gptr'}}
+}
+void g() {
+  f();  // expected-note 2 {{}}
+  long *lp = ::new (gptr) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testGlobalPtrAsPlace
+
+namespace testRvalue {
+short gs;
+void *f() {
+  return &gs;
+}
+void g() {
+  long *lp = ::new (f()) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testRvalue
+
+namespace testNoWarning {
+void *f();
+void g() {
+  long *lp = ::new (f()) long;
+  (void)lp;
+}
+} // namespace testNoWarning
+
+namespace testPtrToArrayAsPlace {
+void f() {
+  //char *st = new char [8];
+  char buf[3];// expected-note {{'buf' initialized here}}
+  void *st = buf; // expected-note {{'st' initialized here}}
+  long *lp = ::new (st) long; // expected-warning{{Storage provided to placement new is only 3 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayAsPlace
+
+namespace testPtrToArrayWithOffsetAsPlace {
+void f() {
+  int buf[3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf + 2) long; // expected-warning{{Storage provided to placement new is only 4 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayWithOffsetAsPlace
+
+namespace testHeapAllocatedBuffer {
+void g2() {
+  char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testHeapAllocatedBuffer
+
+namespace testMultiDimensionalArray {
+void f() {
+  char buf[2][3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 6 byte

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:32
+  const MemRegion *BaseRegion = MRegion->getBaseRegion();
+  assert(BaseRegion == Offset.getRegion());
+

This assertion fails on real code quite often (e.g. on LLVM/Clang code). I 
don't really understand why. @NoQ what is you understanding on this? Perhaps I 
could try to reduce a case from the real code to see an example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 234901.
martong added a comment.

- Better handling of unknown values


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new-user-defined.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- /dev/null
+++ clang/test/Analysis/placement-new.cpp
@@ -0,0 +1,141 @@
+// RUN: %clang_analyze_cc1 -std=c++11 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus.NewDelete \
+// RUN:   -analyzer-checker=cplusplus.PlacementNew \
+// RUN:   -analyzer-output=text -verify \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  long *lp = ::new (&s) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)lp;
+}
+
+namespace testArrayNew {
+void f() {
+  short s;// expected-note {{'s' declared without an initial value}}
+  char *buf = ::new (&s) char[8]; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
+  (void)buf;
+}
+} // namespace testArrayNew
+
+namespace testBufferInOtherFun {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  short buf; // expected-note {{'buf' declared without an initial value}}
+  f(&buf);   // expected-note 2 {{}}
+}
+} // namespace testBufferInOtherFun
+
+namespace testArrayBuffer {
+void f(void *place) {
+  long *lp = ::new (place) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+void g() {
+  char buf[2]; // expected-note {{'buf' initialized here}}
+  f(&buf); // expected-note 2 {{}}
+}
+} // namespace testArrayBuffer
+
+namespace testGlobalPtrAsPlace {
+void *gptr = nullptr;
+short gs;
+void f() {
+  gptr = &gs; // expected-note {{Value assigned to 'gptr'}}
+}
+void g() {
+  f();  // expected-note 2 {{}}
+  long *lp = ::new (gptr) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testGlobalPtrAsPlace
+
+namespace testRvalue {
+short gs;
+void *f() {
+  return &gs;
+}
+void g() {
+  long *lp = ::new (f()) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testRvalue
+
+namespace testNoWarning {
+void *f();
+void g() {
+  long *lp = ::new (f()) long;
+  (void)lp;
+}
+} // namespace testNoWarning
+
+namespace testPtrToArrayAsPlace {
+void f() {
+  //char *st = new char [8];
+  char buf[3];// expected-note {{'buf' initialized here}}
+  void *st = buf; // expected-note {{'st' initialized here}}
+  long *lp = ::new (st) long; // expected-warning{{Storage provided to placement new is only 3 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayAsPlace
+
+namespace testPtrToArrayWithOffsetAsPlace {
+void f() {
+  int buf[3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf + 2) long; // expected-warning{{Storage provided to placement new is only 4 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testPtrToArrayWithOffsetAsPlace
+
+namespace testHeapAllocatedBuffer {
+void g2() {
+  char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testHeapAllocatedBuffer
+
+namespace testMultiDimensionalArray {
+void f() {
+  char buf[2][3];  // expected-note {{'buf' initialized here}}
+  long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 6 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
+  (void)lp;
+}
+} // namespace testMultiDimensionalArray
+
+namespace testMultiDimensionalArray2 {
+void f() {
+  char buf[2][3];  // expected-note {{'buf' ini

[PATCH] D71612: [analyzer] Add PlacementNewChecker

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

xazax.hun added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:472
+def PlacementNewChecker : Checker<"PlacementNew">,
+  HelpText<"Check if default placement new is provided with pointers to "
+   "sufficient storage capacity">,

Probably you want to add documentation to `clang/docs/analyzer/checkers.rst` as 
well for better visibility.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:51
+SVal PlacementNewChecker::getExtentSizeOfNewTarget(
+CheckerContext &C, const CXXNewExpr *NE, ProgramStateRef State) const {
+  SValBuilder &SvalBuilder = C.getSValBuilder();

A very minor nit, but checker APIs tend to have `CheckerContext` as the last 
parameter. Maybe following this guideline within the checkers as well makes 
them a bit more natural.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:59
+SVal ElementCount = C.getSVal(SizeExpr);
+Optional ElementCountNL = ElementCount.getAs();
+if (ElementCountNL) {

You can move this line into the if condition.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs();
+  if (!SizeOfTargetCI)

Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as `ConcreteInt`s, I 
think you should rather use `evalBinOp` to compare them. That method is more 
future proof as if we cannot constraint these values down to a single integer 
but we still have some information about them a sufficiently smart solver could 
prove the relationship between the symbolic values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:32
+  const MemRegion *BaseRegion = MRegion->getBaseRegion();
+  assert(BaseRegion == Offset.getRegion());
+

martong wrote:
> This assertion fails on real code quite often (e.g. on LLVM/Clang code). I 
> don't really understand why. @NoQ what is you understanding on this? Perhaps 
> I could try to reduce a case from the real code to see an example.
`RegionOffset` cannot really represent symbolic offsets :( You should be able 
to re-add the assertion after checking for symbolic offsets.

And, yeah, you can always easily reduce any crash with `creduce`.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:22
+ProgramStateRef State) const;
+  mutable std::unique_ptr BT_Placement;
+};

martong wrote:
> xazax.hun wrote:
> > I think now it is safe to have the bugtype by value and use member 
> > initialization.
> Ok, I've made it to be a simple member. Also could remove the `mutable` 
> specifier.
And use a default initializer instead of constructor >.>
`BuiltinBug BT_Placement{this, "Insufficient storage BB"}`

(also i don't understand `BuiltinBug` and i'm afraid of using it, can we just 
have `BugType` with explicit description and category?)



Comment at: clang/test/Analysis/placement-new.cpp:11
+void f() {
+  short s; // expected-note-re {{'s' declared{{.*
+  long *lp = ::new (&s) long; // expected-warning{{Argument of default 
placement new provides storage capacity of 2 bytes, but the allocated type 
requires storage capacity of 8 bytes}} expected-note 3 {{}}

martong wrote:
> NoQ wrote:
> > I'm legit curious what's hidden behind the regex.
> Ok, I removed the regexes.
> But keep in mind that, this note is coming from `trackExpressionValue` and is 
> changing if the initializer of `s` is changing.
> I did not want to formulate expectations on a note that is coming from an 
> implementation detail and can change easily. Also, If `trackExpressionValue` 
> changes than we need to rewrite all these tests, unless a regex is used, 
> perhaps just a simple expected-note {{}} would be the best from this point of 
> view.
Because it's up to the checker whether to use `trackExpressionValue` or provide 
its own visitor, i'd rather keep the notes tested. If the notes provided by 
`trackExpressionValue` aren't good enough, it's ultimately the checker's 
problem (which may or may not be fixed by improving `trackExpressionValue`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

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

Szelethus added inline comments.
Herald added subscribers: ASDenysPetrov, steakhal.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:1
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"

Header blurb is missing :) Could you sneak it in there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:1
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"

Szelethus wrote:
> Header blurb is missing :) Could you sneak it in there?
Sure, I am getting right on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D71612#1828059 , @NoQ wrote:

> This looks fantastic. Let's enable by it default!


Ok, I've done that: https://github.com/llvm/llvm-project/commit/bc29069dc40


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

🎉🎉🎉


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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