On Wed, May 30, 2012 at 1:14 PM, Anna Zaks <[email protected]> wrote: > > On May 29, 2012, at 6:10 PM, Richard Smith wrote: > > Hi, > > On Tue, May 29, 2012 at 5:34 PM, Anna Zaks <[email protected]> wrote: > >> Author: zaks >> Date: Tue May 29 19:34:21 2012 >> New Revision: 157659 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=157659&view=rev >> Log: >> Add fixits for memory access warnings. >> > > We have a rule that fix-its on warnings aren't supposed to change the > code's semantics (-Werror -fixit shouldn't change the semantics of a valid > program). Consequently, fix-its such as this are usually placed on an > accompanying note. > > > > We discussed this fixit internally, and reached a consensus that we should > not issue a fixit in this case. > The warning only warns on the following code, where the user most likely > forgot to both dereference AND multiply by the number of elements. >
It's likely they forgot the number of elements just because memcpy is usually used to copy several elements - or is there some other reason to believe the suggested fixit isn't really adequate? I'm curious why we would suggest what amounts to a fixit in the diagnostic, but not as an actual fixit in a note (indeed the sentence fragment in the diagnostic could be pulled out into the text of the note). - David > memcpy(to, from, sizeof(to)); > > Note, we currently do not (but should) warn on the following, where the > fixit would be more appropriate. > memcpy(to, from, sizeof(to)*N); > > I plan to make the warning message more user-friendly: > - warning: argument to 'sizeof' in 'memcpy' call is the same expression > as the destination; did you mean to dereference it? > - memcpy(to, from, sizeof(to)) > - ~~ ~~ > > + warning: 'memcpy' call operates on objects of type 'Blah' while the > size is based on a different type 'Blah*'. > + memcpy(to, from, sizeof(to)) > + ~~ ~~ > + note: did you mean to dereference the argument to 'sizeof' (and > multiply it by the number of elements)? > > Anna. > > Also, do not display the builtin name and macro expansion when the >> function is a builtin. >> >> Added: >> cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp >> Modified: >> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> cfe/trunk/lib/Sema/SemaChecking.cpp >> >> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=157659&r1=157658&r2=157659&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) >> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue May 29 >> 19:34:21 2012 >> @@ -331,7 +331,7 @@ >> def note_bad_memaccess_silence : Note< >> "explicitly cast the pointer to silence this warning">; >> def warn_sizeof_pointer_expr_memaccess : Warning< >> - "argument to 'sizeof' in %0 call is the same expression as the " >> + "argument to 'sizeof' in '%0' call is the same expression as the " >> "%select{destination|source}1; did you mean to " >> "%select{dereference it|remove the addressof|provide an explicit >> length}2?">, >> InGroup<DiagGroup<"sizeof-pointer-memaccess">>; >> >> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=157659&r1=157658&r2=157659&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue May 29 19:34:21 2012 >> @@ -2755,19 +2755,49 @@ >> // TODO: For strncpy() and friends, this could suggest >> sizeof(dst) >> // over sizeof(src) as well. >> unsigned ActionIdx = 0; // Default is to suggest dereferencing. >> + FixItHint Fixit = FixItHint(); // Default hint. >> + StringRef ReadableName = FnName->getName(); >> + >> + if (isa<DeclRefExpr>(SizeOfArg)) >> + Fixit = FixItHint::CreateInsertion(SizeOfArg->getLocStart(), >> "*"); >> + >> if (const UnaryOperator *UnaryOp = >> dyn_cast<UnaryOperator>(Dest)) >> - if (UnaryOp->getOpcode() == UO_AddrOf) >> + if (UnaryOp->getOpcode() == UO_AddrOf) { >> + Fixit = FixItHint::CreateRemoval( >> + >> CharSourceRange::getTokenRange(SizeOfArg->getLocStart(), >> + >> SizeOfArg->getLocStart())); >> ActionIdx = 1; // If its an address-of operator, just >> remove it. >> + } >> if (Context.getTypeSize(PointeeTy) == Context.getCharWidth()) >> ActionIdx = 2; // If the pointee's size is sizeof(char), >> // suggest an explicit length. >> unsigned DestSrcSelect = >> (BId == Builtin::BIstrndup ? 1 : ArgIdx); >> - DiagRuntimeBehavior(SizeOfArg->getExprLoc(), Dest, >> + >> + // If the function is defined as a builtin macro, do not show >> macro >> + // expansion. >> + SourceLocation SL = SizeOfArg->getExprLoc(); >> + SourceRange DSR = Dest->getSourceRange(); >> + SourceRange SSR = SizeOfArg->getSourceRange(); >> + SourceManager &SM = PP.getSourceManager(); >> + >> + if (SM.isMacroArgExpansion(SL)) { >> + ReadableName = Lexer::getImmediateMacroName(SL, SM, >> LangOpts); >> + SL = SM.getSpellingLoc(SL); >> + DSR = SourceRange(SM.getSpellingLoc(DSR.getBegin()), >> + SM.getSpellingLoc(DSR.getEnd())); >> + SSR = SourceRange(SM.getSpellingLoc(SSR.getBegin()), >> + SM.getSpellingLoc(SSR.getEnd())); >> + } >> + >> + DiagRuntimeBehavior(SL, Dest, >> >> PDiag(diag::warn_sizeof_pointer_expr_memaccess) >> - << FnName << DestSrcSelect << ActionIdx >> - << Dest->getSourceRange() >> - << SizeOfArg->getSourceRange()); >> + << ReadableName >> + << DestSrcSelect >> + << ActionIdx >> + << DSR >> + << SSR >> + << Fixit); >> break; >> } >> } >> >> Added: cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp?rev=157659&view=auto >> >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp (added) >> +++ cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp Tue May 29 >> 19:34:21 2012 >> @@ -0,0 +1,24 @@ >> +// RUN: cp %s %t >> +// RUN: not %clang_cc1 -fixit -Werror -x c++ -std=c++98 %t >> +// RUN: %clang_cc1 -fsyntax-only -Werror -x c++ -std=c++98 %t >> +// RUN: cp %s %t >> +// RUN: not %clang_cc1 -DUSE_BUILTINS -fixit -Werror -x c++ -std=c++98 %t >> +// RUN: %clang_cc1 -DUSE_BUILTINS -fsyntax-only -Werror -x c++ >> -std=c++98 %t >> + >> +extern "C" void *memcpy(void *s1, const void *s2, unsigned n); >> + >> +#ifdef USE_BUILTINS >> +# define BUILTIN(f) __builtin_ ## f >> +#else >> +# define BUILTIN(f) f >> +#endif >> + >> +#define memcpy BUILTIN(memcpy) >> + >> +int testFixits(int *to, int *from) { >> + memcpy(to, from, sizeof(to)); // \ >> + // expected-warning {{argument to 'sizeof' in 'memcpy' call is >> the same expression as the destination; did you mean to dereference it?}} >> + memcpy(0, &from, sizeof(&from)); // \ >> + // expected-warning {{argument to 'sizeof' in 'memcpy' call is >> the same expression as the source; did you mean to remove the addressof?}} >> + return 0; >> +} >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
