Page MenuHomePhabricator

[analyzer] Add a checker option to detect nested dead stores
ClosedPublic

Authored by steakhal on Mon, Aug 26, 4:47 AM.

Details

Summary

Enables the users to specify an optional flag which would warn for more dead stores.
Previously it ignored if the dead store happened e.g. in an if condition.

if ((X = generate())) { // dead store to X
}

This patch introduces the WarnForDeadNestedAssignments option to the checker, which is false by default - so this change would not affect any previous users.
I have updated the code, tests and the docs as well. If I missed something, tell me.

I also ran the analysis on Clang which generated 14 more reports compared to the unmodified version. All of them seemed reasonable for me.
Shouldn't we enable this option by default?
You can check those at this CodeChecker instance. Note that this link will no longer work after the patch has been accepted.

Related previous patches:
rGf224820b45c6847b91071da8d7ade59f373b96f3

Diff Detail

Repository
rL LLVM

Event Timeline

steakhal created this revision.Mon, Aug 26, 4:47 AM
Szelethus accepted this revision.Mon, Aug 26, 5:09 AM

I really-really like this change. The results look great, though I'm not sure what happened here, are we sure that this originates from the analyzer, and is not some storing error?

In any case, please use clang-format-diff.py on this patch, otherwise LGTM.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
644 ↗(On Diff #217089)

Mmm, how about we soften this warning? :)

"Enabling this may result in some false positive finds.

646 ↗(On Diff #217089)

We mark checker options that are still in development with InAlpha, but I feel like this feature is as good as it gets. Feel free to mark it as Released, so it'll be user facing!

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
223 ↗(On Diff #217089)

Could you please move this comment to the previous line?

This revision is now accepted and ready to land.Mon, Aug 26, 5:09 AM
steakhal marked 3 inline comments as done.Mon, Aug 26, 8:31 AM

Thank you for your response @Szelethus.
Fixed, updating the patch.

steakhal updated this revision to Diff 217162.Mon, Aug 26, 8:34 AM

Reformatted using clang-format-diff.py.
Minor fixes which were requested by @Szelethus.

steakhal updated this revision to Diff 217164.Mon, Aug 26, 8:39 AM

Fix copy-paste mistake.
This time upload the correct version.

@NoQ What do you think, should it be under a flag (as it would be now), or enabled by default?
I think these warnings are valuable and we should consider it enabling by default.
An interesting fact is that previously rGf224820b45c6847b91071da8d7ade59f373b96f3 patch disabled this warning saying that it generates too many false-positives without any real benefit.

But after seeing the reports on the LLVM codebase (CodeChecker instance for the diff) I still question this decision. What is your opinion?

Any comment how could this result in false-positives are welcomed.

@Szelethus Your catch with the mispositioned report message is kinda strange. I will investigate that, but I think it's probably connected to something deeper, and most likely related to CodeChecker itself.

@Szelethus The mispositioned report message was my fault. I used a different version of clang for the analysis and to upload the results, which resulted in some mispositioned reports.
I've fixed the linked CodeChecker instance.

NoQ added a comment.Tue, Aug 27, 6:10 PM

Mmm, i don't know what was that commit meant to fix. Your evaluation looks fairly sufficient for turning it on by default. Let's make it an on-by-default option and flip the flag back into off-by-default if it turns out to be somehow broken.

clang/docs/analyzer/checkers.rst
299 ↗(On Diff #217164)

several?

clang/test/Analysis/dead-stores.c
81 ↗(On Diff #217164)

Pls consider -verify=... for new tests (cf. D60732).

@Szelethus The mispositioned report message was my fault. I used a different version of clang for the analysis and to upload the results, which resulted in some mispositioned reports.
I've fixed the linked CodeChecker instance.

If you want to see how the analyzer behaves, I advise to keep an LLVM repository purely for analysis that you're not ever refreshing, and a separate one for building your clang :)

steakhal marked 2 inline comments as done.Wed, Aug 28, 8:48 AM

Fixes for @NoQ's comments.
I will update the patch.

steakhal updated this revision to Diff 217665.Wed, Aug 28, 9:04 AM

Changes:

  • Flag option marked as 'enabled by default'.
  • Reformat all the test cases for C, C++ and Obj C.
  • Now uses -verify=tags approach.
  • Fixes checker documentation.
Szelethus accepted this revision.Wed, Aug 28, 10:54 AM

Excellent detective work! Thanks! I'll do the honors.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 3, 8:24 AM
NoQ added inline comments.Tue, Sep 3, 2:17 PM
cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
133

I suggest we adopt the idiom of passing the Checker object around and asking it about its options instead of passing each option around separately. This is easier and i don't see any downsides. Moreover, we already pass the Checker around (just type-erased for no good reason). If you don't mind, i'll remove this field as part of resolving merge conflicts in D65182.

Szelethus added inline comments.Tue, Sep 3, 2:24 PM
cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
133

What about subcheckers? In any case, feel free to remove it for now.

Szelethus added inline comments.Tue, Sep 3, 8:29 PM
cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
133

What type erasure do you talk about specifically? In any case, it might happen because of our library layout, I had a bad time with that when I did the checker registration thingie.

NoQ added inline comments.Tue, Sep 3, 10:42 PM
cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
133

I meant const DeadStoresChecker * -> const CheckerBase *.

What about subcheckers?

As long as they're represented by the same checker object that's defined within the same translation unit, this isn't a problem. When they're multiple checker objects - dunno, i've honestly never seen that happen (:

In any case, i don't think it's a hard requirement, just an always-easier-thing-to-implement.