From 869aeccadab5555800f729d56254c8f3355e1b9a Mon Sep 17 00:00:00 2001
From: Anna Zaks <ganna@apple.com>
Date: Wed, 30 May 2012 00:34:21 +0000
Subject: [PATCH] Add fixits for memory access warnings. Also, do not display
 the builtin name and macro expansion when the function is a builtin.

llvm-svn: 157659
---
 .../clang/Basic/DiagnosticSemaKinds.td        |  2 +-
 clang/lib/Sema/SemaChecking.cpp               | 40 ++++++++++++++++---
 .../SemaCXX/warn-memset-bad-sizeof-fixit.cpp  | 24 +++++++++++
 3 files changed, 60 insertions(+), 6 deletions(-)
 create mode 100644 clang/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 98e387bece5a..483145bb8a8b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -331,7 +331,7 @@ def warn_dyn_class_memaccess : Warning<
 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">>;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 769705419e88..004e9948f202 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2755,19 +2755,49 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
           // 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;
         }
       }
diff --git a/clang/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp b/clang/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp
new file mode 100644
index 000000000000..f76f49739300
--- /dev/null
+++ b/clang/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp
@@ -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;
+}