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