<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 18, 2014 at 2:05 PM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">New patch attached. I've checked the behaviour against gcc5 and we warn on the<br>
same cases.<br></blockquote><div><br>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).<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">For the const int **bahc = (const int **)bah; case gcc5 emits a warning with<br>
a different warning (to be safe all intermediate pointers in cast from 'int **'<br>
to 'volatile int **' must be 'const' qualified [-Wcast-qual]). Do you think<br>
it's worth adding similarly worded warning for that case or is the "dropping<br>
const" ok?<br></blockquote><div><br></div><div>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?<br><br>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.<br><br>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:<br><br>Warning<"cast from %0 to %1 drops %select{const and volatile qualifiers|const qualifier|volatile qualifier}2"><br><br>int qualifiers = 0;<br>if (const && volatile)<br> qualifiers = 0;<br>else if (const)<br> qualifiers = 1;<br>else if (volatile)<br> qualifiers = 2;<br><br>etc.<br><br>& 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).<br><br><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><br>
On Tue, Nov 18, 2014 at 09:27:17AM -0800, David Blaikie wrote:<br>
> The last line should have a warning on it (and assuming the<br>
> expected-warning comments in the test are correct, it doesn't):<br>
><br>
> GCC 4.9:<br>
><br>
> warning: cast from type 'int**' to type 'const int**' casts away qualifiers<br>
> [-Wcast-qual]<br>
> const int **bahc = (const int **)bah;<br>
><br>
> On Tue, Nov 18, 2014 at 6:11 AM, Roman Divacky <<a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>> wrote:<br>
><br>
> > Scrap the previous patch. It doesnt work when we add qualifiers. The new<br>
> > version is fixed.<br>
> ><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<br>
> > all?<br>
> > > ><br>
> > > > Chatting to Richard over lunch he mentioned an interesting case where<br>
> > 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<br>
> > 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<br>
> > is a<br>
> > > > "qualification conversion" which is the description of the valid<br>
> > 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,<br>
> > 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** -><br>
> > const<br>
> > > > int**" right: cast from type ???int**??? to type ???const int**???<br>
> > 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>><br>
> > 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<br>
> > warning<br>
> > > > > on<br>
> > > > > > the innermost issue, reporting const or volatile - what about<br>
> > 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<br>
> > 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<br>
> > 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<br>
> > InnerMostSrcType in<br>
> > > > > > the following if - it's invariant that if one is non-null (you can<br>
> > use<br>
> > > > > > QualType values rather than QualType* to address the previous bug,<br>
> > 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 <<br>
> > <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<br>
> > '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<br>
> > only when<br>
> > > > > > > > there's a valid implicit conversion like the void* example?)<br>
> > then we<br>
> > > > > > > should<br>
> > > > > > > > probably drop the top level const, "const char *const" -><br>
> > "char*" -<br>
> > > > > the<br>
> > > > > > > top<br>
> > > > > > > > level const on the first type is confusing/misleading, it's<br>
> > only<br>
> > > > > relevant<br>
> > > > > > > > to show "const char*" and "char*".<br>
> > > > > > > ><br>
> > > > > > > ><br>
> > > > > > > > On Wed, Nov 12, 2014 at 10:41 AM, Roman Divacky <<br>
> > <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<br>
> > wrote:<br>
> > > > > > > > > > (it's a bit easier if you include the test in the same<br>
> > patch<br>
> > > > > file -<br>
> > > > > > > also<br>
> > > > > > > > > > you can use Phabricator if you like - some reviewers<br>
> > 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<br>
> > on a > 1<br>
> > > > > > > level<br>
> > > > > > > > > of<br>
> > > > > > > > > > depth? Demonstrate that we warn on both levels (if that's<br>
> > the<br>
> > > > > right<br>
> > > > > > > thing<br>
> > > > > > > > > > to do?)?<br>
> > > > > > > > > ><br>
> > > > > > > > > > Optionally (probably in a separate follow-up patch) you<br>
> > 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<br>
> > short<br>
> > > > > > > (attached<br>
> > > > > > > > > + a<br>
> > > > > > > > > > > test<br>
> > > > > > > > > > > case).<br>
> > > > > > > > > > ><br>
> > > > > > > > > > > This fixes #13772 and also note that -Wcast-qual is used<br>
> > 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>
> > > 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<br>
> > }0"<br>
> > > "%select{struct|union}1 has size 0 in C, %select{size 1|non-zero<br>
> > 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<br>
> > DestType,<br>
> > > - bool CheckCVR, bool CheckObjCLifetime);<br>
> > > + bool CheckCVR, bool CheckObjCLifetime,<br>
> > > + QualType *TheOffendingSrcType = nullptr,<br>
> > > + QualType *TheOffendingDestType = nullptr,<br>
> > > + Qualifiers *CastAwayQualifiers =<br>
> > nullptr);<br>
> > ><br>
> > > // The Try functions attempt a specific way of casting. If they<br>
> > succeed, they<br>
> > > // return TC_Success. If their way of casting is not appropriate for<br>
> > the given<br>
> > > @@ -462,7 +465,10 @@<br>
> > > /// \param CheckObjCLifetime Whether to check Objective-C lifetime<br>
> > 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<br>
> > 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,<br>
> > 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>
> > &&<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() &&<br>
> > 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 <<<br>
> > 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<br>
> > -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<br>
> > char *' to 'char *' drops const qualifier}}<br>
> > > + char **y1 = (char **)ptrptr; // expected-warning {{cast from<br>
> > 'const char *const' to 'char *' drops const qualifier}}<br>
> > > + const char **y2 = (const char **)ptrptr; // expected-warning {{cast<br>
> > 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<br>
> > from 'const void *' to 'char *' drops const qualifier}}<br>
> > > +<br>
> > > + volatile char *vol = 0;<br>
> > > + char *vol2 = (char *)vol; // expected-warning {{cast from 'volatile<br>
> > char *' to 'char *' drops volatile qualifier}}<br>
> > > + const volatile char *volc = 0;<br>
> > > + char *volc2 = (char *)volc; // expected-warning {{cast from 'const<br>
> > volatile char *' to 'char *' drops const and volatile qualifiers}}<br>
> > > +}<br>
> ><br>
> ><br>
</div></div></blockquote></div><br></div></div>