This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Add analysis that detects unsafe accesses to optionals
ClosedPublic

Authored by sgatev on Mar 8 2022, 1:57 AM.

Details

Summary

Adds a dataflow analysis that detects unsafe accesses to values of type
std::optional, absl::optional, or base::Optional.

Also moves TestingSupport.h|cc out of the unittest directory so that it
can be used by model tests.

Diff Detail

Event Timeline

sgatev created this revision.Mar 8 2022, 1:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 1:57 AM
sgatev requested review of this revision.Mar 8 2022, 1:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
sgatev updated this revision to Diff 413743.Mar 8 2022, 2:17 AM

Mark functions as static and add TODOs.

Maybe add explanation to the patch description for move of TestingSupport.h? Also, mention that this just a core implementation, and fuller support is coming?

clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
21

Delete?

36–37
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
85

How are we confident that the has_value property is populated? Since we're only picking up on declr-refs and members, I think this won't hold otherwise.

sgatev updated this revision to Diff 413852.Mar 8 2022, 9:51 AM
sgatev marked 2 inline comments as done.

Address reviewers' comments.

sgatev edited the summary of this revision. (Show Details)Mar 8 2022, 9:52 AM
sgatev added inline comments.
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
21

Why? I do think it makes sense to separate the model of optional types from the analysis that tracks unsafe uses. I don't think we're in a place to do the separation yet so that's why I added the FIXME.

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
85

I think that's fine. This isn't set in stone and can change as new tests are added. I don't claim that the implementation in this patch covers all features of the language.

ymandel accepted this revision.Mar 8 2022, 10:06 AM
ymandel added inline comments.
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
21

Sorry, I misread it. Fine as is.

This revision is now accepted and ready to land.Mar 8 2022, 10:06 AM
xazax.hun accepted this revision.Mar 8 2022, 4:41 PM
xazax.hun added inline comments.
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
116

One very important omission seems to be optional::operator bool. This is a widely used method and I'd love to see it supported.

Also would love to see FIXMEs for some of the most more frequently used functions/methods:

  • make_optional makes a non-empty optional
  • swap (free function and method)
  • value_or returning its argument when the optional is known to be empty
  • Model the value semantics: Copy ctor, assignment operator
  • Model the move semantics
  • Default constructed optional is empty
  • Invalidation: passing optional by non-const reference/pointer can invalidate its state
sgatev updated this revision to Diff 414029.Mar 9 2022, 1:05 AM
sgatev marked 2 inline comments as done.

Address reviewers' comments.

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
116

Most of these will be implemented in follow up patches soon and you can already find the list in the FIXME in UncheckedOptionalAccessModelTest.cpp. I added to it invalidation so that it covers all features you mentioned here.

This revision was landed with ongoing or failed builds.Mar 9 2022, 1:45 AM
This revision was automatically updated to reflect the committed changes.

Seems like all new files are missing the header blurb about the licence.

Seems like all new files are missing the header blurb about the licence.

Thanks for spotting that! I'll prepare a patch to fix it.