[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/Parse/ParseExpr.cpp:1126
+
+    Actions.StartCheckingNoDeref();
+
----------------
leonardchan wrote:
> rsmith wrote:
> > This parser-driven start/stop mechanism will not work in C++ templates. Instead, can you remove the "start" part and check the noderef exprs as part of popping the ExprEvaluationContextRecord?
> I'm not sure if I should remove the the start and stop methods because for a regular C program, the Push/PopExprEvaluationContextRecord functions don't seem to be called, and even when they are called in C++, the initial record that exists on the stack isn't popped at all.
> 
> Since pending noderef expressions are still parsed and added to the last record during template instantiation, doing another check when popping covers all noderef exprs added during instantiation.
`PushExpressionEvaluationContext` / `PopExpressionEvaluationContext` are called regardless of which language we're parsing. If we're missing `ExpressionEvaluationContext` records around some expression parsing, we should fix that. We should never be creating expressions within the initial `ExpressionEvaluationContext` record (except perhaps during error recovery).

> Since pending noderef expressions are still parsed and added to the last record during template instantiation, doing another check when popping covers all noderef exprs added during instantiation.

That's not how template instantiation works in Clang. We don't re-parse, we perform a recursive tree transformation that does not involve the parser.


================
Comment at: lib/Sema/SemaExpr.cpp:14230-14242
+class DeclRefFinder : public ConstEvaluatedExprVisitor<DeclRefFinder> {
+public:
+  typedef ConstEvaluatedExprVisitor<DeclRefFinder> Inherited;
+
+  DeclRefFinder(ASTContext &Ctx) : Inherited(Ctx) {}
+
+  void VisitDeclRefExpr(const DeclRefExpr *E) { DeclRef = E; }
----------------
leonardchan wrote:
> rsmith wrote:
> > rsmith wrote:
> > > I don't see any reason to assume that the `DeclRefExpr` found by this visitor will be the one you're interested in (the one the `noderef` attribute came from).
> > > 
> > > How about instead you walk over only expressions that you *know* preserve `noderef`, and if you can't find a declaration as the source of the `noderef` attribute, produce a differently-worded diagnostic that doesn't give a variable name?
> > This comment is marked done but has not been addressed.
> My bad. My response to this was adding a check in `StopCheckingNoDerefAndWarn` where we check if the `DeclRefExpr` is `nullptr` and throw the appropriate error if it is found or not.
> 
> Although I can't come up with an example where this doesn't get the declref we want, since the expressions we pass to this visitor are all our marked noderef expressions (which we check via the attribute on the type). So even if we have a complex expression that has multiple declrefs, all noderef declrefs should be checked by this.
How about this:

```
int __attribute__((noderef)) *a;
int __attribute__((noderef)) *b;
int __attribute__((noderef)) *c;
*(a = b, c);
```

The first noderef pointer the visitor finds will be `a`, but that's not the one that was dereferenced.


================
Comment at: lib/Sema/SemaExpr.cpp:14251-14253
+    if (const auto *Ptr = dyn_cast<PointerType>(Ty))
+      Inner = Ptr->getPointeeType();
+    else if (const auto *Arr = dyn_cast<ArrayType>(Ty))
----------------
aaron.ballman wrote:
> I don't think this strips off sugar from the type, so I suspect it won't properly handle a typedef to a pointer type, for instance, or a type including parens. You should add tests for these cases.
Calling `getDesugaredType` is not the right way to address this: use `Ty->getAs<PointerType>()` and `Context.getAsArrayType(Ty)` instead.


================
Comment at: lib/Sema/SemaExpr.cpp:4164-4177
+bool Sema::TypeHasNoDeref(QualType Ty) {
+  // Strip off everything but attributes.
+  QualType OldTy;
+  while (Ty != OldTy && !isa<AttributedType>(Ty)) {
+    OldTy = Ty;
+    Ty = Ty.getSingleStepDesugaredType(Context);
+  }
----------------
You can just use `Ty->hasAttr(attr::NoDeref)` for this now.


================
Comment at: lib/Sema/SemaExpr.cpp:4263-4266
+  auto FoundExpr = LastRecord.PossibleDerefs.find(StrippedExpr);
+  if (FoundExpr != LastRecord.PossibleDerefs.end()) {
+    LastRecord.PossibleDerefs.erase(*FoundExpr);
+  }
----------------
Do you need to do this both here and after walking into the `.` expressions? It seems to me that it would be sufficient to first walk into the LHS of any `.` expressions, and then remove the result from `PossibleDerefs`.


================
Comment at: lib/Sema/SemaExpr.cpp:4269
+  const Expr *Base = nullptr;
+  while (const auto *Member = dyn_cast<MemberExpr>(StrippedExpr)) {
+    Base = Member->getBase()->IgnoreParenImpCasts();
----------------
You should check the `MemberExpr` is a `.` rather than a `->` before stepping into it.


================
Comment at: lib/Sema/SemaExpr.cpp:4278
+    auto FoundBase = LastRecord.PossibleDerefs.find(Base);
+    if (TypeHasNoDeref(BaseTy) &&
+        FoundBase != LastRecord.PossibleDerefs.end()) {
----------------
This type check seems incorrect, because you don't propagate the type through `->` operators. In particular, given

   `&noderef_ptr->x.y`

you will step back to `noderef_ptr->x`, which will not have a noderef type, and not remove it from the set as a result. It seems best to drop this type check.


================
Comment at: lib/Sema/SemaExpr.cpp:4289
+
+  if (TypeHasNoDeref(ResultTy)) {
+    LastRecord.PossibleDerefs.insert(E);
----------------
Do you ensure that the `noderef` attribute will be on the innermost level of the array type? I think you'll need to do so in order to warn on:

```
typedef int A[32];
typedef A __attribute__((noderef)) *B;
int f(B b) { return (*B)[1]; }
```

(Here, we have a pointer to a noderef annotated array of non-noderef-annotated int. So I think we will not emit a warning from the first dereference, because we have a pointer to an array, and we will not emit a warning from the second dereference in the array indexing, because the result type does not have the noderef attribute.)


================
Comment at: lib/Sema/SemaExpr.cpp:4297
+  const Expr *Base = E->getBase();
+  const QualType &BaseTy = Base->getType();
+  if (!(isa<ArrayType>(BaseTy) || isa<PointerType>(BaseTy)))
----------------
`QualType`s should be held by value, not by reference.


================
Comment at: lib/Sema/SemaExpr.cpp:4302-4308
+  if (const auto *Member = dyn_cast<MemberExpr>(Base->IgnoreParenCasts())) {
+    const QualType &MemberBaseTy = Member->getBase()->getType();
+    if (const auto *Ptr = dyn_cast<PointerType>(MemberBaseTy)) {
+      if (TypeHasNoDeref(Ptr->getPointeeType()))
+        LastRecord.PossibleDerefs.insert(E);
+    }
+  }
----------------
There could be a sequence of `MemberExpr`s here, and watch out for `.` vs `->`.


================
Comment at: lib/Sema/SemaExpr.cpp:12865-12866
+
+  if (Opc == UO_Deref && TypeHasNoDeref(UO->getType()))
+    ExprEvalContexts.back().PossibleDerefs.insert(UO);
+
----------------
You should probably not add the expression to the list if it's of array type. Given:

```
typedef int A[32];
typedef A __attribute__((noderef)) *B;
B b;
int __attribute__((noderef)) *p = *b;
```

... you should not warn on the "dereference" here. That should either be handled here or by treating an array-to-pointer decay operation the same as an "address of" operation and removing pending derefs from the list.


Repository:
  rC Clang

https://reviews.llvm.org/D49511