[PATCH] D77836: [Attribute] Fix noderef attribute false-negatives

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: clang/lib/Sema/SemaCast.cpp:2557-2558
           return;
+        CheckNoDeref(Self, SrcExpr.get()->getType(), ResultType,
+                     OpRange.getBegin());
       }
----------------
rsmith wrote:
> Should we be checking this for a C-style cast?  You previously said  you were going to turn off the checks for C-style casts. Did you change your mind on that?
No, you're right. I accidentally left this in.


================
Comment at: clang/lib/Sema/SemaCast.cpp:2982
 
+  CheckNoDeref(*this, Op.SrcExpr.get()->getType(), Op.ResultType, LPLoc);
+
----------------
rsmith wrote:
> As above, should we be checking this for a C-style cast? (It also looks like we're doing this check twice in C++.)
Also accidentally left this in.


================
Comment at: clang/lib/Sema/SemaInit.cpp:8182-8184
+            // Do not check static casts here because they are checked earlier
+            // in Sema::ActOnCXXNamedCast()
+            if (!Kind.isStaticCast()) {
----------------
rsmith wrote:
> leonardchan wrote:
> > leonardchan wrote:
> > > rsmith wrote:
> > > > Are there any `static_cast` cases that don't get here? I'm also a little surprised that `static_cast` would get here but the `static_cast` interpretation of a C-style or functional cast would not.
> > > I'll double check on this. The reason I added this here was because the cast in `static_cast_from_same_ptr_type()` triggered 2 of the `warn_noderef_to_dereferenceable_pointer` warnings and one of them was triggered here.
> > Yeah it seems the `int *a = static_cast<int *>(x);` case in `cast_from_void_ptr()` is an example of a static_cast that doesn't go through here.
> I see, this is only reached for the part of `static_cast` that considers implicit conversion sequences. OK.
> 
> I expect there's similar overlap for all the other kinds of cast too. Should we be suppressing this check for *all* casts, given that we perform the check in `SemaCast` instead?
Hmm. I think for this particular location, `static_cast` contexts are all we need to suppress it for to prevent duplicate warnings from showing.

We don't need to suppress for these since the attribute is allowed to pass through these:
```
    IC_CStyleCast
    IC_FunctionalCast
```

We perform the check for implicit casts here since (AFAICT) only explicit casts (C-style, functional, and C++ style) seem to be covered in `SemaCast`:
```
    IC_Implicit
```
This would cover cases like `int *ptr = noderef_ptr;`.

I'm not sure about these ones, but none of the cases I have seem to fail if I suppress only them:
```
    IC_Normal
    IC_ExplicitConvs
```

It could be though that there's a particular case I'm not covering in my tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77836