Page MenuHomePhabricator

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

Authored by steakhal on Aug 26 2019, 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.Aug 26 2019, 4:47 AM
Szelethus accepted this revision.Aug 26 2019, 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.Aug 26 2019, 5:09 AM
steakhal marked 3 inline comments as done.Aug 26 2019, 8:31 AM

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

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

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

steakhal updated this revision to Diff 217164.Aug 26 2019, 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.Aug 27 2019, 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.Aug 28 2019, 8:48 AM

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

steakhal updated this revision to Diff 217665.Aug 28 2019, 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.Aug 28 2019, 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 TranscriptSep 3 2019, 8:24 AM
NoQ added inline comments.Sep 3 2019, 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.Sep 3 2019, 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.Sep 3 2019, 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.Sep 3 2019, 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.

@steakhal Thanks for this patch. I tried it on Firefox and it found a bunch of stuff we improved.
Some examples:
https://hg.mozilla.org/integration/autoland/rev/db24db8f5f549ff446d1c3c69799187bcc2409e2
https://hg.mozilla.org/integration/autoland/rev/5de53dab979a401d9ba1405974f691927e53c9bd
(and more to come)

I think it should be added to the release notes!

I added it to the release notes here : https://reviews.llvm.org/rC374593
I am wondering if the option( WarnForDeadNestedAssignments ) to disable it is really necessary?
I haven't seen any false positive while deadstore has some.

I added it to the release notes here : https://reviews.llvm.org/rC374593
I am wondering if the option( WarnForDeadNestedAssignments ) to disable it is really necessary?
I haven't seen any false positive while deadstore has some.

Thank you!! Well, the option was in place in case some false positives do arise from this, but we did suspect that wouldn't happen. I'm not against removing it completely, or hiding it (so that it would only be listed under -analyzer-checker-option-help-developer).

Some of folks working on the analyzer are preparing for the conference, hence the slower response time, sorry :^)

Thank you guys the responses. I cannot agree more. The sole reason why this option exists is that if you scroll back in the git blame of that line, you would find a commit, which removed this warning for this exact scenario.
Possibly due to some seemingly false positives.
I've added the author of that patch to the reviewers of this change, but did not respond.

If you interested to dig deeper, you could start here:
rC123394 same on github: https://github.com/steakhal/llvm-project/commit/f224820b45c6847b91071da8d7ade59f373b96f3

And again, thank you that mentioned it in the release notes and for the kind responses as well.