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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: lib/Parse/ParseExpr.cpp:1126
+
+    Actions.StartCheckingNoDeref();
+
----------------
rsmith wrote:
> 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.
So when should a new `ExpressionEvaluationContext` be pushed or popped? 

For the following code:

```
#define NODEREF __attribute__((noderef))

void func(){
  int NODEREF *x;
  *x;
}

int main(){
  func();
}
```

A new context is pushed then popped in C++ but not for C. From what I can tell based off my observations and looking for where `Push/Pop` get called in code, new ones would get added when we enter a new GNU style statement expression, captured region after a pragma, or different error blocks.


================
Comment at: lib/Sema/SemaExpr.cpp:4289
+
+  if (TypeHasNoDeref(ResultTy)) {
+    LastRecord.PossibleDerefs.insert(E);
----------------
rsmith wrote:
> 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.)
Hmmmm, so normally in order to check what's correct, I usually run these examples through `sparse` since that's the tool that actually checks `noderef` for gcc, but it seems that sparse instead diagnoses a warning on the array indexing instead and nothing for the first dereference.

This shouldn't be though since, as you pointed out, the array does not have `noderef` types. For a simple array,

```
int __attribute__((noderef)) x[10];
x[0];
```

`sparse` diagnoses the appropriate warning for the array index. Personally though, I would chalk this up to an edge case that wasn't thought of before in sparse, since something like this isn't handled on their existing validation tests.

Currently, this diagnoses a warning on the first dereference, but I can also understand why it shouldn't warn because accessing `noderef` structs shouldn't warn if the member accessed is an array. The only case though this applies in sparse's tests are with structs and they don't provide a test for dereferencing a pointer to an array.

I think the reason for why the struct example still passes is that if the member is an array, none of the actual data in the struct is being accessed. Instead, the value returned is just a pointer to the start of the array within the struct and equivalent to just adding an offset to the struct pointer. I think that no warning should also be thrown for this example if it is guaranteed that array `A` can similarly be found from simple pointer arithmetic on pointer `B`. I don't think it can unless B were an array or struct also, but I could be wrong or overthinking this.


Repository:
  rC Clang

https://reviews.llvm.org/D49511