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

Sat Jan 25 12:14:48 PST 2020

aaron.ballman added a comment.

Thank you for the work on this!

================
Comment at: clang/include/clang/AST/LifetimeAttr.h:24
+/// 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
----------------
constrains lifetime -> constrains the lifetime

================
Comment at: clang/include/clang/AST/LifetimeAttr.h:25
+/// contract. A lifetime contract constrains lifetime of a
+/// LifetimeContractVariable to be at least as big as the lifetime of other
+/// LifetimeContractVariables.
----------------
big -> long

================
Comment at: clang/include/clang/AST/LifetimeAttr.h:29
+/// 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.
----------------
What is a drilldown expression? That's not a term I'm familiar with.

================
Comment at: clang/include/clang/AST/LifetimeAttr.h:70
+  bool operator<(const LifetimeContractVariable &O) const {
+    if (Tag != O.Tag)
+      return Tag < O.Tag;
----------------
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.

================
Comment at: clang/include/clang/AST/LifetimeAttr.h:79
+      if (RD != O.RD)
+        return std::less<const RecordDecl *>()(RD, O.RD);
+
----------------
This will have non deterministic behavior between program executions -- are you sure that's what we want? Similar below for fields.

================
Comment at: clang/include/clang/AST/LifetimeAttr.h:153-154
+
+  LifetimeContractVariable(TagType T) : Tag(T) {}
+  LifetimeContractVariable(const RecordDecl *RD) : RD(RD), Tag(This) {}
+  LifetimeContractVariable(const ParmVarDecl *PVD, int Deref)
----------------
These constructors should probably be `explicit`?

================
Comment at: clang/include/clang/AST/LifetimeAttr.h:163
+// Maps each annotated entity to a lifetime set.
+using LifetimeContracts = std::map<LifetimeContractVariable, LifetimeSet>;
+
----------------
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?

================
Comment at: clang/include/clang/Basic/Attr.td:2902

+def LifetimeContract : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "pre">, CXX11<"gsl", "post">];
----------------
I think the attribute should probably only be supported in C++ for now, and should have `let LangOpts = [CPlusPlus];` in the declaration. WDYT?

================
Comment at: clang/include/clang/Basic/Attr.td:2941
+  }];
+  let Documentation = [Undocumented]; // FIXME
+}
----------------
I agree with the FIXME comment. ;-)

================
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:
> > 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.

================
Comment at: clang/lib/AST/LifetimeAttr.cpp:23
+      const auto *Ctor = CE->getConstructor();
+      if (Ctor->getParent()->getName() == "PSet")
+        return CE;
----------------
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?

================
Comment at: clang/lib/AST/LifetimeAttr.cpp:123
+      if (ULE->getName().isIdentifier() &&
+          ULE->getName().getAsIdentifierInfo()->getName() == "lifetime")
+        break;
----------------
Same question about identifier namespaces here as above.

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4516
+
+static void handleLifetimeContractAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  LifetimeContractAttr *LCAttr;
----------------
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.

================
Comment at: clang/lib/Sema/SemaType.cpp:7727

+    // Move function type attribute to the declarator.
+    case ParsedAttr::AT_LifetimeContract:
----------------
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.

================
Comment at: clang/test/Sema/attr-psets-annotation.cpp:1
+// NOT RUN: %clang_cc1 -fsyntax-only -Wlifetime -Wlifetime-dump-contracts -verify %s
+
----------------
I think this test should be in SemaCXX instead of Sema.

================
Comment at: clang/test/Sema/attr-psets-annotation.cpp:44
+void basic(int *a, int *b) [[gsl::pre(gsl::lifetime(b, {a}))]];
+// expected-warning at -1 {{Pre { b -> { a }; }}}
+
----------------
Please spell out the full diagnostic the first time it is used in the test file.

================
Comment at: clang/test/Sema/attr-psets-annotation.cpp:130
+// Warnings/errors
+
+void unsupported_contract(int *a, int *b) [[gsl::pre(gsl::lifetime(b, {a++}))]];
----------------
This is missing all of the tests which verify the sanity of the attribute (appertains to the correct subject, accepts the correct number/type of arguments, etc).

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

https://reviews.llvm.org/D72810