Page MenuHomePhabricator

[analyzer] Deprecate `-analyzer-store region` flag
ClosedPublic

Authored by steakhal on May 23 2022, 9:11 AM.

Details

Summary

I'm trying to remove unused options from the Analyses.def file, then
merge the rest of the useful options into the AnalyzerOptions.def.
Then make sure one can set these by an -analyzer-config XXX=YYY style
flag.
Then surface the -analyzer-config to the clang frontend;

After all of this, we can pursue the tablegen approach described
https://discourse.llvm.org/t/rfc-tablegen-clang-static-analyzer-engine-options-for-better-documentation/61488

In this patch, I'm proposing flag deprecations.
We should support deprecated analyzer flags for exactly one release. In
this case I'm planning to drop this flag in clang-16.

In the clang frontend, now we won't pass this option to the cc1
frontend, rather emit a warning diagnostic reminding the users about
this deprecated flag, which will be turned into error in clang-16.

Unfortunately, I had to remove all the tests referring to this flag,
causing a mass change. I've also added a test for checking this warning.

I've seen that scan-build also uses this flag, but I think we should
remove that part only after we turn this into a hard error.

Diff Detail

Event Timeline

steakhal created this revision.May 23 2022, 9:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
steakhal requested review of this revision.May 23 2022, 9:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 9:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

We should support deprecated analyzer flags for at least one release. In this case I'm planning to drop this flag in clang-17

Should we emit a warning for the user about the deprecation?

steakhal planned changes to this revision.May 23 2022, 11:35 AM

We should support deprecated analyzer flags for at least one release. In this case I'm planning to drop this flag in clang-17

Should we emit a warning for the user about the deprecation?

Definitely!

steakhal updated this revision to Diff 431618.May 24 2022, 1:55 AM
steakhal edited the summary of this revision. (Show Details)
  • Emit a warning when passing this option.
  • State explicitly in the release notes that passing this option to clang-17 and above will cause a hard error.
  • Remove all spellings of this option in tests.
  • Add a test for testing the deprecation warning.

@NoQ, please have a look at this.

clang/test/Analysis/deprecated-flags-and-options.cpp
2–14

This is the new test file. All the rest of the changed test files are unimportant.

martong accepted this revision.May 24 2022, 3:02 AM

LGTM.

This revision is now accepted and ready to land.May 24 2022, 3:02 AM
steakhal updated this revision to Diff 432509.May 27 2022, 3:10 AM
steakhal edited the summary of this revision. (Show Details)
  • Specify exactly that we propose one release deprecation, then removal.
  • Test the help message.
martong accepted this revision.May 27 2022, 5:43 AM

Still looks good.

I believe, the silence from the code owner means that he agrees with this change.
I'll land it tomorrow.

I believe, the silence from the code owner means that he agrees with this change.
I'll land it tomorrow.

I believe there is a misconception about the role of a code owner, we cannot expect Artem to review all patches. According to the LLVM Developer Poicy

The sole responsibility of a code owner is to ensure that a commit to their area of the code is appropriately reviewed, either by themself or by someone else.

Many other analyzer pathces in the past had been reviewed by me and you @steakhal and we didn't receive any complaints after they landed. So, I think it is okay to land this one as well.

This revision was landed with ongoing or failed builds.Fri, Jun 10, 3:58 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Fri, Jun 10, 4:13 AM

Looks like this breaks building clang-tidy: http://45.33.8.238/linux/78232/step_4.txt

Please take a look, and revert for now if it takes a while to fix.

Looks like this breaks building clang-tidy: http://45.33.8.238/linux/78232/step_4.txt

Please take a look, and revert for now if it takes a while to fix.

Thanks. It should be fixed by now.

steakhal updated this revision to Diff 436124.Sat, Jun 11, 3:28 AM
  • Add the new diag::warn_analyzer_deprecated_option warning to the "deprecated-static-analyzer-flag" DiagGroup to prevent breaking the clang/test/Misc/warning-flags.c test file.

I'm also adding @thakis to review this change, prior to proceeding.

steakhal reopened this revision.Sat, Jun 11, 3:29 AM
steakhal added a reviewer: thakis.
This revision is now accepted and ready to land.Sat, Jun 11, 3:29 AM
steakhal requested review of this revision.Sat, Jun 11, 3:29 AM

(As long as this builds and passes tests, I don't have an opinion on this change.)

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jun 14, 12:21 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.