This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Add transfer functions for data members and this pointers
ClosedPublic

Authored by sgatev on Jan 11 2022, 4:18 AM.

Details

Summary

This is part of the implementation of the dataflow analysis framework.
See "[RFC] A dataflow analysis framework for Clang AST" on cfe-dev.

Diff Detail

Event Timeline

sgatev created this revision.Jan 11 2022, 4:18 AM
sgatev requested review of this revision.Jan 11 2022, 4:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2022, 4:18 AM
sgatev updated this revision to Diff 398907.Jan 11 2022, 4:23 AM

Add missing include.

ymandel retitled this revision from [clang][dataflow] Add transfer functions data members and this pointers to [clang][dataflow] Add transfer functions for data members and this pointers.Jan 11 2022, 9:15 AM
ymandel accepted this revision.Jan 11 2022, 11:10 AM

Thanks!

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
57

nit: invert the if and return?

clang/lib/Analysis/FlowSensitive/Transfer.cpp
144

nit: maybe comment to explain why we use this setting?

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
834

maybe dyn_cast with ASSERT? (or somesuch)

919

Why EXPECT here but ASSERT (as last line) in previous tests?

1040

dyn_cast and ASSERT (or somesuch)? I'd think that whether or not we have a registered this in the environment is something we want to test?

1059

why test a class distinct from a struct? Is there a meaningful difference between the two with regard to our model? if so, might it be worth instead testing explicit public vs private?

This revision is now accepted and ready to land.Jan 11 2022, 11:10 AM
xazax.hun added inline comments.Jan 11 2022, 11:41 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
120

It feels a bit wrong to have a separate ThisPointeeLoc here. But as far as I understand, this is an artifact of the AST representation we have. I think a superior AST representation would have an implicit ParmVarDecl to represent this, so we would not need to do any special handling at all, the general code path would do the right thing. I think the C++ language, with the deducing this started its approach in this direction officially. I really hope someone will have the time to fix all the mess and get rid of all the unnecessary corner cases in the foreseeable future :)

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
52

There might be an optimization opportunity for initValueInStorageLocation. If we initialize a class or struct, we will end up create locations for all of the fields recursively (unless self-referential) right? Actually, in many of the codebases we cannot even access many of those fields because they might be private/protected etc. Unfortunately, it might not be easy to query whether a field is accessible from this method, but I wonder if it is worth a TODO somewhere.

clang/lib/Analysis/FlowSensitive/Transfer.cpp
130

Nit: I got confused for a second what will happen in a loop. I wonder if createStorageLocation is better renamed to createOrGetStorageLocation to express the fact it will not always create a new location. But I don't have strong feelings about this, also feel free to defer to a later PR.

140–141

I wonder if we also want to create a non-null pointer value for these in the future so we can evaluate certain if statements.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
813

I wonder if we can make the tests a bit more concise by merging some of them. E.g. we could have a single test with both a pointer, a reference, and a value param. Although I understand that some people like to keep most tests minimal, so feel free to ignore this.

1062

I'd love to see a test with multiple fields and a nested struct.

sgatev updated this revision to Diff 399094.Jan 11 2022, 2:58 PM
sgatev marked 12 inline comments as done.

Address reviewers' comments.

Thank you both for the reviews!

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
52

Right. I added a FIXME in initValueInStorageLocationUnlessSelfReferential.

57

I prefer to keep the block self-contained as we might end up adding code after the outer if statement.

clang/lib/Analysis/FlowSensitive/Transfer.cpp
130

Ack. I added a FIXME.

140–141

I added a FIXME.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
813

I'll keep the tests separate for now, but I'll think about how to make them more concise.

919

My bad. I updated all tests to use ASSERT only where necessary.

1059

For completeness. Changing the isStructureOrClassType predicate in initValueInStorageLocationUnlessSelfReferential to isStructureType or isClassType should make one of these tests fail. I'll address public and private members in a follow up.

1062

I added a nested struct and an extra member.

xazax.hun accepted this revision.Jan 11 2022, 3:52 PM
xazax.hun added inline comments.Jan 11 2022, 3:55 PM
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
1034

A chained access would be nice, like Qux.Baz.

sgatev updated this revision to Diff 399219.Jan 11 2022, 11:18 PM

Address reviewers' comments.

sgatev marked an inline comment as done.Jan 11 2022, 11:18 PM
This revision was landed with ongoing or failed builds.Jan 11 2022, 11:44 PM
This revision was automatically updated to reflect the committed changes.