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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
