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

[mantognini]

Summary

Essentially, having a default member initializer for a constant member
does not necessarily imply the member will have the given default value.

Remove part of a2e053638bbf ([analyzer] Treat more const variables and
fields as known contants., 2018-05-04).

Fix #47878

[mantognini]

One thing I'm not sure about and couldn't easily find in the doc is how to reference in the commit message the bug (https://llvm.org/PR48534) this patch fixes. Is it good as is?

[steakhal]

LGTM

In D124621#3481906, @mantognini wrote:

One thing I'm not sure about and couldn't easily find in the doc is how to reference in the commit message the bug (https://llvm.org/PR48534) this patch fixes. Is it good as is?

AFAIK we should prefer GitHub issue numbers to the old BugZilla numbers.
Could you please update all references of 48534 -> 47878.

In addition, I think Fixes #47878 should work in the commit message, and automatically close the given GitHub issue.

Please wait for another accept. If you don't get one let's say for a week, just commit it as-is.

BTW have you measured the observable impact of this patch on large codebases? Do you have any stats?

clang/test/Analysis/cxx-member-initializer-const-field.cpp

84

88

[steakhal]

Noq wrote https://github.com/llvm/llvm-project/issues/47878#issuecomment-981036634

Aha, uhm, yeah, i see. The static analyzer indeed thinks that a combination of "const" and a field initializer causes the field to forever stay that way. We'll need to undo this relatively recently added shortcut.

What "recently added shortcut" did he mention? Could you please refer to that commit in the patch summary, please?

[mantognini]

Update commit message and add FIXMEs

[mantognini]

In D124621#3481973, @steakhal wrote:

LGTM

In D124621#3481906, @mantognini wrote:

One thing I'm not sure about and couldn't easily find in the doc is how to reference in the commit message the bug (https://llvm.org/PR48534) this patch fixes. Is it good as is?

AFAIK we should prefer GitHub issue numbers to the old BugZilla numbers.
Could you please update all references of 48534 -> 47878.

In addition, I think Fixes #47878 should work in the commit message, and automatically close the given GitHub issue.

Thanks for the feedback!

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. :-)

In D124621#3481981, @steakhal wrote:

Noq wrote https://github.com/llvm/llvm-project/issues/47878#issuecomment-981036634

Aha, uhm, yeah, i see. The static analyzer indeed thinks that a combination of "const" and a field initializer causes the field to forever stay that way. We'll need to undo this relatively recently added shortcut.

What "recently added shortcut" did he mention? Could you please refer to that commit in the patch summary, please?

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.

[steakhal]

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!

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1982–1984

@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?

[mantognini]

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.

[steakhal]

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.

[r.stahl]

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

[mantognini]

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.

[steakhal]

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!

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

if (const TypedValueRegion *baseR =
     }
   }
   return getBindingForFieldOrElementCommon(B, R, R->getElementType());
 }
 
 SVal RegionStoreManager::getBindingForField(RegionBindingsConstRef B,
                                             const FieldRegion* R) {
 
   // Check if the region has a binding.
   if (const Optional<SVal> &V = 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<SVal> 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<VarRegion>(superR)) {
     const VarDecl *VD = VR->getDecl();
     QualType RecordVarTy = VD->getType();
     unsigned Index = FD->getFieldIndex();
     // Either the record variable or the field has an initializer that we can
     // trust. We trust initializers of constants and, additionally, respect
     // initializers of globals when analyzing main().

clang/test/Analysis/cxx-member-initializer-const-field.cpp

+// 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}}
+  }
+};