Page MenuHomePhabricator

[analyzer] Move UninitializedObjectChecker out of alpha
ClosedPublic

Authored by Szelethus on Feb 23 2019, 1:57 AM.

Details

Summary

I've tested the checker on 5 open source projects with 3 different configurations:

http://cc.inf.elte.hu:15420/Default/#

I think there is room for discussion whether which package should this checker reside in, but in terms of development, I would argue that this is ready to go.

Shall I make some other tests?

Diff Detail

Event Timeline

Szelethus created this revision.Feb 23 2019, 1:57 AM
NoQ added a comment.Mar 7 2019, 3:13 PM

At a glance, it seems as if these are great findings for a hardening effort, but most of these don't look like immediate bugs (had a look at 10 or so), so i guess i'd suggest optin.cplusplus, at least for now.

Also, hmm, how about the following heuristics (looked at the positive in rtags and thought about this):

  • if the field is public, don't warn (the structure seems to be used simply as a convenient group of variables?)
  • if all uninitialized fields are in all cases written into immediately after the constructor ends, suppress the warning (i vaguely remember that something similar was already proposed, but i might be wrong)

Also huge thanks for keeping things documented!!

In D58573#1422198, @NoQ wrote:

Also, hmm, how about the following heuristics (looked at the positive in rtags and thought about this):

  • if the field is public, don't warn (the structure seems to be used simply as a convenient group of variables?)

I disagree with this. If anything, public uninitialized fields should be reported the most.

  • if all uninitialized fields are in all cases written into immediately after the constructor ends, suppress the warning (i vaguely remember that something similar was already proposed, but i might be wrong)

I think we'd need dataflow analysis for that. We do suppress reports though where all fields of an object is uninitialized.

In case no bug reports against the checker are reported on the mailing list or Bugzilla, I wholeheartedly agree with kicking the ball here.

As for the package, perhaps we could go into optin.bugprone or optin.conventions? (These are new package names...)

Ping, @NoQ, if we settled on optin.cplusplus, would you be fine with this patch?

NoQ added a comment.Mar 29 2019, 2:18 PM

Ping, @NoQ, if we settled on optin.cplusplus, would you be fine with this patch?

Yup, totally!

One heuristic that we could use is to ignore inherited data members optionally. Does that sound any good?

Szelethus updated this revision to Diff 194667.Apr 11 2019, 5:50 AM
Szelethus retitled this revision from [analyzer] Move UninitializedObject out of alpha to [analyzer] Move UninitializedObjectChecker out of alpha.

Moved the checker to optin.cplusplus.

NoQ accepted this revision.Apr 19 2019, 8:30 AM

Ouch, forgot to accept it. Here you go!

This revision is now accepted and ready to land.Apr 19 2019, 8:30 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the checker! We are hitting a crash on _Atomic fields, could you take a look? https://bugs.llvm.org/show_bug.cgi?id=41590 Thanks!

Thanks for the checker! We are hitting a crash on _Atomic fields, could you take a look? https://bugs.llvm.org/show_bug.cgi?id=41590 Thanks!

Yup, consider it fixed (in a little while)! Thanks for the report!

I suspect the problem is similar. I'll take a look at this either today or tomorrow, thanks!

I took the liberty to add that in the release notes of clang
https://reviews.llvm.org/rG5f163c7e2e62