Page MenuHomePhabricator

[Attribute] Fix noderef attribute false-negatives
AcceptedPublic

Authored by leonardchan on Apr 9 2020, 3:32 PM.

Details

Summary

noderef was failing to trigger warnings in some cases related to c++ style casting. This patch addresses them.

Diff Detail

Event Timeline

leonardchan created this revision.Apr 9 2020, 3:32 PM
rsmith added inline comments.Apr 11 2020, 10:25 PM
clang/lib/Sema/SemaCast.cpp
280

Warning on this seems reasonable for static_cast and dynamic_cast, but should reinterpret_cast (or the reinterpret_cast interpretation of a C-style cast) warn? Presumably we want to leave open some type system holes for the case where the programmer really does know what they're doing.

clang/lib/Sema/SemaInit.cpp
8198–8205

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.

aaron.ballman added inline comments.Apr 12 2020, 11:15 AM
clang/test/Frontend/noderef.c
211 ↗(On Diff #256428)

The name of the function is a bit odd given that there's an explicit cast.

leonardchan marked 3 inline comments as done.Apr 13 2020, 1:34 PM
leonardchan added inline comments.
clang/lib/Sema/SemaCast.cpp
280

My initial idea for this was that users could explicitly disable the warning with -Wno-noderef or pragmas for line-level granularity, but I can see how this could be less convenient/not consistent with other casts. Will update to allow C-style + reinterpret_casts.

clang/lib/Sema/SemaInit.cpp
8198–8205

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.

clang/test/Frontend/noderef.c
211 ↗(On Diff #256428)

Woops. Will update the name.

leonardchan marked an inline comment as done.

Updated to address comments and added an RAII class for easier checking before exiting functions.

clang/lib/Sema/SemaInit.cpp
8198–8205

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.

rsmith added inline comments.Apr 24 2020, 3:04 PM
clang/lib/Sema/SemaCast.cpp
750

Please use UpperCamelCase for local variables.

2590–2591

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?

3013

As above, should we be checking this for a C-style cast? (It also looks like we're doing this check twice in C++.)

clang/lib/Sema/SemaInit.cpp
8198–8205

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?

leonardchan marked 6 inline comments as done.
leonardchan added inline comments.
clang/lib/Sema/SemaCast.cpp
2590–2591

No, you're right. I accidentally left this in.

3013

Also accidentally left this in.

clang/lib/Sema/SemaInit.cpp
8198–8205

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.

aaron.ballman accepted this revision.Sat, May 30, 10:43 AM

LGTM, but please wait for a few days in case @rsmith has further concerns.

clang/lib/Sema/SemaInit.cpp
8198–8204

The comment should end with a period.

This revision is now accepted and ready to land.Sat, May 30, 10:43 AM
leonardchan marked an inline comment as done.Mon, Jun 1, 11:48 AM