[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG68ee5ec07d4a: [Analyzer] Fix assumptions about const field 
with member-initializer (authored by mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/cxx-member-initializer-const-field.cpp

Index: clang/test/Analysis/cxx-member-initializer-const-field.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-member-initializer-const-field.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// This tests false-positive issues related to PR48534.
+//
+// Essentially, having a default member initializer for a constant member does
+// not necessarily imply the member will have the given default value.
+
+struct WithConstructor {
+  int *const ptr = nullptr;
+  WithConstructor(int *x) : ptr(x) {}
+
+  static auto compliant() {
+WithConstructor c(new int);
+return *(c.ptr); // no warning
+  }
+
+  static auto compliantWithParam(WithConstructor c) {
+return *(c.ptr); // no warning
+  }
+
+  static auto issue() {
+WithConstructor c(nullptr);
+return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct RegularAggregate {
+  int *const ptr = nullptr;
+
+  static int compliant() {
+RegularAggregate c{new int};
+return *(c.ptr); // no warning
+  }
+
+  static int issue() {
+RegularAggregate c;
+return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct WithConstructorAndArithmetic {
+  int const i = 0;
+  WithConstructorAndArithmetic(int x) : i(x + 1) {}
+
+  static int compliant(int y) {
+WithConstructorAndArithmetic c(0);
+return y / c.i; // no warning
+  }
+
+  static int issue(int y) {
+WithConstructorAndArithmetic c(-1);
+return y / c.i; // expected-warning{{Division by zero}}
+  }
+};
+
+struct WithConstructorDeclarationOnly {
+  int const i = 0;
+  WithConstructorDeclarationOnly(int x); // definition not visible.
+
+  static int compliant1(int y) {
+WithConstructorDeclarationOnly c(0);
+return y / c.i; // no warning
+  }
+
+  static int compliant2(int y) {
+WithConstructorDeclarationOnly c(-1);
+return y / c.i; // no warning
+  }
+};
+
+// NonAggregateFP is not an aggregate (j is a private non-static field) and has no custom constructor.
+// So we know i and j will always be 0 and 42, respectively.
+// That being said, this is not implemented because it is deemed too rare to be worth the complexity.
+struct NonAggregateFP {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+public:
+  static int falsePositive1(NonAggregateFP c) {
+return 10 / c.i; // FIXME: Currently, no warning.
+  }
+
+  static int falsePositive2(NonAggregateFP c) {
+return 10 / (c.j - 42); // FIXME: Currently, no warning.
+  }
+};
+
+struct NonAggregate {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+  NonAggregate(NonAggregate const &); // not provided, could set i and j to arbitrary values.
+
+public:
+  static int compliant1(NonAggregate c) {
+return 10 / c.i; // no warning
+  }
+
+  static int compliant2(NonAggregate c) {
+return 10 / (c.j - 42); // no warning
+  }
+};
+
+struct WithStaticMember {
+  static int const i = 0;
+
+  static int issue1(WithStaticMember c) {
+return 10 / c.i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+
+  static int issue2() {
+return 10 / WithStaticMember::i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+};
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1983,15 +1983,9 @@
   if (const Optional  = B.getDirectBinding(R))
 return *V;
 
-  // Is the field declared constant and has an in-class initializer?
+  // If the containing record was initialized, try to get its constant value.
   const FieldDecl *FD = R->getDecl();
   QualType Ty = FD->getType();
-  if (Ty.isConstQualified())
-if (const Expr *Init = FD->getInClassInitializer())
-  if (Optional V = svalBuilder.getConstantVal(Init))
-return *V;
-
-  // If the containing record was initialized, try to get its constant value.
   const MemRegion* superR = R->getSuperRegion();
   if (const auto *VR = dyn_cast(superR)) {
 const VarDecl *VD = VR->getDecl();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

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

steakhal added a comment.

In D124621#3487507 , @mantognini 
wrote:

> Thanks for digging in the past. I confirm I didn't see any regression in 
> existing tests, so I'll land it.

Go for it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

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

[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D124621#3486004 , @r.stahl wrote:

> I can confirm the issue with my patch, so this piece of code needs to be 
> removed.
>
> As long as the following test still succeeds, this looks good to me. Back 
> then, the analyzer was not able to cover that case without that addition.
>
> https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/globals.cpp#L110

Thanks for digging in the past. I confirm I didn't see any regression in 
existing tests, so I'll land it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

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

[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl accepted this revision.
r.stahl added a comment.

I can confirm the issue with my patch, so this piece of code needs to be 
removed.

As long as the following test still succeeds, this looks good to me. Back then, 
the analyzer was not able to cover that case without that addition.

https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/globals.cpp#L110


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

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

[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

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

steakhal added a comment.

In D124621#3485799 , @mantognini 
wrote:

> Thanks for the tip. I had to fix a thing or two to get SATest.py working with 
> my setup (I'll try to upstream those fixes at some point). However, these 
> projects do not highlight the false-positive being fixed here. I.e., I get no 
> difference in reports with and without this patch. But I'll keep this in mind 
> when working on other improvements.

Okay. Thanks for your commitment.
Please wait for another approval.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

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

[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D124621#3482847 , @steakhal wrote:

> For the upcoming patches, it would be nice to test the patches on a small set 
> of open-source projects for exactly this reason.
> I think there is a `clang/utils/analyzer/SATest.py` script helping you on 
> this part.
> It seems we have quite a few projects on the testset 
> `clang/utils/analyzer/projects/projects.json`.
> We are not using it, because we have a different internal testing 
> infrastructure, but it's definitely better than nothing.

Thanks for the tip. I had to fix a thing or two to get SATest.py working with 
my setup (I'll try to upstream those fixes at some point). However, these 
projects do not highlight the false-positive being fixed here. I.e., I get no 
difference in reports with and without this patch. But I'll keep this in mind 
when working on other improvements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

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

[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

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

steakhal accepted this revision.
steakhal added a comment.

In D124621#3482782 , @mantognini 
wrote:

> In D124621#3481973 , @steakhal 
> wrote:
>
>> BTW have you measured the observable impact of this patch on large 
>> codebases? Do you have any stats?
>
> I can't share the data but I can say it fixes some user reports. :-)

For the upcoming patches, it would be nice to test the patches on a small set 
of open-source projects for exactly this reason.
I think there is a `clang/utils/analyzer/SATest.py` script helping you on this 
part.
It seems we have quite a few projects on the testset 
`clang/utils/analyzer/projects/projects.json`.
We are not using it, because we have a different internal testing 
infrastructure, but it's definitely better than nothing.

> I think @NoQ refers to https://reviews.llvm.org/D45774 but I'll wait for a 
> week or so for confirmation in case there's more to it.

Cool!




Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1982-1984
   // Check if the region has a binding.
   if (const Optional  = B.getDirectBinding(R))
 return *V;

@NoQ I was always puzzled why don't we check if we have default bindings after 
checking direct bindings.
Do you have anything about that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

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