[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!