This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mantognini on Apr 28 2022, 8:44 AM.

Details

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

Diff Detail

Event Timeline

mantognini created this revision.Apr 28 2022, 8:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 8:44 AM
mantognini published this revision for review.Apr 29 2022, 12:45 AM

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?

Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2022, 12:45 AM
steakhal accepted this revision.Apr 29 2022, 1:22 AM

LGTM

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
83
87
This revision is now accepted and ready to land.Apr 29 2022, 1:22 AM

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?

Update commit message and add FIXMEs

mantognini marked 2 inline comments as done.Apr 29 2022, 9:05 AM

LGTM

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

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.

mantognini retitled this revision from CPP-2461 [Analyzer] Fix assumptions about const field with member-initializer to [Analyzer] Fix assumptions about const field with member-initializer.Apr 29 2022, 9:06 AM
mantognini edited the summary of this revision. (Show Details)
steakhal accepted this revision.Apr 29 2022, 9:25 AM

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?

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.

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 accepted this revision.May 2 2022, 9:52 AM

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

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.

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!