[PATCH]: Implementation of -Wcast-qual
David Blaikie
dblaikie at gmail.com
Wed Nov 19 10:05:37 PST 2014
On Tue, Nov 18, 2014 at 2:05 PM, Roman Divacky <rdivacky at vlakno.cz> wrote:
> New patch attached. I've checked the behaviour against gcc5 and we warn on
> the
> same cases.
>
Great! Thanks so much for your patience & diligence. Does GCC mention the
same types as Clang? (I notice in some of your examples... oh, you don't
have such an example. What does GCC and Clang do for "const char **" ->
"char **"? It looks like, judging by the "int**" -> "const int **" warning,
we might print just the inner type there as well ("cast from const char* to
char*") - is that what GCC does? Or does it print the full outer types
always? Is your patch's behavior going to be better for users (probably,
but I'm not sure).
> For the const int **bahc = (const int **)bah; case gcc5 emits a warning
> with
> a different warning (to be safe all intermediate pointers in cast from
> 'int **'
> to 'volatile int **' must be 'const' qualified [-Wcast-qual]). Do you think
> it's worth adding similarly worded warning for that case or is the
> "dropping
> const" ok?
>
I think it's OK, but confusing - it'd be good to make it better, but
possibly as a follow-up patch. Could you include FIXMEs in the test case
for both the const and volatile cases for this showing what GCC does (or
otherwise suggesting better wording). I'm assuming in even the volatile
case, the correct (and GCC5) text is that you need to add const on the
intermediate pointers?
Actually, now that I look at the test case, it seems a bit worse - it's not
printing the top-level type in this case (int**->const int**) it's printing
the nested type (int*->const int*) and saying it drops const, which is even
more confusing... hrm. So maybe it makes sense to fix this in the current
patch rather than a follow-up, given how confusing it is.
One thing to do with this current patch, would be to use a %select rather
than %2 and %3 being directly substituted (having to add that 's' is a bit
weird). Something like this:
Warning<"cast from %0 to %1 drops %select{const and volatile
qualifiers|const qualifier|volatile qualifier}2">
int qualifiers = 0;
if (const && volatile)
qualifiers = 0;
else if (const)
qualifiers = 1;
else if (volatile)
qualifiers = 2;
etc.
& doing the better "all intermediate pointers in cast" thing shouldn't be
too hard, right? Just another warning in the .td file, plus a condition
before all that to say "if !const && !volatile <do the other warning>" but
you'll need to get those types right (providing the top level types, not
the nested types, especially in that instance).
>
> On Tue, Nov 18, 2014 at 09:27:17AM -0800, David Blaikie wrote:
> > The last line should have a warning on it (and assuming the
> > expected-warning comments in the test are correct, it doesn't):
> >
> > GCC 4.9:
> >
> > warning: cast from type 'int**' to type 'const int**' casts away
> qualifiers
> > [-Wcast-qual]
> > const int **bahc = (const int **)bah;
> >
> > On Tue, Nov 18, 2014 at 6:11 AM, Roman Divacky <rdivacky at vlakno.cz>
> wrote:
> >
> > > Scrap the previous patch. It doesnt work when we add qualifiers. The
> new
> > > version is fixed.
> > >
> > > On Tue, Nov 18, 2014 at 12:49:59PM +0100, Roman Divacky wrote:
> > > > Perhaps like this?
> > > >
> > > > On Fri, Nov 14, 2014 at 01:17:38PM -0800, David Blaikie wrote:
> > > > > Yep. Though the diagnostic for dropping const and volatile is a
> bit off
> > > > > ("drops const volatile qualifier" should probably read "drops
> const and
> > > > > volatile qualifiers"? (maybe you'll need a %select for this - you
> could
> > > > > probably use a %select for the const and volatile separately too
> if you
> > > > > like)). What does GCC do here? Does it warn on dropping volatile at
> > > all?
> > > > >
> > > > > Chatting to Richard over lunch he mentioned an interesting case
> where
> > > we
> > > > > might want to warn:
> > > > >
> > > > > int **x;
> > > > > auto y = (const int **)x;
> > > > >
> > > > > which, if it were a static_cast, would warn:
> > > > >
> > > > > cast.cpp:2:10: error: static_cast from 'int **' to 'const int
> **' is
> > > not
> > > > > allowed
> > > > > auto y = static_cast<const int **>(x);
> > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > >
> > > > > I'm just not sure we'll be able to get a good diagnostic in both
> these
> > > > > cases. But as I think/type this out I think:
> > > > >
> > > > > We should just use the same machinery that powers static_cast here,
> > > > > (Richard mentioned there should be a way to test whether a
> conversion
> > > is a
> > > > > "qualification conversion" which is the description of the valid
> > > implicit
> > > > > qualification changes, and not the above const-changing cases))
> and we
> > > > > should teach the machinery to give us enough information to create
> good
> > > > > diagnostics - telling the user where the missing const, volatile,
> etc,
> > > is.
> > > > >
> > > > > Sorry to go through so many iterations - it didn't occur to me
> until
> > > > > Richard mentioned it that there was this more general approach.
> > > > >
> > > > > (wonder what GCC does here? - hmm, looks like it gets the "int** ->
> > > const
> > > > > int**" right: cast from type ???int**??? to type ???const int**???
> > > casts away
> > > > > qualifiers)
> > > > >
> > > > > On Fri, Nov 14, 2014 at 12:02 PM, Roman Divacky <
> rdivacky at vlakno.cz>
> > > wrote:
> > > > >
> > > > > > Like this?
> > > > > >
> > > > > > On Fri, Nov 14, 2014 at 11:20:49AM -0800, David Blaikie wrote:
> > > > > > > I take it this is consistent with the GCC warning - in terms of
> > > warning
> > > > > > on
> > > > > > > the innermost issue, reporting const or volatile - what about
> > > dropping
> > > > > > > const and volatile at the same time?
> > > > > > >
> > > > > > > Issues with the current code:
> > > > > > >
> > > > > > > * DestPtr and SrcPtr don't need to be initialized to null,
> they'll
> > > be
> > > > > > > written to on the first loop iteration as needed - avoiding
> excess
> > > > > > > initialization helps tools like MSan find more bugs rather
> than the
> > > > > > program
> > > > > > > silently using unintended default values
> > > > > > >
> > > > > > > * InnerMostDestType and InnerMostSrcType will be dangling
> pointers
> > > after
> > > > > > > the while loop (so accessing them in the proceeding 'if' is UB)
> > > > > > >
> > > > > > > * you don't need to check both InnerMostDestType and
> > > InnerMostSrcType in
> > > > > > > the following if - it's invariant that if one is non-null (you
> can
> > > use
> > > > > > > QualType values rather than QualType* to address the previous
> bug,
> > > and
> > > > > > use
> > > > > > > QualTypes "isNull()" member function here) so is the other
> > > > > > >
> > > > > > > On Fri, Nov 14, 2014 at 11:07 AM, Roman Divacky <
> > > rdivacky at vlakno.cz>
> > > > > > wrote:
> > > > > > >
> > > > > > > > Actually, try this patch. It includes check for volatile as
> well.
> > > > > > > >
> > > > > > > > On Wed, Nov 12, 2014 at 12:39:20PM -0800, David Blaikie
> wrote:
> > > > > > > > > [+Richard for oversight]
> > > > > > > > >
> > > > > > > > > char **y1 = (char **)ptrptr; // expected-warning {{cast
> from
> > > 'const
> > > > > > char
> > > > > > > > > *const *' to 'char **' drops const qualifier}}
> expected-warning
> > > > > > {{cast
> > > > > > > > from
> > > > > > > > > 'const char *const' to 'char *' drops const qualifier}}
> > > > > > > > >
> > > > > > > > > I think if we're going to warn on multiple layers (I'm not
> sure
> > > > > > that's
> > > > > > > > > ideal - is that consistent with GCC's warning? Does GCC
> warn on
> > > > > > > > mismatched
> > > > > > > > > types too - "const T*" -> "U*"? - do we warn there too, or
> > > only when
> > > > > > > > > there's a valid implicit conversion like the void*
> example?)
> > > then we
> > > > > > > > should
> > > > > > > > > probably drop the top level const, "const char *const" ->
> > > "char*" -
> > > > > > the
> > > > > > > > top
> > > > > > > > > level const on the first type is confusing/misleading, it's
> > > only
> > > > > > relevant
> > > > > > > > > to show "const char*" and "char*".
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Wed, Nov 12, 2014 at 10:41 AM, Roman Divacky <
> > > rdivacky at vlakno.cz>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > I expanded the testcase and fixed the grammar in the
> actual
> > > > > > warning.
> > > > > > > > > >
> > > > > > > > > > New patch attached.
> > > > > > > > > >
> > > > > > > > > > On Tue, Nov 11, 2014 at 05:03:33PM -0800, David Blaikie
> > > wrote:
> > > > > > > > > > > (it's a bit easier if you include the test in the same
> > > patch
> > > > > > file -
> > > > > > > > also
> > > > > > > > > > > you can use Phabricator if you like - some reviewers
> > > perefer it)
> > > > > > > > > > >
> > > > > > > > > > > Since you've got the loop there for seeing through
> multiple
> > > > > > levels of
> > > > > > > > > > > pointer, should you have a test case that exercises
> that
> > > on a > 1
> > > > > > > > level
> > > > > > > > > > of
> > > > > > > > > > > depth? Demonstrate that we warn on both levels (if
> that's
> > > the
> > > > > > right
> > > > > > > > thing
> > > > > > > > > > > to do?)?
> > > > > > > > > > >
> > > > > > > > > > > Optionally (probably in a separate follow-up patch) you
> > > could
> > > > > > add a
> > > > > > > > note
> > > > > > > > > > > with a fixit to include the missing consts.
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Nov 11, 2014 at 10:58 AM, Roman Divacky <
> > > > > > rdivacky at vlakno.cz>
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Hi,
> > > > > > > > > > > >
> > > > > > > > > > > > I implemented -Wcast-qual. The patch is actually
> quite
> > > short
> > > > > > > > (attached
> > > > > > > > > > + a
> > > > > > > > > > > > test
> > > > > > > > > > > > case).
> > > > > > > > > > > >
> > > > > > > > > > > > This fixes #13772 and also note that -Wcast-qual is
> used
> > > in
> > > > > > llvm
> > > > > > > > build
> > > > > > > > > > > > itself.
> > > > > > > > > > > >
> > > > > > > > > > > > Is this ok to be commited? Thanks
> > > > > > > > > > > >
> > > > > > > > > > > > Roman
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > _______________________________________________
> > > > > > > > > > > > cfe-commits mailing list
> > > > > > > > > > > > cfe-commits at cs.uiuc.edu
> > > > > > > > > > > >
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > >
> > > > Index: include/clang/Basic/DiagnosticGroups.td
> > > > ===================================================================
> > > > --- include/clang/Basic/DiagnosticGroups.td (revision 222228)
> > > > +++ include/clang/Basic/DiagnosticGroups.td (working copy)
> > > > @@ -60,7 +60,7 @@
> > > > def KeywordCompat : DiagGroup<"keyword-compat">;
> > > > def GNUCaseRange : DiagGroup<"gnu-case-range">;
> > > > def CastAlign : DiagGroup<"cast-align">;
> > > > -def : DiagGroup<"cast-qual">;
> > > > +def CastQual : DiagGroup<"cast-qual">;
> > > > def : DiagGroup<"char-align">;
> > > > def Comment : DiagGroup<"comment">;
> > > > def GNUComplexInteger : DiagGroup<"gnu-complex-integer">;
> > > > Index: include/clang/Basic/DiagnosticSemaKinds.td
> > > > ===================================================================
> > > > --- include/clang/Basic/DiagnosticSemaKinds.td (revision
> 222228)
> > > > +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> > > > @@ -6104,6 +6104,8 @@
> > > > def warn_zero_size_struct_union_in_extern_c :
> Warning<"%select{|empty
> > > }0"
> > > > "%select{struct|union}1 has size 0 in C, %select{size 1|non-zero
> > > size}2 in C++">,
> > > > InGroup<ExternCCompat>;
> > > > +def warn_cast_qual : Warning<"cast from %0 to %1 drops %2
> qualifier%3">,
> > > > + InGroup<CastQual>, DefaultIgnore;
> > > > } // End of general sema category.
> > > >
> > > > // inline asm.
> > > > Index: lib/Sema/SemaCast.cpp
> > > > ===================================================================
> > > > --- lib/Sema/SemaCast.cpp (revision 222228)
> > > > +++ lib/Sema/SemaCast.cpp (working copy)
> > > > @@ -143,7 +143,10 @@
> > > > }
> > > >
> > > > static bool CastsAwayConstness(Sema &Self, QualType SrcType,
> QualType
> > > DestType,
> > > > - bool CheckCVR, bool
> CheckObjCLifetime);
> > > > + bool CheckCVR, bool
> CheckObjCLifetime,
> > > > + QualType *TheOffendingSrcType =
> nullptr,
> > > > + QualType *TheOffendingDestType =
> nullptr,
> > > > + Qualifiers *CastAwayQualifiers =
> > > nullptr);
> > > >
> > > > // The Try functions attempt a specific way of casting. If they
> > > succeed, they
> > > > // return TC_Success. If their way of casting is not appropriate for
> > > the given
> > > > @@ -462,7 +465,10 @@
> > > > /// \param CheckObjCLifetime Whether to check Objective-C lifetime
> > > qualifiers.
> > > > static bool
> > > > CastsAwayConstness(Sema &Self, QualType SrcType, QualType DestType,
> > > > - bool CheckCVR, bool CheckObjCLifetime) {
> > > > + bool CheckCVR, bool CheckObjCLifetime,
> > > > + QualType *TheOffendingSrcType,
> > > > + QualType *TheOffendingDestType,
> > > > + Qualifiers *CastAwayQualifiers) {
> > > > // If the only checking we care about is for Objective-C lifetime
> > > qualifiers,
> > > > // and we're not in ARC mode, there's nothing to check.
> > > > if (!CheckCVR && CheckObjCLifetime &&
> > > > @@ -487,6 +493,8 @@
> > > > // Find the qualifiers. We only care about cvr-qualifiers for the
> > > > // purpose of this check, because other qualifiers (address
> spaces,
> > > > // Objective-C GC, etc.) are part of the type's identity.
> > > > + QualType PrevUnwrappedSrcType = UnwrappedSrcType;
> > > > + QualType PrevUnwrappedDestType = UnwrappedDestType;
> > > > while (UnwrapDissimilarPointerTypes(UnwrappedSrcType,
> > > UnwrappedDestType)) {
> > > > // Determine the relevant qualifiers at this level.
> > > > Qualifiers SrcQuals, DestQuals;
> > > > @@ -497,6 +505,13 @@
> > > > if (CheckCVR) {
> > > >
> RetainedSrcQuals.setCVRQualifiers(SrcQuals.getCVRQualifiers());
> > > >
> RetainedDestQuals.setCVRQualifiers(DestQuals.getCVRQualifiers());
> > > > +
> > > > + if (RetainedSrcQuals != RetainedDestQuals &&
> TheOffendingSrcType
> > > &&
> > > > + TheOffendingDestType && CastAwayQualifiers) {
> > > > + *TheOffendingSrcType = PrevUnwrappedSrcType;
> > > > + *TheOffendingDestType = PrevUnwrappedDestType;
> > > > + *CastAwayQualifiers = RetainedSrcQuals - RetainedDestQuals;
> > > > + }
> > > > }
> > > >
> > > > if (CheckObjCLifetime &&
> > > > @@ -505,6 +520,9 @@
> > > >
> > > > cv1.push_back(RetainedSrcQuals);
> > > > cv2.push_back(RetainedDestQuals);
> > > > +
> > > > + PrevUnwrappedSrcType = UnwrappedSrcType;
> > > > + PrevUnwrappedDestType = UnwrappedDestType;
> > > > }
> > > > if (cv1.empty())
> > > > return false;
> > > > @@ -2371,6 +2389,28 @@
> > > >
> > > > if (Kind == CK_BitCast)
> > > > checkCastAlign();
> > > > +
> > > > + // -Wcast-qual
> > > > + QualType TheOffendingSrcType, TheOffendingDestType;
> > > > + Qualifiers CastAwayQualifiers;
> > > > + if (CastsAwayConstness(Self, SrcType, DestType, true, false,
> > > > + &TheOffendingSrcType, &TheOffendingDestType,
> > > > + &CastAwayQualifiers)) {
> > > > + const char *qualifiers;
> > > > + const char *suffix = "";
> > > > + if (CastAwayQualifiers.hasConst() &&
> > > CastAwayQualifiers.hasVolatile()) {
> > > > + qualifiers = "const and volatile";
> > > > + suffix = "s";
> > > > + } else if (CastAwayQualifiers.hasConst())
> > > > + qualifiers = "const";
> > > > + else if (CastAwayQualifiers.hasVolatile())
> > > > + qualifiers = "volatile";
> > > > + else {
> > > > + llvm_unreachable("Impossible qualifier");
> > > > + }
> > > > + Self.Diag(SrcExpr.get()->getLocStart(), diag::warn_cast_qual) <<
> > > > + TheOffendingSrcType << TheOffendingDestType << qualifiers <<
> > > suffix;
> > > > + }
> > > > }
> > > >
> > > > ExprResult Sema::BuildCStyleCastExpr(SourceLocation LPLoc,
> > > > Index: test/Sema/warn-cast-qual.c
> > > > ===================================================================
> > > > --- test/Sema/warn-cast-qual.c (revision 0)
> > > > +++ test/Sema/warn-cast-qual.c (working copy)
> > > > @@ -0,0 +1,19 @@
> > > > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only
> > > -Wcast-qual -verify %s
> > > > +
> > > > +#include <stdint.h>
> > > > +
> > > > +void foo() {
> > > > + const char * const ptr = 0;
> > > > + const char * const *ptrptr = 0;
> > > > + char *y = (char *)ptr; // expected-warning {{cast from 'const
> > > char *' to 'char *' drops const qualifier}}
> > > > + char **y1 = (char **)ptrptr; // expected-warning {{cast from
> > > 'const char *const' to 'char *' drops const qualifier}}
> > > > + const char **y2 = (const char **)ptrptr; // expected-warning
> {{cast
> > > from 'const char *const *' to 'const char **' drops const qualifier}}
> > > > +
> > > > + char *z = (char *)(uintptr_t)(const void *)ptr; // no warning
> > > > + char *z1 = (char *)(const void *)ptr; // expected-warning
> {{cast
> > > from 'const void *' to 'char *' drops const qualifier}}
> > > > +
> > > > + volatile char *vol = 0;
> > > > + char *vol2 = (char *)vol; // expected-warning {{cast from
> 'volatile
> > > char *' to 'char *' drops volatile qualifier}}
> > > > + const volatile char *volc = 0;
> > > > + char *volc2 = (char *)volc; // expected-warning {{cast from 'const
> > > volatile char *' to 'char *' drops const and volatile qualifiers}}
> > > > +}
> > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141119/a57c7b4b/attachment.html>
More information about the cfe-commits
mailing list