[PATCH] D72810: [LifetimeAnalysis] Add support for lifetime annotations on functions

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 marked an inline comment as done.
gribozavr2 added inline comments.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:22
+
+/// This represents an abstract memory location that is used in the lifetime
+/// contract representation.
----------------
gribozavr2 wrote:
> "/// An abstract memory location..."
> 
> It would also really help to show some examples of the sorts of locations it can express (especially the nested field accesses).
Drop the "this represents", it does not add anything (you can say so about *every* class, "this represents an expression", "this represents a person", "this represents an postal address").

"An abstract memory location that participates in defining a lifetime contract. A lifetime contract constrains lifetime of a LifetimeContractVariable to be at least as big as the lifetime of other LifetimeContractVariables.

The memory locations that we can describe are: return values of a function, `this` pointer, any function parameter, a "drilldown" expression based on function parameters, null etc."



================
Comment at: clang/include/clang/AST/LifetimeAttr.h:29
+public:
+  LifetimeContractVariable(const ParmVarDecl *PVD, int Deref = 0)
+      : ParamIdx(PVD->getFunctionScopeIndex()), Tag(Param) {
----------------
Also a factory, please.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:161
+// lifetime ends in the set.
+using LifetimeSet = std::set<LifetimeContractVariable>;
+// Maps each annotated entity to a lifetime set.
----------------
WDYT about s/LifetimeSet/ObjectLifetimeSet/ ?

Comment:

"A lifetime of a pointee of a specific pointer-like C++ object. This lifetime is represented as a disjunction of different lifetime possibilities (set elements). Each lifetime possibility is specified by naming another object that the pointee can point at."

Also, three slashes for doc comments.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:162
+using LifetimeSet = std::set<LifetimeContractVariable>;
+// Maps each annotated entity to a lifetime set.
+using LifetimeContracts = std::map<LifetimeContractVariable, LifetimeSet>;
----------------
"Lifetime constraints for multiple objects. The key of the map is the pointer-like object, the value is the lifetime of the pointee.

Can be used to describe all lifetime constraints required by a given function, or all lifetimes inferred at a specific program point etc."


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:163
+// Maps each annotated entity to a lifetime set.
+using LifetimeContracts = std::map<LifetimeContractVariable, LifetimeSet>;
+
----------------
Generally, DenseMap and DenseSet are faster. Any reason to not use them instead?


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:166
+namespace process_lifetime_contracts {
+/// This function has the sole purpose of converting the AST representation of a
+/// contract into the LifetimeContracts representation which is easier to work with.
----------------
Cut the fluff.

"Converts an AST of a lifetime contract (that is, the `gtl::lifetime(...) call expression) to a LifetimeContracts object that is used throughout the lifetime analysis.

If the AST does not describe a valid contract, the source range of the erroneous part is returned.

When the function succeeds, the returned SourceRange is invalid."

Having written this out, I'm contemplating whether the return value should be an optional of a source range.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3342
+  InGroup<LifetimeAnalysis>, DefaultIgnore;
+def warn_dump_lifetime_contracts : Warning<"%0">, InGroup<LifetimeDumpContracts>, DefaultIgnore;
+
----------------
xazax.hun wrote:
> gribozavr2 wrote:
> > I think a warning that is a debugging aid is new territory.
> Do you think a cc1 flag would be sufficient or do you have other ideas/preferences?
Yes, I think that would be quite standard (similar to dumping the AST, dumping the CFG etc.)


================
Comment at: clang/lib/AST/LifetimeAttr.cpp:55
+    else if (Name == "Static")
+      return LifetimeContractSet{ContractVariable::staticVal()};
+    else if (Name == "Invalid")
----------------
xazax.hun wrote:
> gribozavr2 wrote:
> > The lifetime paper says it is called "global" now:
> > 
> > > Renamed static to global to reduce confusion with the keyword.
> > 
> > Also, the paper spells these special names in all lowercase letters.
> In this case the implementation was out of date, sorry for that. Now all the names should reflect the paper except for the `Return`.  The function name to express return values did not work out well due to how name lookup works (and overloads). Herb already updated his working draft to reflect this change. I will ask him if he is willing to publish the working draft somewhere. 
> 
> Also, he argued that `Return` is capitalized because it has a slightly different meaning compared to `null`, `global`, and `invalid`. In the lifetime annotations, `Return` will be on the 'left hand side', while the others will be on the 'right hand side'. 
I don't think the left-hand-side/right-hand-side difference helps me as a reader much. However, we would have a huge implementation problem if `return` was not capitalized (because it would be a keyword, and our expression parser really would not like that). So... whatever works, as long as we are consistent with the spec.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4532
+  if (ErrorRange.isValid())
+    S.Diag(ErrorRange.getBegin(), diag::warn_unsupported_expression)
+        << ErrorRange;
----------------
xazax.hun wrote:
> gribozavr2 wrote:
> > I'm assuming higher quality error messages are going to come later? Right now it is a catch-all message that is shown for unsupported expressions, expressions that don't type check, and expressions that will never be supported.
> Yes, I would love to improve the error message in the future. Note that, the current message is not that bad as it will highlight the unsupported expression. Expressions that does not type check will produce a warning earlier while parsing the argument of the attribute. Those warnings would be identical to warnings from expressions within the functions.
> 
> As for invalid lifetime annotations that are valid C++ expressions, ideally we want to make this impossible. (This patch does not push this direction yet.) Basically, we would like to make a DSL where every expression that type-checks using C++ type checking rules is a valid lifetime annotation. 
> Expressions that does not type check will produce a warning earlier while parsing the argument of the attribute.

I'd love some basic tests to show that.


================
Comment at: clang/test/Sema/attr-psets-annotation.cpp:24
+struct PointerTraits {
+  static auto deref(const T &t) -> decltype(*t);
+};
----------------
xazax.hun wrote:
> gribozavr2 wrote:
> > If this code imitates an existing library, please ignore.
> > 
> > Declaring PointerTraits::deref to be a function and then invoking it and using decltype on the return value seems unnecessarily complex.
> > 
> > You could declare a type alias within PointerTraits instead.
> > 
> > ```
> > template<typename T>
> > struct PointerTraits {
> >   using DerefType = decltype(*declvar<T>());
> > };
> > 
> > template<typename T>
> > struct PointerTraits<T*> {
> >   using DerefType = T;
> > }; // Is this specialization needed? Wouldn't the primary template work?
> > 
> > template<typename T>
> > struct PointerTraits<T&> {
> >   using DerefType = T;
> > };
> > ```
> > 
> > Also, a specialization for rvalue references?
> > 
> > However, I'm not sure if the specialization even for regular references will be used -- the free function deref() strips the reference from T (also strips constness from it!) Due to reference collapse rules, T will never be a reference.
> I agree with you. 
> 
> > If this code imitates an existing library, please ignore.
> 
> Yes and no. This imitates an experiment (not a well established library). In that experiment I was looking into generating some runtime checks from static lifetime annotations. It turns out most of the time it is not possible to generate appropriate runtime checks from these annotations except for some corner cases like nullability. So the reason why I had this as a function because it carried out some null checks.
> 
> Since null pointers are easy to catch dynamically using other methods I do not have strong feelings about the current API. Do you think it is worth to keep it as a function or do you prefer the simpler way?
> 
> > However, I'm not sure if the specialization even for regular references will be used...
> 
> That is correct, this is a leftover from the implementation which was using a trick to trigger these specializations like:
> 
> ```
> #define deref(arg) deref<decltype(arg)>(arg)
> ```
> 
Yes, I'd prefer a simpler implementation.


================
Comment at: clang/test/Sema/attr-psets-annotation.cpp:95
+  void f(X **out)
+      [[gsl::post(lifetime(deref(out), {this}))]];
+  // expected-warning at -2 {{Pre { }  Post { *out -> { this }; }}}
----------------
xazax.hun wrote:
> gribozavr2 wrote:
> > I'm not sure what causes this unqualified call to `deref` to call `gsl::deref` (in a different namespace). Does not seem like ADL would do it, since the type of `out` is also not in `gsl`.
> I have a using namespace above to keep tests more concise.
I'd strongly suggest to un-use that namespace to make tests more realistic. Or even add basic tests for both cases, and keep the bulk of the tests close to what you expect users to write. There will be some differences in the AST between the two cases, I believe.


================
Comment at: clang/test/Sema/attr-psets-annotation.cpp:35
+struct PSet {
+  PSet(...);
+};
----------------
Using a C variadic here gives me pause. I can't definitely say it would be a problem, however, C variadics are weird, can only pass certain types correctly, require passing them by value etc. So there might be some issues down the line, for example, with static analysis that warns about passing non-C-compatible types through a variadic, or with noncopyable types failing to typecheck in the call to PSet's constructor.

I think it would be better to do a variadic template that takes everything by const reference.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72810/new/

https://reviews.llvm.org/D72810