[cfe-commits] r157659 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp
David Blaikie
dblaikie at gmail.com
Wed May 30 16:26:00 PDT 2012
On Wed, May 30, 2012 at 1:14 PM, Anna Zaks <ganna at apple.com> wrote:
>
> 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.
>
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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
>
> _______________________________________________
> 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/9ea7adc9/attachment.html>
More information about the cfe-commits
mailing list