[PATCH]: Implementation of -Wcast-qual
David Blaikie
dblaikie at gmail.com
Tue Nov 18 09:27:17 PST 2014
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/20141118/207dab15/attachment.html>
More information about the cfe-commits
mailing list