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

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

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


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:163
+// Maps each annotated entity to a lifetime set.
+using LifetimeContracts = std::map<LifetimeContractVariable, LifetimeSet>;
+
----------------
aaron.ballman wrote:
> xazax.hun wrote:
> > gribozavr2 wrote:
> > > Generally, DenseMap and DenseSet are faster. Any reason to not use them instead?
> > No specific reason other than I am always insecure about the choice. Dense data structures are great for small objects and I do not know where the break even point is and never really did any benchmarks. Is there some guidelines somewhere for what object size should we prefer one over the other?
> I usually figure that if it's less than two pointers in size, it's sufficiently small, but maybe others have a different opinion. This class is probably a bit large for using dense containers.
> 
> I do worry a little bit about the hoops we're jumping through to make the class orderable -- is there a reason to avoid an unordered container instead?
I am not insisting on using ordered containers but note that since I have no idea how to get a deterministic and efficient hash value of a `RecordDecl` I would likely just include the address of the canonical decl. So the order of the elements in the container would be non-deterministic both ways. But the algorithm (other than the debug dumps which are fixable) will not depend on the order of the elements.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:70
+  bool operator<(const LifetimeContractVariable &O) const {
+    if (Tag != O.Tag)
+      return Tag < O.Tag;
----------------
aaron.ballman wrote:
> I think it would be easier to read if the pattern was: `if (Foo < Bar) return true;` as opposed to checking inequality before returning the comparison.
I think this might not be equivalent. 

The code below will always early return if the `Tag` is not the same.
```
if (Tag != O.Tag)
  return Tag < O.Tag;
```

The code below will only early return if the condition is true.

```
if (Tag < O.Tag)
  return true;
```

So I think the pattern above is an optimization. 



================
Comment at: clang/include/clang/AST/LifetimeAttr.h:79
+      if (RD != O.RD)
+        return std::less<const RecordDecl *>()(RD, O.RD);
+
----------------
aaron.ballman wrote:
> This will have non deterministic behavior between program executions -- are you sure that's what we want? Similar below for fields.
Good point. As far as the analysis is concerned the end result should always be the same. I see potential problems in the tests where the contents of some ordered data structures using this ordering is dumped. I do not see any natural (and preformant) way to order RecordDecls. (I can order FieldDecls using their indices.) Are you ok with sorting the string representation before outputting them as strings? This should solve the potential non-deterministic behavior of tests. 


================
Comment at: clang/include/clang/Basic/Attr.td:2902
 
+def LifetimeContract : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "pre">, CXX11<"gsl", "post">];
----------------
aaron.ballman wrote:
> I think the attribute should probably only be supported in C++ for now, and should have `let LangOpts = [CPlusPlus];` in the declaration. WDYT?
Makes sense, thanks!


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3342
+  InGroup<LifetimeAnalysis>, DefaultIgnore;
+def warn_dump_lifetime_contracts : Warning<"%0">, InGroup<LifetimeDumpContracts>, DefaultIgnore;
+
----------------
aaron.ballman wrote:
> xazax.hun wrote:
> > gribozavr2 wrote:
> > > 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.)
> > I started to look into a cc1 flag, but it looks like it needs a bit more plumbing I expected as some does not have access to the CompilerInvocation object or the FrontendOptions. While it is not too hard to pass an additional boolean to Sema I also started to think about the user/developer experience (ok, regular users are not expected to do debugging). The advantage of warnings are that we get a source location for free, so it is really easy to see where the message is originated from. And while it is true that I cannot think of any examples of using warnings for debugging the Clang Static Analyzer is full of checks that are only dumping debug data as warnings.
> I also prefer a cc1 flag -- using warnings to control debugging behavior is novel and doesn't seem like something we should encourage. I don't think you would need to pass any additional flags to Sema however; I would expect this to show up in a language option so it can be queried through `LangOpts`. This makes it available to other things, like clang-tidy checks as well.
Yeah, plumbing through `LangOpts` will definitely work. The main reason I was reluctant to do so because I do not see anything in the language options that serves the purpose of debugging and I was wondering it this would really belong there.


================
Comment at: clang/lib/AST/LifetimeAttr.cpp:23
+      const auto *Ctor = CE->getConstructor();
+      if (Ctor->getParent()->getName() == "PSet")
+        return CE;
----------------
aaron.ballman wrote:
> This need some explanation as it sort of comes out of thin air. What is special about a class named `PSet`? Does it need to be in the global namespace, or a class named `PSet` in any namespace is special?
I agree, I will make this more strict. Basically, there is a need for a small support library for these contracts to work. That support library is not defined anywhere yet, but it will be expected to live in the `gsl`namespace. 


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4516
+
+static void handleLifetimeContractAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  LifetimeContractAttr *LCAttr;
----------------
aaron.ballman wrote:
> I suspect you want this to handle redeclarations where you may be working with an inherited attribute? If so, you probably should be following the `merge` pattern used by other attributes.
It is not completely the same. I did not give much testing for redeclarations yet. The main purpuse of this code is to handle the following use case:

```
void multiple_annotations(int *a, int *b, int *c)
    [[gsl::pre(gsl::lifetime(b, {a}))]]
    [[gsl::pre(gsl::lifetime(c, {a}))]];
```

So the same declaration might have multiple instances of the annotation (similar to contracts) and we need to merge them. 


================
Comment at: clang/lib/Sema/SemaType.cpp:7727
 
+    // Move function type attribute to the declarator.
+    case ParsedAttr::AT_LifetimeContract:
----------------
aaron.ballman wrote:
> This is not an okay thing to do for C++ attributes. They have a specific meaning as to what they appertain to and do not move around to better subjects like GNU-style attributes.
Yeah, I know that. But this is problematic in the standard. See the contracts proposal which also kind of violates this rule by adding an exception. Basically, this is the poor man's version of emulating the contracts proposal.  In the long term we plan to piggyback on contracts rather than hacking the C++ attributes.


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

https://reviews.llvm.org/D72810