[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]

On May 29, 2012, at 6:24 PM, Chandler Carruth wrote:

> On Tue, May 29, 2012 at 5:34 PM, Anna Zaks <ganna at apple.com> wrote:
> +          // 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()));
> +          }
> 
> I'm not terribly fond of this kind of highly customized printing logic when we may not even emit the diagnostic.
> 

Are you concerned about performance? I doubt that this warning is triggered much and I only added retrieval of more suitable source location information in case the faulty call is a builtin macro.

As is the warning quality is unacceptable when memcpy(and others) are defined as builtins:

-/Users/anya/tmp/ex.m:3:27: warning: argument to 'sizeof' in '__builtin___memcpy_chk' call is the same expression as the
-      destination; did you mean to dereference it? [-Wsizeof-pointer-memaccess]
-  memcpy(to, from, sizeof(to));
-  ~~~~~~~~~~~~~~~~~~~~~~~~^~~~
-/usr/include/secure/_string.h:55:41: note: expanded from macro 'memcpy'
-   ? __builtin___memcpy_chk (dest, src, len, __darwin_obsz0 (dest))     \

+/Users/anya/tmp/ex.m:3:27: warning: argument to 'sizeof' in memcpy' call is the same expression as the
+     destination; did you mean to dereference it? [-Wsizeof-pointer-memaccess]
+ memcpy(to, from, sizeof(to));
  ~~~~~~~~~~~~~~~~~~~~~~~~^~~~


> I wonder -- can we construct this information from within the diagnostic printer, and get it to fire in more circumstances? If not, is there a way to simply tell the diagnostic engine the intent -- to print the builtin name -- rather than hard coding the logic to get from here to there?


I agree that we should come up with a better solution for handling builtin printing, which would apply in all cases. Since diagnostic engine works with SourceLocations, I am not sure it can detect if a location is a builtin or not. Teaching it to print builtins sounds like a plausible idea. However, we might be breaking the abstraction by doing that. (We might as well teach it about printing functions.) I am not very well familiar with that code.

This band-aid fix can be removed after the proper solution is in place.

Anna.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120529/c85280e7/attachment.html>