Page MenuHomePhabricator

[clang][dataflow] Add base types for building dataflow analyses
AcceptedPublic

Authored by sgatev on Fri, Nov 19, 4:32 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.Fri, Nov 19, 4:32 AM
sgatev requested review of this revision.Fri, Nov 19, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 19, 4:32 AM
sgatev updated this revision to Diff 388465.Fri, Nov 19, 4:42 AM

Fix ifndefs.

sgatev updated this revision to Diff 388469.Fri, Nov 19, 4:56 AM

Use triple slash at the start of declaration comments.

ymandel added inline comments.Fri, Nov 19, 6:21 AM
clang/lib/Analysis/FlowSensitive/DataflowAnalysis.cpp
21–22 ↗(On Diff #388469)

It's more common in .cpp files to use using declarations instead:

using clang;
using dataflow;
sgatev updated this revision to Diff 388490.Fri, Nov 19, 6:35 AM

Add using namespace declarations in the cpp file.

sgatev marked an inline comment as done.Fri, Nov 19, 6:38 AM
xazax.hun added inline comments.Fri, Nov 19, 11:45 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
49

Does the Dynamic in the suffix refer to the fact this is using type erasure as opposed to templates?

I have multiple questions at this point:

  • Why do we prefer type erasure over generic programming?
  • Do you plan to have non-dynamic counterparts?

Nit: having Dynamic both in the class name and in the method names sounds overly verbose to me.
Nit: please add a comment what dynamic refers to in the name,

141

Do we need a ctor? Or is this a workaround for aggregate initialization not working well with certain library facilities?

clang/lib/Analysis/FlowSensitive/DataflowAnalysis.cpp
26 ↗(On Diff #388490)

Is there a way to query how the CFG was built? Adding an assert to see if setAlwaysAdd was set might be useful.

sgatev updated this revision to Diff 389166.Tue, Nov 23, 4:31 AM

Remove unnecessary constructor.

sgatev updated this revision to Diff 389189.Tue, Nov 23, 6:31 AM
sgatev marked an inline comment as done.

Add a note about asserting the requirements of the CFG object.

sgatev updated this revision to Diff 389522.Wed, Nov 24, 9:23 AM
sgatev marked an inline comment as done.

Document the role of the "Dynamic" suffix in the name of DataflowAnalysisDynamic and its members.

sgatev updated this revision to Diff 389773.Thu, Nov 25, 6:52 AM

Put typed and type-erased interfaces in separate files.

sgatev updated this revision to Diff 389774.Thu, Nov 25, 7:00 AM

Rename Environment.h to DataflowEnvironment.h.

sgatev added inline comments.Thu, Nov 25, 8:13 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
49

Right. The "Dynamic" suffix emphasizes the type-erased nature of the class and its members and is used to differentiate them from their typed counterparts in DataflowAnalysis. I added this to the documentation of the type. I also split the typed and type-erased interfaces in separate files. Users of the framework shouldn't need to interact with types and functions from DataflowAnalysisDynamic.h.

The names are verbose, but should be used in relatively few internal places in the framework. Currently, we have one call site for each of the methods of DataflowAnalysisDynamic. Nevertheless, if you have ideas for better names I'd be happy to change them.

The reason we went with a type-erased layer is to avoid pulling a lot of code in the templates. Over time the implementation got cleaner and as I'm preparing these patches I see some opportunities to simplify it further. However, it's still non-trivial amount of code. I think we should revisit this decision at some point and consider having entirely template-based implementation of the algorithm. I personally don't see clear benefits of one approach over the other at this point. If you have strong preference for using templates, we can consider going down that route now.

141

We don't really need it at this point. Removed.

clang/lib/Analysis/FlowSensitive/DataflowAnalysis.cpp
26 ↗(On Diff #388490)

Good point. I believe there isn't a way to do that currently, but this is something we should consider implementing. I added a FIXME.

xazax.hun accepted this revision.Thu, Nov 25, 3:21 PM

Thanks, it looks good to me. Most of my comments are just brainstorming, exploring alternative ideas. Feel free to ignore some/all of them.

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
49

Thanks for the explanation! I don't have a strong preference, we could stick with the type-erased version unless we see some reason to change in the future. However, I don't see much value in the "Dynamic" suffix for the method names. What do you think about simply dropping them?

95

If I understand this correctly, this could derive from DataflowAnalysisStateDynamic, it could just provide a getter function that casts the type erased lattice element to LatticeT, returning a reference to the contents of the any object. As a result, you would no longer need to do move/copy in runDataflowAnalysis. On the other hand, the user would need to call a getter to get out the lattice element. I guess we expect lattice elements to be relatively cheap to move. Feel free to leave this unchanged, it is more of an observation.

103

While it is probably obvious to most of us, I wonder if it is obvious to all future readers that the block IDs are the indices of the vector. Depending on how beginner-friendly do we want these comments to be we could make that more explicit.

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisDynamic.h
45 ↗(On Diff #389774)

Alternatively, we could replace Dynamic with TypeErased in the class name making the comment redundant.

This revision is now accepted and ready to land.Thu, Nov 25, 3:21 PM
sgatev updated this revision to Diff 390029.Fri, Nov 26, 5:18 AM
sgatev marked 3 inline comments as done.

Replace "Dynamic" with "TypeErased" in the names of types and their members.

sgatev updated this revision to Diff 390034.Fri, Nov 26, 5:49 AM

Minor tweaks to documentation.

gribozavr2 accepted this revision.Fri, Nov 26, 5:50 AM

Thanks Gábor and Dmitri!

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
49

Well, the only reason I see to have the suffix is to differentiate the type-erased methods from the methods of the concrete dataflow analysis class that will derive from DataflowAnalysis. For example, we could have

class ConstantPropagationAnalysis : public DataflowAnalysis<
  ConstantPropagationAnalysis, ConstantPropagationLattice> {
public:
  ConstantPropagationLattice initialElement();

  ConstantPropagationLattice transfer(
    const Stmt *, const ConstantPropagationLattice &, Environment &);
};

In this setup ConstantPropagationAnalysis has two methods that return the initial element and two methods that perform the transfer function - one pair in the definition and one pair inherited from TypeErasedDataflowAnalysis. For the initial element we need to use different names as the two methods differ only in their return types. For the transfer function we could use the same name and rely on overload resolution, however it seems that's not recommended - https://clang.llvm.org/docs/DiagnosticsReference.html#woverloaded-virtual. The names of the remaining methods (joinTypeErased and isEqualTypeErased) have the suffix for consistency.

95

Acknowledged. I'll keep this option in mind. For now I suggest we keep the typed and type-erased structs separate if possible and if the cost isn't too high.

103

Let's make it clear. I added a note on that.

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisDynamic.h
45 ↗(On Diff #389774)

That would be appropriate indeed. I updated the names.

ymandel accepted this revision.Mon, Nov 29, 6:36 AM