This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Deadstore static analysis: Fix false positive on C++17 assignments
ClosedPublic

Authored by frederic-tingaud-sonarsource on May 27 2022, 6:08 AM.

Details

Summary

Dead store detection automatically checks that an expression is a CXXConstructor and skips it because of potential side effects. In C++17, with guaranteed copy elision, this check can fail because we actually receive the implicit cast of a CXXConstructor.
Most checks in the dead store analysis were already stripping all casts and parenthesis and those that weren't were either forgotten (like the constructor) or would not suffer from it, so this patch proposes to factorize the stripping.
It has an impact on where the dead store warning is reported in the case of an explicit cast, from

auto a = static_cast<B>(A());
         ^~~~~~~~~~~~~~~~~~~

to

auto a = static_cast<B>(A());
                        ^~~

which we think is an improvement.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 6:08 AM
Herald added a subscriber: martong. · View Herald Transcript
frederic-tingaud-sonarsource requested review of this revision.May 27 2022, 6:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 6:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
martong retitled this revision from Deadstore static analysis: Fix false positive on C++17 assignments to [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments.May 27 2022, 7:09 AM
martong edited reviewers, added: steakhal, NoQ; removed: dcoughlin.

I haven't checked the implementation; the Deadstores checker was always a nemesis for me.

Off the top of my head, I think in the analyzer we have the elide-constructors=true/false analyzer config option, which quotes:

Whether elidable C++ copy-constructors and move-constructors should be actually elided during analysis. Both behaviors are allowed by the C++ standard, and the analyzer, like CodeGen, defaults to eliding. Starting with C++17 some elisions become mandatory, and in these cases the option will be ignored.

You also mentioned this copy elision, but I don't see anywhere checks for the standard version in code. I would expect that your change should apply only to C++17 and above.

clang/test/Analysis/dead-stores.cpp
63–68

So we don't warn for these since the opaque ctor call might introduce some side-effects.
Please note this after the no-warning tag.

frederic-tingaud-sonarsource marked an inline comment as done.May 27 2022, 7:56 AM

Thanks for looking at the PR!

You also mentioned this copy elision, but I don't see anywhere checks for the standard version in code. I would expect that your change should apply only to C++17 and above.

The false-positive due to copy elision in C++17 is the main pain point that this patch fixes, which is why I mention it in the title, but it does in fact also solve other cases for all C++ versions if there are parentheses or casts involved.

NoQ added a comment.May 31 2022, 4:57 PM

Oh nice, I think I've heard about such problems, thanks a lot.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
108

It looks like this introduces a change in behavior in unrelated situations, eg. the ones in objc-arc.m.plist where the diagnostic is moved to the right:

id obj1 = (__bridge_transfer id)CFCreateSomething(); // expected-warning{{never read}}
          ^before               ^after

I think this change is undesirable because it distracts from the assignment. In this case you can easily avoid this problem by using IgnoreParenImpCasts() instead, but it also generally makes sense to have patches that target specific situations to match on these situations more precisely. Eg., could you see if you can add a check that a constructor is involved, and maybe check for the exact cast kind that we want to unwrap?

Thanks @NoQ!

The change in behavior is indeed what I was mentioning in the summary. After discussing it with a colleague, it seemed to us that pointing to a more precise range where the object is allocated was an improvement. I understand your point about pointing at something that is nearer to the assignment, but in that case, wouldn't it make sense to point at the whole assignment?

id obj1 = (__bridge_transfer id)CFCreateSomething(); // expected-warning{{never read}}
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We already detect the presence of a constructor, but to skip the report altogether as constructors could have undetected side effects.

It might also be interesting to think about cases that went through transitive assignment stripping when talking about the diagnostic position:
Before my patch, a dead store on variable a was displayed as follows:

B b;
A a = static_cast<A>(b = createB());
                         ^~~~~~~~~

(I'm switching back to C++ because I'm not sure exactly how legal such a thing would be in objective-C).
With the current fix, that doesn't change.
It might be clearer for the user to either put the diagnostic on the whole assignment or at least the whole initialization.

Update diagnostic location to highlight the whole content of the initialization.

Adding an additional false negative fixed by this patch.

frederic-tingaud-sonarsource marked an inline comment as done.Jul 18 2022, 10:40 AM

Hi @NoQ ,
Do the changes correspond to what you had in mind?

Looks good to me, but I think you should wait for @NoQ for approval as he had some concerns previously.
@NoQ, do you think it's in a good shape for landing?

Hi @NoQ . Would you have time to look if the changes I did to this PR solve your concerns?

steakhal accepted this revision.Aug 18 2022, 1:16 AM

It feels like there is no objection. I think it looks great and you did everything you could to address the review comments.
I believe we should not postpone this any further.
Consider committing this tomorrow, unless in the meantime @NoQ objects.

This revision is now accepted and ready to land.Aug 18 2022, 1:16 AM

This change affected analyzer findings for C language source as well. Mostly true positives, but there is one false positive we know about. I filed https://github.com/llvm/llvm-project/issues/57434 to track that new false positive.