[cfe-commits] r157659 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp
Anna Zaks
ganna at apple.com
Wed May 30 13:14:27 PDT 2012
On May 29, 2012, at 6:10 PM, Richard Smith wrote:
> Hi,
>
> On Tue, May 29, 2012 at 5:34 PM, Anna Zaks <ganna at apple.com> 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.
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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120530/5959971d/attachment.html>
More information about the cfe-commits
mailing list