This is an archive of the discontinued LLVM Phabricator instance.

[Attribute] Fix noderef attribute false-negatives
ClosedPublic

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
275

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
8182–8189

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
275

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
8182–8189

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
8182–8189

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
734

Please use UpperCamelCase for local variables.

2557–2558

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?

2980

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
8182–8189

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
2557–2558

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

2980

Also accidentally left this in.

clang/lib/Sema/SemaInit.cpp
8182–8189

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.May 30 2020, 10:43 AM

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

clang/lib/Sema/SemaInit.cpp
8182

The comment should end with a period.

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

Lemme know if there's any more feedback. Will aim for committing this sometime at the end of the day.

This revision was automatically updated to reflect the committed changes.