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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 6 inline comments as done.
xazax.hun added inline comments.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:143
+using LifetimeContractSet = std::set<ContractVariable>;
+using LifetimeContractMap = std::map<ContractVariable, LifetimeContractSet>;
+
----------------
gribozavr2 wrote:
> These names are not descriptive...
I changed them slightly and added some comments. Let me know if they are better or if you have some alternatives.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:146
+namespace process_lifetime_contracts {
+// This function and the callees are have the sole purpose of matching the
+// AST that describes the contracts. We are only interested in identifier names
----------------
gribozavr2 wrote:
> I'm sorry, but this comment is missing the most important piece of information -- what this function does.
> 
> This comment says "matching the AST that describes the contracts", which does give me a feeling of the compiler component, but I have no idea about how exactly it uses the expression that is passed in, and what happens with the map, and why a SourceRange is returned.
You are completely right. This comment was mainly an implementation related one. Now I have a more user facing comment in the header and moved the implementation related part to the cpp file.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3342
+  InGroup<LifetimeAnalysis>, DefaultIgnore;
+def warn_dump_lifetime_contracts : Warning<"%0">, InGroup<LifetimeDumpContracts>, DefaultIgnore;
+
----------------
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?


================
Comment at: clang/lib/AST/LifetimeAttr.cpp:55
+    else if (Name == "Static")
+      return LifetimeContractSet{ContractVariable::staticVal()};
+    else if (Name == "Invalid")
----------------
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'. 


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4532
+  if (ErrorRange.isValid())
+    S.Diag(ErrorRange.getBegin(), diag::warn_unsupported_expression)
+        << ErrorRange;
----------------
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. 


================
Comment at: clang/test/Sema/attr-psets-annotation.cpp:24
+struct PointerTraits {
+  static auto deref(const T &t) -> decltype(*t);
+};
----------------
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)
```



================
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 }; }}}
----------------
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.


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

https://reviews.llvm.org/D72810