Re: [PATCH] D12832: [Driver] Add support for Windows 10 SDK

[Igor Kudrin via cfe-commits]

ikudrin marked 2 inline comments as done.
ikudrin added a comment.

http://reviews.llvm.org/D12832



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12832: [Driver] Added support for Windows 10 SDK

[Igor Kudrin via cfe-commits]

ikudrin updated this revision to Diff 34965.
ikudrin added a comment.

- reworked the conditional statement
- formatting


http://reviews.llvm.org/D12832

Files:
  cfe/trunk/lib/Driver/MSVCToolChain.cpp
  cfe/trunk/lib/Driver/ToolChains.h

Index: cfe/trunk/lib/Driver/ToolChains.h
===
--- cfe/trunk/lib/Driver/ToolChains.h
+++ cfe/trunk/lib/Driver/ToolChains.h
@@ -838,7 +838,9 @@
   const llvm::opt::ArgList &DriverArgs,
   llvm::opt::ArgStringList &CC1Args) const override;
 
-  bool getWindowsSDKDir(std::string &path, int &major, int &minor) const;
+  bool getWindowsSDKDir(std::string &path, int &major,
+std::string &windowsSDKIncludeVersion,
+std::string &windowsSDKLibVersion) const;
   bool getWindowsSDKLibraryPath(std::string &path) const;
   /// \brief Check if Universal CRT should be used if available
   bool useUniversalCRT(std::string &visualStudioDir) const;
@@ -856,7 +858,9 @@
   void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList &DriverArgs,
  llvm::opt::ArgStringList &CC1Args,
  const std::string &folder,
- const char *subfolder) const;
+ const Twine &subfolder1,
+ const Twine &subfolder2 = "",
+ const Twine &subfolder3 = "") const;
 
   Tool *buildLinker() const override;
   Tool *buildAssembler() const override;
Index: cfe/trunk/lib/Driver/MSVCToolChain.cpp
===
--- cfe/trunk/lib/Driver/MSVCToolChain.cpp
+++ cfe/trunk/lib/Driver/MSVCToolChain.cpp
@@ -220,27 +220,83 @@
   }
 }
 
+// Find the most recent version of Universal CRT or Windows 10 SDK.
+// vcvarsqueryregistry.bat from Visual Studio 2015 sorts entries in the include
+// directory by name and uses the last one of the list.
+// So we compare entry names lexicographically to find the greatest one.
+static bool getWindows10SDKVersion(const std::string &SDKPath,
+   std::string &SDKVersion) {
+  SDKVersion.clear();
+
+  std::error_code EC;
+  llvm::SmallString<128> IncludePath(SDKPath);
+  llvm::sys::path::append(IncludePath, "Include");
+  for (llvm::sys::fs::directory_iterator DirIt(IncludePath, EC), DirEnd;
+   DirIt != DirEnd && !EC; DirIt.increment(EC)) {
+if (!llvm::sys::fs::is_directory(DirIt->path()))
+  continue;
+const StringRef CandidateName = llvm::sys::path::filename(DirIt->path());
+if (CandidateName > SDKVersion)
+  SDKVersion = CandidateName;
+  }
+
+  return !SDKVersion.empty();
+}
+
 /// \brief Get Windows SDK installation directory.
-bool MSVCToolChain::getWindowsSDKDir(std::string &path, int &major,
- int &minor) const {
-  std::string sdkVersion;
+bool MSVCToolChain::getWindowsSDKDir(std::string &Path, int &Major,
+ std::string &WindowsSDKIncludeVersion,
+ std::string &WindowsSDKLibVersion) const {
+  std::string RegistrySDKVersion;
   // Try the Windows registry.
-  bool hasSDKDir = getSystemRegistryString(
-  "SOFTWARE\\Microsoft\\Microsoft SDKs\\Windows\\$VERSION",
-  "InstallationFolder", path, &sdkVersion);
-  if (!sdkVersion.empty())
-std::sscanf(sdkVersion.c_str(), "v%d.%d", &major, &minor);
-  return hasSDKDir && !path.empty();
+  if (!getSystemRegistryString(
+  "SOFTWARE\\Microsoft\\Microsoft SDKs\\Windows\\$VERSION",
+  "InstallationFolder", Path, &RegistrySDKVersion))
+return false;
+  if (Path.empty() || RegistrySDKVersion.empty())
+return false;
+
+  WindowsSDKIncludeVersion.clear();
+  WindowsSDKLibVersion.clear();
+  Major = 0;
+  std::sscanf(RegistrySDKVersion.c_str(), "v%d.", &Major);
+  if (Major <= 7)
+return true;
+  if (Major == 8) {
+// Windows SDK 8.x installs libraries in a folder whose names depend on the
+// version of the OS you're targeting.  By default choose the newest, which
+// usually corresponds to the version of the OS you've installed the SDK on.
+const char *Tests[] = {"winv6.3", "win8", "win7"};
+for (const char *Test : Tests) {
+  llvm::SmallString<128> TestPath(Path);
+  llvm::sys::path::append(TestPath, "Lib", Test);
+  if (llvm::sys::fs::exists(TestPath.c_str())) {
+WindowsSDKLibVersion = Test;
+break;
+  }
+}
+return !WindowsSDKLibVersion.empty();
+  }
+  if (Major == 10) {
+if (!getWindows10SDKVersion(Path, WindowsSDKIncludeVersion))
+  return false;
+WindowsSDKLibVersion = WindowsSDKIncludeVersion;
+return true;
+  }
+  // Unsupported SDK version
+  return false;
 }
 
 // Gets the library path required to link against the Windows SDK.
 bool MSVCToolChain::getWindowsSDKLibraryPath(std::string &path) co

Re: [PATCH] D12921: clang-format: Support 'template<>' (no space).

[Daniel Jasper via cfe-commits]

djasper added a comment.

This has come up before and the decision was that this is not important enough 
to meet the bar for an additional clang-format option. clang-format options 
have a certain cost and this specific space is so entirely unimportant that we 
don't want to pay it.

We have tried to eradicate this style wherever we find it (e.g. in the C++ 
standard) and encourage people to just change their style here if they want to 
use clang-format.


http://reviews.llvm.org/D12921



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12832: [Driver] Added support for Windows 10 SDK

[David Majnemer via cfe-commits]

majnemer added a subscriber: majnemer.


Comment at: cfe/trunk/lib/Driver/MSVCToolChain.cpp:263
@@ +262,3 @@
+  std::sscanf(RegistrySDKVersion.c_str(), "v%d.", &Major);
+  if (Major > 7 && Major < 10) {
+// Windows SDK 8.x installs libraries in a folder whose names depend on the

Isn't this just `Major == 8 || Major == 9` ?


Comment at: cfe/trunk/lib/Driver/MSVCToolChain.cpp:278-279
@@ +277,4 @@
+  return false;
+  }
+  else {
+if (!getWindows10SDKVersion(Path, WindowsSDKIncludeVersion))

Same line please.


http://reviews.llvm.org/D12832



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12832: [Driver] Added support for Windows 10 SDK

[Igor Kudrin via cfe-commits]

ikudrin added a comment.

Ping?


http://reviews.llvm.org/D12832



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12922: Add support for function attribute "notail"

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

The llvm-side patch is here: http://reviews.llvm.org/D12923


http://reviews.llvm.org/D12922



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12922: Add support for function attribute "notail"

[Akira Hatanaka via cfe-commits]

ahatanak created this revision.
ahatanak added a reviewer: aaron.ballman.
ahatanak added a subscriber: cfe-commits.

This patch adds support for a new function attribute "notail". The attribute is 
used to disable tail call optimization on calls to functions marked with the 
attribute.

This is different from the attribute proposed in D12547, which disables tail 
call optimizations on call sites within the marked function.

http://reviews.llvm.org/D12547

http://reviews.llvm.org/D12922

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/CodeGen/CGCall.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/attr-no-tail.c

Index: test/CodeGen/attr-no-tail.c
===
--- /dev/null
+++ test/CodeGen/attr-no-tail.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -o - | 
FileCheck %s
+
+// CHECK: define i32 @callee0(i32 %a) [[ATTRNOTAIL0:#[0-9]+]] {
+// CHECK: declare i32 @callee1(i32) [[ATTRNOTAIL1:#[0-9]+]]
+// CHECK: declare i32 @callee2(i32) [[ATTRTAIL0:#[0-9]+]]
+
+int callee0(int a) __attribute__((notail)) {
+  return a + 1;
+}
+
+int callee1(int) __attribute__((notail));
+
+int callee2(int);
+
+int caller0(int a) {
+  if (a > 0)
+return callee0(a);
+  if (a < 0)
+return callee1(a);
+  return callee2(a);
+}
+
+// CHECK: attributes [[ATTRNOTAIL0]] = { {{.*}}notail{{.*}} }
+// CHECK: attributes [[ATTRNOTAIL1]] = { {{.*}}notail{{.*}} }
+// CHECK-NOT: attributes [[ATTRTAIL0]] = { {{.*}}notail{{.*}} }
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4878,6 +4878,9 @@
   case AttributeList::AT_ReturnsTwice:
 handleSimpleAttribute(S, D, Attr);
 break;
+  case AttributeList::AT_NoTail:
+handleSimpleAttribute(S, D, Attr);
+break;
   case AttributeList::AT_Used:
 handleUsedAttr(S, D, Attr);
 break;
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -1414,6 +1414,8 @@
   FuncAttrs.addAttribute(llvm::Attribute::NoReturn);
 if (TargetDecl->hasAttr())
   FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);
+if (TargetDecl->hasAttr())
+  FuncAttrs.addAttribute(llvm::Attribute::NoTail);
 
 if (const FunctionDecl *Fn = dyn_cast(TargetDecl)) {
   const FunctionProtoType *FPT = Fn->getType()->getAs();
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -1612,3 +1612,10 @@
 arguments, with arbitrary offsets.
   }];
 }
+
+def NoTailDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Tail call optimization is not performed on calls to a function marked 
``notail``.
+  }];
+}
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -1020,6 +1020,12 @@
   let Documentation = [Undocumented];
 }
 
+def NoTail : InheritableAttr {
+  let Spellings = [GNU<"notail">, CXX11<"clang", "notail">];
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let Documentation = [NoTailDocs];
+}
+
 def NoThrow : InheritableAttr {
   let Spellings = [GCC<"nothrow">, Declspec<"nothrow">];
   let Documentation = [Undocumented];


Index: test/CodeGen/attr-no-tail.c
===
--- /dev/null
+++ test/CodeGen/attr-no-tail.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -o - | FileCheck %s
+
+// CHECK: define i32 @callee0(i32 %a) [[ATTRNOTAIL0:#[0-9]+]] {
+// CHECK: declare i32 @callee1(i32) [[ATTRNOTAIL1:#[0-9]+]]
+// CHECK: declare i32 @callee2(i32) [[ATTRTAIL0:#[0-9]+]]
+
+int callee0(int a) __attribute__((notail)) {
+  return a + 1;
+}
+
+int callee1(int) __attribute__((notail));
+
+int callee2(int);
+
+int caller0(int a) {
+  if (a > 0)
+return callee0(a);
+  if (a < 0)
+return callee1(a);
+  return callee2(a);
+}
+
+// CHECK: attributes [[ATTRNOTAIL0]] = { {{.*}}notail{{.*}} }
+// CHECK: attributes [[ATTRNOTAIL1]] = { {{.*}}notail{{.*}} }
+// CHECK-NOT: attributes [[ATTRTAIL0]] = { {{.*}}notail{{.*}} }
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4878,6 +4878,9 @@
   case AttributeList::AT_ReturnsTwice:
 handleSimpleAttribute(S, D, Attr);
 break;
+  case AttributeList::AT_NoTail:
+handleSimpleAttribute(S, D, Attr);
+break;
   case AttributeList::AT_Used:
 handleUsedAttr(S, D, Attr);
 break;
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -1414,6 +1

[PATCH] D12921: clang-format: Support 'template<>' (no space).

[strager via cfe-commits]

strager created this revision.
strager added a reviewer: djasper.
strager added subscribers: cfe-commits, abdulras, sas.
Herald added a subscriber: klimek.

Some styles don't put a space between 'template' and the
opening '<'. Introduce SpaceAfterTemplateKeyword which, when
set to false, causes 'template' and '<' to not have a space
between.

http://reviews.llvm.org/D12921

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5728,6 +5728,17 @@
   verifyFormat("template  void Foo(Ts*... ts) {}", PointersLeft);
 }
 
+TEST_F(FormatTest, SpaceAfterTemplate) {
+  FormatStyle Style = getLLVMStyle();
+  Style.SpaceAfterTemplateKeyword = false;
+  verifyFormat("template<> class Foo {}", Style);
+  verifyFormat("template<> void Foo() {}", Style);
+  verifyFormat("template class Foo {}", Style);
+  verifyFormat("template void Foo() {}", Style);
+  verifyFormat("template class Foo {}", Style);
+  verifyFormat("template void Foo() {}", Style);
+}
+
 TEST_F(FormatTest, AdaptivelyFormatsPointersAndReferences) {
   EXPECT_EQ("int *a;\n"
 "int *a;\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1827,7 +1827,7 @@
   if (Right.isOneOf(tok::semi, tok::comma))
 return false;
   if (Right.is(tok::less) &&
-  (Left.is(tok::kw_template) ||
+  ((Left.is(tok::kw_template) && Style.SpaceAfterTemplateKeyword) ||
(Line.Type == LT_ObjCDecl && Style.ObjCSpaceBeforeProtocolList)))
 return true;
   if (Left.isOneOf(tok::exclaim, tok::tilde))
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -268,6 +268,7 @@
Style.PenaltyReturnTypeOnItsOwnLine);
 IO.mapOptional("PointerAlignment", Style.PointerAlignment);
 IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
+IO.mapOptional("SpaceAfterTemplateKeyword", 
Style.SpaceAfterTemplateKeyword);
 IO.mapOptional("SpaceBeforeAssignmentOperators",
Style.SpaceBeforeAssignmentOperators);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
@@ -398,6 +399,7 @@
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpaceAfterCStyleCast = false;
+  LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpacesInAngles = false;
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -367,6 +367,10 @@
   /// \brief If \c true, a space may be inserted after C style casts.
   bool SpaceAfterCStyleCast;
 
+  /// \brief If \c true, a space may be inserted between the 'template' keyword
+  /// and the following '<'.
+  bool SpaceAfterTemplateKeyword;
+
   /// \brief If \c false, spaces will be removed before assignment operators.
   bool SpaceBeforeAssignmentOperators;
 
@@ -512,6 +516,7 @@
PenaltyReturnTypeOnItsOwnLine == R.PenaltyReturnTypeOnItsOwnLine &&
PointerAlignment == R.PointerAlignment &&
SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
+   SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators 
&&
SpaceBeforeParens == R.SpaceBeforeParens &&
SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
Index: docs/ClangFormatStyleOptions.rst
===
--- docs/ClangFormatStyleOptions.rst
+++ docs/ClangFormatStyleOptions.rst
@@ -481,6 +481,10 @@
 **SpaceAfterCStyleCast** (``bool``)
   If ``true``, a space may be inserted after C style casts.
 
+**SpaceAfterTemplateKeyword** (``bool``)
+  If ``true``, a space may be inserted between the 'template' keyword
+  and the following '<'.
+
 **SpaceBeforeAssignmentOperators** (``bool``)
   If ``false``, spaces will be removed before assignment operators.
 


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5728,6 +5728,17 @@
   verifyFormat("template  void Foo(Ts*... ts) {}", PointersLeft);
 }
 
+TEST_F(FormatTest, SpaceAfterTemplate) {
+  FormatStyle Style = getLLVMStyle();
+  Style.SpaceAfterTemplateKeyword = false;
+  verifyFormat("template<> class Foo {}", Style);
+  verifyFormat("template<> v

Re: [PATCH] D10371: clang-format: Support @synchronized.

[strager via cfe-commits]

strager updated this revision to Diff 34957.
strager added a comment.

Rebase. Fix style issues.


http://reviews.llvm.org/D10371

Files:
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2419,6 +2419,30 @@
Style);
 }
 
+TEST_F(FormatTest, FormatObjCSynchronized) {
+  FormatStyle Style = getLLVMStyleWithColumns(32);
+  verifyFormat("@synchronized(foo) {\n"
+   "  f();\n"
+   "}\n",
+   Style);
+  verifyFormat("@synchronized([self\n"
+   "veryLongMethodNameWithParameters:\n"
+   "YES]) {\n"
+   "  f();\n"
+   "}\n",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  verifyFormat("@synchronized(foo)\n"
+   "{\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(foo)\n"
+   "{\n"
+   "  f();\n"
+   "}\n",
+   Style);
+}
+
 TEST_F(FormatTest, StaticInitializers) {
   verifyFormat("static SomeClass SC = {1, 'a'};");
 
@@ -8373,6 +8397,10 @@
"  break;\n"
"}",
NoSpace);
+  verifyFormat("@synchronized(x) {\n"
+   "  do_something();\n"
+   "}",
+   NoSpace);
   verifyFormat("auto i = std::make_unique(5);", NoSpace);
   verifyFormat("size_t x = sizeof(x);", NoSpace);
   verifyFormat("auto f(int x) -> decltype(x);", NoSpace);
@@ -8408,6 +8436,10 @@
"  break;\n"
"}",
Space);
+  verifyFormat("@synchronized (x) {\n"
+   "  do_something ();\n"
+   "}",
+   Space);
   verifyFormat("A::A () : a (1) {}", Space);
   verifyFormat("void f () __attribute__ ((asdf));", Space);
   verifyFormat("*(&a + 1);\n"
@@ -8460,6 +8492,10 @@
"  break;\n"
"}",
Spaces);
+  verifyFormat("@synchronized( x ) {\n"
+   "  do_something();\n"
+   "}",
+   Spaces);
 
   Spaces.SpacesInParentheses = false;
   Spaces.SpacesInCStyleCastParentheses = true;
@@ -8497,6 +8533,10 @@
"  break;\n"
"}",
Spaces);
+  verifyFormat("@synchronized(x) {\n"
+   "  do_something( );\n"
+   "}",
+   Spaces);
 
   // Run the first set of tests again with:
   Spaces.SpaceAfterCStyleCast = true;
@@ -8523,6 +8563,10 @@
"  break;\n"
"}",
Spaces);
+  verifyFormat("@synchronized(x) {\n"
+   "  do_something( );\n"
+   "}",
+   Spaces);
 
   // Run subset of tests again with:
   Spaces.SpacesInCStyleCastParentheses = false;
@@ -8534,6 +8578,10 @@
"  do_something((int) i);\n"
"} while (something( ));",
Spaces);
+  verifyFormat("@synchronized((NSLock) x) {\n"
+   "  do_something( );\n"
+   "}",
+   Spaces);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInSquareBrackets) {
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -679,8 +679,12 @@
   addUnwrappedLine();
   return;
 case tok::objc_autoreleasepool:
+case tok::objc_synchronized:
   nextToken();
-  if (FormatTok->Tok.is(tok::l_brace)) {
+  if (FormatTok->is(tok::l_paren)) {
+parseParens();
+  }
+  if (FormatTok->is(tok::l_brace)) {
 if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
 Style.BreakBeforeBraces == FormatStyle::BS_GNU)
   addUnwrappedLine();
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -122,7 +122,7 @@
 if (Left->Previous &&
 (Left->Previous->isOneOf(tok::kw_static_assert, tok::kw_decltype,
  tok::kw_if, tok::kw_while, tok::l_paren,
- tok::comma) ||
+ tok::comma, Keywords.kw_synchronized) ||
  Left->Previous->is(TT_BinaryOperator))) {
   // static_assert, if and while usually contain expressions.
   Contexts.back().IsExpression = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12614: [OpenMP] Offloading descriptor registration and device codegen.

[Samuel Antao via cfe-commits]

sfantao updated the summary for this revision.
sfantao updated this revision to Diff 34955.
sfantao added a comment.

Rebase on top of last changes in http://reviews.llvm.org/D12871.


http://reviews.llvm.org/D12614

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Basic/LangOptions.def
  include/clang/Basic/LangOptions.h
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGOpenMPRuntime.h
  lib/CodeGen/CGStmtOpenMP.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/OpenMP/target_codegen.cpp
  test/OpenMP/target_codegen_registration.cpp
  test/OpenMP/target_messages.cpp

Index: test/OpenMP/target_messages.cpp
===
--- test/OpenMP/target_messages.cpp
+++ test/OpenMP/target_messages.cpp
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -verify -fopenmp -std=c++11 -o - %s
+// RUN: not %clang_cc1 -fopenmp -std=c++11 -omptargets=aaa-bbb-ccc-ddd -o - %s 2>&1 | FileCheck %s
+// CHECK: error: OpenMP target is invalid: 'aaa-bbb-ccc-ddd'
 
 void foo() {
 }
Index: test/OpenMP/target_codegen_registration.cpp
===
--- /dev/null
+++ test/OpenMP/target_codegen_registration.cpp
@@ -0,0 +1,437 @@
+// Test host codegen.
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -omptargets=powerpc64le-ibm-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -omptargets=powerpc64le-ibm-linux-gnu -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple powerpc64le-unknown-unknown -omptargets=powerpc64le-ibm-linux-gnu -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple i386-unknown-unknown -omptargets=i386-pc-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple i386-unknown-unknown -omptargets=i386-pc-linux-gnu -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple i386-unknown-unknown -omptargets=i386-pc-linux-gnu -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+
+// Test target codegen - host bc file has to be created first.
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -omptargets=powerpc64le-ibm-linux-gnu -emit-llvm-bc %s -o %t-ppc-host.bc
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -omptargets=powerpc64le-ibm-linux-gnu -emit-llvm %s -fopenmp-is-device -omp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck %s -check-prefix=TCHECK
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -omptargets=powerpc64le-ibm-linux-gnu -emit-pch -fopenmp-is-device -omp-host-ir-file-path %t-ppc-host.bc -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple powerpc64le-unknown-unknown -omptargets=powerpc64le-ibm-linux-gnu -std=c++11 -fopenmp-is-device -omp-host-ir-file-path %t-ppc-host.bc -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s -check-prefix=TCHECK
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple i386-unknown-unknown -omptargets=i386-pc-linux-gnu -emit-llvm-bc %s -o %t-x86-host.bc
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple i386-unknown-unknown -omptargets=i386-pc-linux-gnu -emit-llvm %s -fopenmp-is-device -omp-host-ir-file-path %t-x86-host.bc -o - | FileCheck %s -check-prefix=TCHECK
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple i386-unknown-unknown -omptargets=i386-pc-linux-gnu -emit-pch -fopenmp-is-device -omp-host-ir-file-path %t-x86-host.bc -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple i386-unknown-unknown -omptargets=i386-pc-linux-gnu -std=c++11 -fopenmp-is-device -omp-host-ir-file-path %t-x86-host.bc -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s -check-prefix=TCHECK
+
+// Check that no target code is emmitted if no omptests flag was provided.
+// RxUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECK-NTARGET
+
+// expected-no-diagnostics
+#ifndef HEADER
+#define HEADER
+
+// CHECK-DAG: [[SA:%.+]] = type { [4 x i32] }
+// CHECK-DAG: [[SB:%.+]] = type { [8 x i32] }
+// CHECK-DAG: [[SC:%.+]] = type { [16 x i32] }
+// CHECK-DAG: [[SD:%.+]] = type { [32 x i32] }
+// CHECK-DAG: [[SE:%.+]] = type { [64 x i32] }
+// CHECK-DAG: [[ST1:%.+]] = type { [228 x i32] }
+// CHECK-DAG: [[ST2:%.+]] = type { [1128 x i32] }
+// CHECK-DAG: [[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]] }
+// CHECK-DAG: [[DEVTY:%.+]] = type { i8*, i8*, [[ENTTY]]*, [[ENTTY]]* }
+// CHECK-DAG: [[DSCTY:%.+]] = type { i32, [[DEVTY]]*, [[ENTTY]]*, [[ENTTY]]* }
+
+// TCHECK:[[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]] }
+
+// CHECK-DAG: [[A1:@.+]] = internal global [[SA]]
+// CHECK-DAG: [[A2:@.+]] = global [[SA]]
+

Re: [PATCH] D12700: [clang-tidy] install helper scripts in CMake build

[Eugene Zelenko via cfe-commits]

Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko updated this revision to Diff 34953.
Eugene.Zelenko added a comment.

I added installation in configure. See also http://reviews.llvm.org/D12919 for 
further configure improvements idea.


Repository:
  rL LLVM

http://reviews.llvm.org/D12700

Files:
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/Makefile

Index: clang-tidy/tool/Makefile
===
--- clang-tidy/tool/Makefile
+++ clang-tidy/tool/Makefile
@@ -26,3 +26,22 @@
   clangEdit.a clangAST.a clangLex.a clangBasic.a
 
 include $(CLANG_LEVEL)/Makefile
+
+PROJ_sharedir := $(DESTDIR)$(PROJ_prefix)/share/clang
+
+FILESLIST := $(notdir $(wildcard $(PROJ_SRC_DIR)/*.py))
+
+SRCFILES := $(addprefix $(PROJ_SRC_DIR)/, $(FILESLIST))
+DESTFILES := $(addprefix $(PROJ_sharedir)/, $(FILESLIST))
+
+$(PROJ_sharedir):
+   $(Echo) Making install directory: $@
+   $(Verb) $(MKDIR) $@
+
+$(DESTFILES): $(SRCFILES)
+
+$(PROJ_sharedir)/%.py: $(PROJ_SRC_DIR)/%.py
+   $(Echo) Installing script file: $(notdir $<)
+   $(Verb) $(ScriptInstall) $< $(PROJ_sharedir)
+
+install-local:: $(PROJ_sharedir) $(DESTFILES)
Index: clang-tidy/tool/CMakeLists.txt
===
--- clang-tidy/tool/CMakeLists.txt
+++ clang-tidy/tool/CMakeLists.txt
@@ -21,3 +21,5 @@
 install(TARGETS clang-tidy
   RUNTIME DESTINATION bin)
 
+install(PROGRAMS clang-tidy-diff.py DESTINATION share/clang)
+install(PROGRAMS run-clang-tidy.py DESTINATION share/clang)


Index: clang-tidy/tool/Makefile
===
--- clang-tidy/tool/Makefile
+++ clang-tidy/tool/Makefile
@@ -26,3 +26,22 @@
 	   clangEdit.a clangAST.a clangLex.a clangBasic.a
 
 include $(CLANG_LEVEL)/Makefile
+
+PROJ_sharedir := $(DESTDIR)$(PROJ_prefix)/share/clang
+
+FILESLIST := $(notdir $(wildcard $(PROJ_SRC_DIR)/*.py))
+
+SRCFILES := $(addprefix $(PROJ_SRC_DIR)/, $(FILESLIST))
+DESTFILES := $(addprefix $(PROJ_sharedir)/, $(FILESLIST))
+
+$(PROJ_sharedir):
+	$(Echo) Making install directory: $@
+	$(Verb) $(MKDIR) $@
+
+$(DESTFILES): $(SRCFILES)
+
+$(PROJ_sharedir)/%.py: $(PROJ_SRC_DIR)/%.py
+	$(Echo) Installing script file: $(notdir $<)
+	$(Verb) $(ScriptInstall) $< $(PROJ_sharedir)
+
+install-local:: $(PROJ_sharedir) $(DESTFILES)
Index: clang-tidy/tool/CMakeLists.txt
===
--- clang-tidy/tool/CMakeLists.txt
+++ clang-tidy/tool/CMakeLists.txt
@@ -21,3 +21,5 @@
 install(TARGETS clang-tidy
   RUNTIME DESTINATION bin)
 
+install(PROGRAMS clang-tidy-diff.py DESTINATION share/clang)
+install(PROGRAMS run-clang-tidy.py DESTINATION share/clang)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12919: [clang-format] Make configure installation consistent with CMake

[Eugene Zelenko via cfe-commits]

Eugene.Zelenko created this revision.
Eugene.Zelenko added a reviewer: hans.
Eugene.Zelenko added a subscriber: cfe-commits.
Eugene.Zelenko set the repository for this revision to rL LLVM.

During D12700 review I was asked to make configure installation consistent with 
CMake. Since I modeled implementation of Clang-format, I decided to fix problem 
in Clang-format make file too.

Just in case, possible improvements to configure (if it'll be around for awhile 
and somebody with more knowledge then me willing to do them):

* Make PROJ_sharedir in top-level Makefile.rules, since it's used for LLVM 
CMake modules and will be used for Clang-tidy.
* Since git-clang-format is Python script, it should be link from bin to share 
directory.

Repository:
  rL LLVM

http://reviews.llvm.org/D12919

Files:
  tools/clang-format/Makefile

Index: tools/clang-format/Makefile
===
--- tools/clang-format/Makefile
+++ tools/clang-format/Makefile
@@ -20,3 +20,36 @@
clangLex.a clangBasic.a 
 
 include $(CLANG_LEVEL)/Makefile
+
+PROJ_sharedir := $(DESTDIR)$(PROJ_prefix)/share/clang
+
+FILESLIST := $(notdir $(wildcard $(PROJ_SRC_DIR)/*.applescript))
+FILESLIST += $(notdir $(wildcard $(PROJ_SRC_DIR)/*.el))
+FILESLIST += $(notdir $(wildcard $(PROJ_SRC_DIR)/*.py))
+
+SRCFILES := $(addprefix $(PROJ_SRC_DIR)/, $(FILESLIST))
+DESTFILES := $(addprefix $(PROJ_sharedir)/, $(FILESLIST))
+
+$(PROJ_sharedir):
+   $(Echo) Making install directory: $@
+   $(Verb) $(MKDIR) $@
+
+$(DESTFILES): $(SRCFILES)
+
+$(PROJ_sharedir)/%.applescript: $(PROJ_SRC_DIR)/%.applescript
+   $(Echo) Installing script file: $(notdir $<)
+   $(Verb) $(DataInstall) $< $(PROJ_sharedir)
+
+$(PROJ_sharedir)/%.el: $(PROJ_SRC_DIR)/%.el
+   $(Echo) Installing script file: $(notdir $<)
+   $(Verb) $(DataInstall) $< $(PROJ_sharedir)
+
+$(PROJ_sharedir)/%.py: $(PROJ_SRC_DIR)/%.py
+   $(Echo) Installing script file: $(notdir $<)
+   $(Verb) $(ScriptInstall) $< $(PROJ_sharedir)
+
+$(PROJ_bindir)/git-clang-format: $(PROJ_SRC_DIR)/git-clang-format
+   $(Echo) Installing script file: $(notdir $<)
+   $(Verb) $(ScriptInstall) $< $(PROJ_bindir)
+
+install-local:: $(PROJ_sharedir) $(DESTFILES) $(PROJ_bindir)/git-clang-format


Index: tools/clang-format/Makefile
===
--- tools/clang-format/Makefile
+++ tools/clang-format/Makefile
@@ -20,3 +20,36 @@
clangLex.a clangBasic.a 
 
 include $(CLANG_LEVEL)/Makefile
+
+PROJ_sharedir := $(DESTDIR)$(PROJ_prefix)/share/clang
+
+FILESLIST := $(notdir $(wildcard $(PROJ_SRC_DIR)/*.applescript))
+FILESLIST += $(notdir $(wildcard $(PROJ_SRC_DIR)/*.el))
+FILESLIST += $(notdir $(wildcard $(PROJ_SRC_DIR)/*.py))
+
+SRCFILES := $(addprefix $(PROJ_SRC_DIR)/, $(FILESLIST))
+DESTFILES := $(addprefix $(PROJ_sharedir)/, $(FILESLIST))
+
+$(PROJ_sharedir):
+	$(Echo) Making install directory: $@
+	$(Verb) $(MKDIR) $@
+
+$(DESTFILES): $(SRCFILES)
+
+$(PROJ_sharedir)/%.applescript: $(PROJ_SRC_DIR)/%.applescript
+	$(Echo) Installing script file: $(notdir $<)
+	$(Verb) $(DataInstall) $< $(PROJ_sharedir)
+
+$(PROJ_sharedir)/%.el: $(PROJ_SRC_DIR)/%.el
+	$(Echo) Installing script file: $(notdir $<)
+	$(Verb) $(DataInstall) $< $(PROJ_sharedir)
+
+$(PROJ_sharedir)/%.py: $(PROJ_SRC_DIR)/%.py
+	$(Echo) Installing script file: $(notdir $<)
+	$(Verb) $(ScriptInstall) $< $(PROJ_sharedir)
+
+$(PROJ_bindir)/git-clang-format: $(PROJ_SRC_DIR)/git-clang-format
+	$(Echo) Installing script file: $(notdir $<)
+	$(Verb) $(ScriptInstall) $< $(PROJ_bindir)
+
+install-local:: $(PROJ_sharedir) $(DESTFILES) $(PROJ_bindir)/git-clang-format
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

On Wed, Sep 16, 2015 at 5:27 PM, Nico Weber via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> On Tue, Sep 15, 2015 at 5:50 PM, Richard Smith 
> wrote:
>
>> On Tue, Sep 15, 2015 at 12:38 PM, Nico Weber  wrote:
>>
>>> With this patch, we warn on `bool a : 4;`, yet we don't warn on `bool b`
>>> (which has 8 bits storage, 1 bit value). Warning on `bool b` is silly of
>>> course, but why is warning on `bool a : 4` useful? That's like 50% more
>>> storage efficient than `bool b` ;-)
>>>
>>> It's possible that this is a good warning for some reason, but I don't
>>> quite see why yet.
>>>
>>
>> Why would we warn on "unsigned n : 57;"? The bit-field is wider than
>> necessary, and we have no idea what the programmer was trying to do
>>
>
> Warning on this kind of makes sense to me, as the field is wider than the
> default width of int. (Not warning on that doesn't seem terrible to me
> either though.)
>
> I'm only confused about the bool case with bitfield sizes < 8 I think. We
> warn that the bitfield is wider than the value size, even though it's
> smaller than the default storage size, and we don't warn on regular bools.
>
> To get an idea how often this warning fires, I ran it on a large-ish open
> source codebase I had flying around. The only place it fired on is one
> header in protobuf (extension_set.h). I looked at the history of that file,
> and it had a struct that used to look like
>
>   struct Extension {
> SomeEnum e;
> bool a;
> bool b;
> bool c;
> int d;
> // ...some more stuff...
>   };
>
> Someone then added another field to this and for some reason decided to do
> it like so:
>
>   struct Extension {
> SomeEnum e;
> bool a;
> bool b1 : 4;
> bool b2 : 4;
> bool c;
> int d;
> // ...some more stuff...
>   };
>
> Neither the commit message nor the review discussion mention the bitfield
> at all as far as I can tell. Now, given that this isn't a small struct and
> it has a bunch of normal bools, I don't know why they added the new field
> as bitfield while this wasn't deemed necessary for the existing bools. My
> best guess is that that they didn't want to add 3 bytes of padding (due to
> the int field), which seems like a decent reason.
>
> Had the warning been in place when this code got written, I suppose they
> had used ": 1" instead. Does this make this code much better? It doesn't
> seem like it to me. So after doing a warning quality eval, I'd suggest to
> not emit the warning for bool bitfields if the bitfield size is < 8. (But
> since the warning fires only very rarely, I don't feel very strongly about
> this.)
>

I agree it doesn't make the code /much/ better. But if I were reading that,
I would certainly pause for a few moments wondering what the author was
thinking. I also don't feel especially strongly about this, but I don't see
a good rationale for warning on 'bool : 9' but not on 'bool : 5'.

, but it doesn't seem likely they got that effect. Would you be more
>> convinced if we amended the diagnostic to provide a fixit suggesting using
>> an anonymous bit-field to insert padding?
>>
>
> Isn't the Right Fix (tm) to make bool bitfields 1 wide and rely on the
> compiler to figure out padding?
>

It depends; maybe the intent is to be compatible with some on-disk format,
and the explicit padding is important:

struct X {
  int n : 3;
  bool b : 3;
  int n : 2;
};

Changing the bool bit-field to 1 bit without inserting an anonymous
bit-field would change the struct layout.


> On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith 
>>> wrote:
>>>
 On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik 
 wrote:

> As of DR262, the C standard clarified that the width of a bit-field
> can not exceed that of the specified type, and this change was primarily 
> to
> ensure that Clang correctly enforced this part of the standard. Looking at
> the C++11 standard again, it states that although the specified width of a
> bit-field may exceed the number of bits in the *object representation* 
> (which
> includes padding bits) of the specified type, the extra bits will not take
> any part in the bit-field's *value representation*.
>
> Taking this into account, it seems that the correct way to validate
> the width of a bit-field (ignoring the special case of MS in C mode) would
> be to use getIntWidth in C mode, and getTypeSize in C++ mode.
>
> I would be happy create a patch to make this change tomorrow if people
> are in agreement.
>
 David Majnemer has already landed a couple of changes to fix this up,
 so hopefully that won't be necessary. Thanks for working on this!

> Rachel
>
>
> [image: Inactive hide details for Nico Weber ---09/14/2015 09:53:25
> PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith  Weber ---09/14/2015 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard
> Smith  wrote:
>
> From: Nico Weber 
> To: Richard 

Re: r247618 - C11 _Bool bitfield diagnostic

[Nico Weber via cfe-commits]

On Tue, Sep 15, 2015 at 5:50 PM, Richard Smith 
wrote:

> On Tue, Sep 15, 2015 at 12:38 PM, Nico Weber  wrote:
>
>> With this patch, we warn on `bool a : 4;`, yet we don't warn on `bool b`
>> (which has 8 bits storage, 1 bit value). Warning on `bool b` is silly of
>> course, but why is warning on `bool a : 4` useful? That's like 50% more
>> storage efficient than `bool b` ;-)
>>
>> It's possible that this is a good warning for some reason, but I don't
>> quite see why yet.
>>
>
> Why would we warn on "unsigned n : 57;"? The bit-field is wider than
> necessary, and we have no idea what the programmer was trying to do
>

Warning on this kind of makes sense to me, as the field is wider than the
default width of int. (Not warning on that doesn't seem terrible to me
either though.)

I'm only confused about the bool case with bitfield sizes < 8 I think. We
warn that the bitfield is wider than the value size, even though it's
smaller than the default storage size, and we don't warn on regular bools.

To get an idea how often this warning fires, I ran it on a large-ish open
source codebase I had flying around. The only place it fired on is one
header in protobuf (extension_set.h). I looked at the history of that file,
and it had a struct that used to look like

  struct Extension {
SomeEnum e;
bool a;
bool b;
bool c;
int d;
// ...some more stuff...
  };

Someone then added another field to this and for some reason decided to do
it like so:

  struct Extension {
SomeEnum e;
bool a;
bool b1 : 4;
bool b2 : 4;
bool c;
int d;
// ...some more stuff...
  };

Neither the commit message nor the review discussion mention the bitfield
at all as far as I can tell. Now, given that this isn't a small struct and
it has a bunch of normal bools, I don't know why they added the new field
as bitfield while this wasn't deemed necessary for the existing bools. My
best guess is that that they didn't want to add 3 bytes of padding (due to
the int field), which seems like a decent reason.

Had the warning been in place when this code got written, I suppose they
had used ": 1" instead. Does this make this code much better? It doesn't
seem like it to me. So after doing a warning quality eval, I'd suggest to
not emit the warning for bool bitfields if the bitfield size is < 8. (But
since the warning fires only very rarely, I don't feel very strongly about
this.)

, but it doesn't seem likely they got that effect. Would you be more
> convinced if we amended the diagnostic to provide a fixit suggesting using
> an anonymous bit-field to insert padding?
>

Isn't the Right Fix (tm) to make bool bitfields 1 wide and rely on the
compiler to figure out padding?


>
> On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith 
>> wrote:
>>
>>> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik  wrote:
>>>
 As of DR262, the C standard clarified that the width of a bit-field can
 not exceed that of the specified type, and this change was primarily to
 ensure that Clang correctly enforced this part of the standard. Looking at
 the C++11 standard again, it states that although the specified width of a
 bit-field may exceed the number of bits in the *object representation* 
 (which
 includes padding bits) of the specified type, the extra bits will not take
 any part in the bit-field's *value representation*.

 Taking this into account, it seems that the correct way to validate the
 width of a bit-field (ignoring the special case of MS in C mode) would be
 to use getIntWidth in C mode, and getTypeSize in C++ mode.

 I would be happy create a patch to make this change tomorrow if people
 are in agreement.

>>> David Majnemer has already landed a couple of changes to fix this up, so
>>> hopefully that won't be necessary. Thanks for working on this!
>>>
 Rachel


 [image: Inactive hide details for Nico Weber ---09/14/2015 09:53:25
 PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith >>> Weber ---09/14/2015 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard
 Smith  wrote:

 From: Nico Weber 
 To: Richard Smith 
 Cc: Rachel Craik/Toronto/IBM@IBMCA, cfe-commits <
 cfe-commits@lists.llvm.org>
 Date: 09/14/2015 09:53 PM
 Subject: Re: r247618 - C11 _Bool bitfield diagnostic
 Sent by: tha...@google.com
 --



 On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <*rich...@metafoo.co.uk*
 > wrote:

On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
*cfe-commits@lists.llvm.org* > wrote:
   This also fires for bool in C++ files, even though the commit
   message saying C11 and _Bool. Given the test changes, I suppose 
 that's
   intentional? This fires a lot on existing code, for example protobuf:

   
 ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
   error: wi

Re: [PATCH] D10370: clang-format: Implement AlwaysBreakAfterDeclarationReturnType.

[strager via cfe-commits]

strager updated this revision to Diff 34947.
strager added a comment.

Rebase.


http://reviews.llvm.org/D10370

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -4658,6 +4658,44 @@
"  \"c\";");
 }
 
+TEST_F(FormatTest, DeclarationReturnTypeBreakingStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_TopLevel;
+  verifyFormat("class C {\n"
+   "  int f();\n"
+   "};\n"
+   "int\n"
+   "f();",
+   Style);
+  Style.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_All;
+  verifyFormat("class C {\n"
+   "  int\n"
+   "  f();\n"
+   "};\n"
+   "int\n"
+   "f();",
+   Style);
+  verifyFormat("const char *f(void) { return \"\"; }\n"
+   "const char *\n"
+   "bar(void);\n",
+   Style);
+  verifyFormat("template  T *f(T &c) { return NULL; }\n"
+   "template \n"
+   "T *\n"
+   "f(T &c);\n",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Stroustrup;
+  verifyFormat("const char *f(void) { return \"\"; }\n"
+   "const char *\n"
+   "bar(void);\n",
+   Style);
+  verifyFormat("template  T *f(T &c) { return NULL; }\n"
+   "template \n"
+   "T *\n"
+   "f(T &c);\n",
+   Style);
+}
+
 TEST_F(FormatTest, DefinitionReturnTypeBreakingStyle) {
   FormatStyle Style = getLLVMStyle();
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_TopLevel;
@@ -4738,6 +4776,46 @@
Style);
 }
 
+TEST_F(FormatTest, AlwaysBreakAfterDeclarationAndDefinitionReturnTypeMixed) {
+  FormatStyle AfterType = getLLVMStyle();
+  AfterType.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_None;
+  AfterType.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  verifyFormat("void f(void) {\n" // No break here.
+   "  f();\n"
+   "  f();\n"
+   "}\n"
+   "void bar(void);\n", // No break here.
+   AfterType);
+  AfterType.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_All;
+  AfterType.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  verifyFormat("void f(void) {\n" // No break here.
+   "  f();\n"
+   "  f();\n"
+   "}\n"
+   "void\n"
+   "bar(void);\n", // Break here.
+   AfterType);
+  AfterType.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_None;
+  AfterType.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
+  verifyFormat("void\n"
+   "f(void) {\n" // Break here.
+   "  f();\n"
+   "  f();\n"
+   "}\n"
+   "void bar(void);\n", // No break here.
+   AfterType);
+  AfterType.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_All;
+  AfterType.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
+  verifyFormat("void\n"
+   "f(void) {\n" // Break here.
+   "  f();\n"
+   "  f();\n"
+   "}\n"
+   "void\n"
+   "bar(void);\n", // Break here.
+   AfterType);
+}
+
 TEST_F(FormatTest, AlwaysBreakBeforeMultilineStrings) {
   FormatStyle NoBreak = getLLVMStyle();
   NoBreak.AlwaysBreakBeforeMultilineStrings = false;
@@ -9409,6 +9487,15 @@
   CHECK_PARSE("BreakBeforeBraces: GNU", BreakBeforeBraces, FormatStyle::BS_GNU);
   CHECK_PARSE("BreakBeforeBraces: WebKit", BreakBeforeBraces, FormatStyle::BS_WebKit);
 
+  Style.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_All;
+  CHECK_PARSE("AlwaysBreakAfterDeclarationReturnType: None",
+  AlwaysBreakAfterDeclarationReturnType, FormatStyle::DRTBS_None);
+  CHECK_PARSE("AlwaysBreakAfterDeclarationReturnType: All",
+  AlwaysBreakAfterDeclarationReturnType, FormatStyle::DRTBS_All);
+  CHECK_PARSE("AlwaysBreakAfterDeclarationReturnType: TopLevel",
+  AlwaysBreakAfterDeclarationReturnType,
+  FormatStyle::DRTBS_TopLevel);
+
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
   CHECK_PARSE("AlwaysBreakAfterDefinitionReturnType: None",
   AlwaysBreakAfterDefinitionReturnType, FormatStyle::DRTBS_None);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1593,15 +1593,18 @@
 

[PATCH] D12917: [CUDA] Allow parsing of host and device code simultaneously.

[Artem Belevich via cfe-commits]

tra created this revision.
tra added reviewers: echristo, eliben, jpienaar.
tra added a subscriber: cfe-commits.
Herald added subscribers: dschuff, jfb.

The patch:
  - adds -aux-triple option to specify target triple
  - propagates aux target info to AST context and Preprocessor
  - pulls in target specific preprocessor macros.
  - pulls in target-specific builtins from aux target.
  - sets appropriate __host__ or __device__ attribute on builtins.

Rationale:

  In order to compile CUDA source with mixed host/device code without 
physically separating host and device code, we need to be able to parse code 
that may contain target-specific constructs from both host and device targets. 

During device-side compilation we need to be able to include any host includes 
we may encounter. Similarly, during host compilation, we do need to include 
device-side includes that may be required for device-specific code in the file 
to parse. In both cases, we need to fake target environment well enough for the 
headers to work (I.e. x86 host's headers want to see __amd64__ or __i386__ 
defined, CUDA includes are looking for NVPTX-specific macros). 

We also need to be able to parse target-specific builtins from both host and 
device targets in the same TU.

Generally speaking it's not possible to achieve this in all cases. Fortunately, 
CUDA's case is simpler and proposed patch works pretty well in practice:

  - clang already implements attribute-based function overloading which allows 
avoiding name clashes between host and device functions.
  - basic host and device types are expected to match, so a lot of type-related 
predefined macros from host and device targets have the same value.
  - host includes (x86 on linux) do not use predefined NVPTX-specific macros, 
so including them is not a problem.
  - builtins from the aux target are only used for parsing and AST 
construction. CUDA never generates IR for them. If a builtin is used from a 
wrong context, it will violate calling restriction and produce an error. 
  - this change includes *all* builtins from the aux target which provides us 
with a superset of builtins that would be available on the opposite side of the 
compilation which will allow creating different ASTs on host and device sides. 
IMO it's similar to already-existing issue of diverging host/device code caused 
by common (ab)use of __CUDA_ARCH__ macro.



http://reviews.llvm.org/D12917

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/Builtins.h
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CompilerInstance.h
  include/clang/Frontend/FrontendOptions.h
  include/clang/Lex/Preprocessor.h
  lib/AST/ASTContext.cpp
  lib/Basic/Builtins.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/InitPreprocessor.cpp
  lib/Lex/Preprocessor.cpp
  lib/Sema/SemaDecl.cpp
  test/SemaCUDA/builtins.cu

Index: test/SemaCUDA/builtins.cu
===
--- test/SemaCUDA/builtins.cu
+++ test/SemaCUDA/builtins.cu
@@ -1,36 +1,31 @@
-// Tests that target-specific builtins have appropriate host/device
-// attributes and that CUDA call restrictions are enforced. Also
-// verify that non-target builtins can be used from both host and
-// device functions.
+// Tests that host and target builtins can be used in the same TU,
+// have appropriate host/device attributes and that CUDA call
+// restrictions are enforced. Also verify that non-target builtins can
+// be used from both host and device functions.
 //
 // REQUIRES: x86-registered-target
 // REQUIRES: nvptx-registered-target
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown \
+// RUN: -aux-triple nvptx64-unknown-cuda \
 // RUN: -fcuda-target-overloads -fsyntax-only -verify %s
 // RUN: %clang_cc1 -triple nvptx64-unknown-cuda -fcuda-is-device \
+// RUN: -aux-triple x86_64-unknown-unknown \
 // RUN: -fcuda-target-overloads -fsyntax-only -verify %s
 
+#if !(defined(__amd64__) && defined(__PTX__))
+#error "Expected to see preprocessor macros from both sides of compilation."
+#endif
 
-#ifdef __CUDA_ARCH__
-// Device-side builtins are not allowed to be called from host functions.
 void hf() {
-  int x = __builtin_ptx_read_tid_x(); // expected-note  {{'__builtin_ptx_read_tid_x' declared here}}
+  int x = __builtin_ia32_rdtsc();
+  int y = __builtin_ptx_read_tid_x(); // expected-note  {{'__builtin_ptx_read_tid_x' declared here}}
   // expected-error@-1 {{reference to __device__ function '__builtin_ptx_read_tid_x' in __host__ function}}
   x = __builtin_abs(1);
 }
+
 __attribute__((device)) void df() {
   int x = __builtin_ptx_read_tid_x();
+  int y = __builtin_ia32_rdtsc(); // expected-error {{reference to __host__ function '__builtin_ia32_rdtsc' in __device__ function}}
+  // expected-note@20 {{'__builtin_ia32_rdtsc' declared here}}
   x = __builtin_abs(1);
 }
-#else
-// Host-side built

Re: [PATCH] D11693: clang-format: Support generalized lambda captures.

[strager via cfe-commits]

strager updated this revision to Diff 34946.
strager marked 2 inline comments as done.
strager added a comment.

Address @djasper's comments.


http://reviews.llvm.org/D11693

Files:
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10057,6 +10057,18 @@
   // More complex introducers.
   verifyFormat("return [i, args...] {};");
 
+  // Lambdas with generalized captures.
+  verifyFormat("auto f = [b = d]() {};\n");
+  verifyFormat("auto f = [b = std::move(d)]() {};\n");
+  verifyFormat("auto f = [b = c, d = e, g]() {};\n");
+  verifyFormat("auto f = [b = a[3]]() {};\n");
+  verifyFormat("auto f = [InRange = a < b && b < c]() {};\n");
+  verifyFormat("auto f = [b = std::vector{1, 2, 3}]() {};\n");
+  verifyFormat("auto f = [b = std::vector{\n"
+   "  1, 2, 3,\n"
+   "}]() {};\n");
+  verifyFormat("return [b = std::vector()] {}();\n");
+
   // Not lambdas.
   verifyFormat("constexpr char hello[]{\"hello\"};");
   verifyFormat("double &operator[](int i) { return 0; }\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1054,6 +1054,27 @@
 nextToken();
 if (FormatTok->is(tok::ellipsis))
   nextToken();
+if (FormatTok->is(tok::equal)) {
+  // Generalized lambda capture.
+  nextToken();
+  while (!eof()) {
+// FIXME: Once we have an expression parser in the UnwrappedLineParser,
+// replace this by using parseAssigmentExpression() inside. See also
+// parseBracedList. For now, parsing matching braces ([], (), {}) is
+// good enough.
+if (FormatTok->is(tok::l_paren)) {
+  parseParens();
+} else if (FormatTok->is(tok::l_square)) {
+  parseSquare();
+} else if (FormatTok->is(tok::l_brace)) {
+  parseBracedList(false);
+} else if (FormatTok->isOneOf(tok::comma, tok::r_square)) {
+  break;
+} else {
+  nextToken();
+}
+  }
+}
 if (FormatTok->is(tok::comma)) {
   nextToken();
 } else if (FormatTok->is(tok::r_square)) {
@@ -1110,7 +1131,8 @@
   nextToken();
 
   // FIXME: Once we have an expression parser in the UnwrappedLineParser,
-  // replace this by using parseAssigmentExpression() inside.
+  // replace this by using parseAssigmentExpression() inside. See also
+  // tryToParseLambdaIntroducer.
   do {
 if (Style.Language == FormatStyle::LK_JavaScript) {
   if (FormatTok->is(Keywords.kw_function)) {


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10057,6 +10057,18 @@
   // More complex introducers.
   verifyFormat("return [i, args...] {};");
 
+  // Lambdas with generalized captures.
+  verifyFormat("auto f = [b = d]() {};\n");
+  verifyFormat("auto f = [b = std::move(d)]() {};\n");
+  verifyFormat("auto f = [b = c, d = e, g]() {};\n");
+  verifyFormat("auto f = [b = a[3]]() {};\n");
+  verifyFormat("auto f = [InRange = a < b && b < c]() {};\n");
+  verifyFormat("auto f = [b = std::vector{1, 2, 3}]() {};\n");
+  verifyFormat("auto f = [b = std::vector{\n"
+   "  1, 2, 3,\n"
+   "}]() {};\n");
+  verifyFormat("return [b = std::vector()] {}();\n");
+
   // Not lambdas.
   verifyFormat("constexpr char hello[]{\"hello\"};");
   verifyFormat("double &operator[](int i) { return 0; }\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1054,6 +1054,27 @@
 nextToken();
 if (FormatTok->is(tok::ellipsis))
   nextToken();
+if (FormatTok->is(tok::equal)) {
+  // Generalized lambda capture.
+  nextToken();
+  while (!eof()) {
+// FIXME: Once we have an expression parser in the UnwrappedLineParser,
+// replace this by using parseAssigmentExpression() inside. See also
+// parseBracedList. For now, parsing matching braces ([], (), {}) is
+// good enough.
+if (FormatTok->is(tok::l_paren)) {
+  parseParens();
+} else if (FormatTok->is(tok::l_square)) {
+  parseSquare();
+} else if (FormatTok->is(tok::l_brace)) {
+  parseBracedList(false);
+} else if (FormatTok->isOneOf(tok::comma, tok::r_square)) {
+  break;
+} else {
+  nextToken();
+}
+  }
+}
 if (FormatTok->is(tok::comma)) {
   nextToken();
 } else if (FormatTok->is(tok::r_square)) {
@@ -1110,7 +1131,8 @@
   nextToken();
 
   // FIXME: Once we have an expressi

Re: [PATCH] D11693: clang-format: Support generalized lambda captures.

[strager via cfe-commits]

strager marked 4 inline comments as done.


Comment at: lib/Format/UnwrappedLineParser.cpp:1057-1058
@@ +1056,4 @@
+  while (!eof()) {
+// FIXME: Once we have an expression parser in the UnwrappedLineParser,
+// replace this by using parseAssigmentExpression() inside.
+if (FormatTok->is(tok::l_paren)) {

strager wrote:
> djasper wrote:
> > I very much doubt that we'll have an Expression parser here anytime soon. 
> > So, I don't think that this FIXME makes much sense. Instead, please provide 
> > a comment on what this is actually doing and in which cases it might fail.
> I copied the comment from elsewhere in the file.
I expanded the comment, including a reference to the other reference to 
`parseAssigmentExpression`.


Comment at: lib/Format/UnwrappedLineParser.cpp:1061
@@ +1060,3 @@
+  parseParens();
+} else if (FormatTok->isOneOf(tok::comma, tok::r_square)) {
+  break;

strager wrote:
> djasper wrote:
> > I think this list should be extended to figure out certain cases where we 
> > know something is fishy. In particular:
> > * If you find an l_square or less, call into parseSquare and parseAngle 
> > respectively.
> > * If you find an r_brace or semi, something is wrong, break.
> > 
> Will do.
Handling r_brace and semi is a bit weird, since we end up aborting mid-stream 
and what's left becomes unparsable/incomplete by clang-format.

parseAngle doesn't exist, and even if it did, the less-than operator wouldn't 
be handled properly.

I added l_square and l_brace support.


http://reviews.llvm.org/D11693



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r247871 - Updating docs for MSan to describe poison-in-dtor.

[Naomi Musgrave via cfe-commits]

Author: nmusgrave
Date: Wed Sep 16 19:10:59 2015
New Revision: 247871

URL: http://llvm.org/viewvc/llvm-project?rev=247871&view=rev
Log:
Updating docs for MSan to describe poison-in-dtor.

Summary:
Describe the compile and runtime flags to enable MemorySanitizer
detection of use-after-destroy.

Reviewers: eugenis

Subscribers: llvm-commits

Differential Revision: http://reviews.llvm.org/D12914

Revise doc description of use-after-dtor.

Change wording to specify memory no longer readable.

Modified:
cfe/trunk/docs/UsersManual.rst

Modified: cfe/trunk/docs/UsersManual.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=247871&r1=247870&r2=247871&view=diff
==
--- cfe/trunk/docs/UsersManual.rst (original)
+++ cfe/trunk/docs/UsersManual.rst Wed Sep 16 19:10:59 2015
@@ -1065,6 +1065,16 @@ are listed below.
   order of memory stores the uninitialized value went
   through. This mode may use extra memory in programs that copy
   uninitialized memory a lot.
+   -  ``-fsanitize-memory-use-after-dtor``: Enables use-after-destruction
+  detection in MemorySanitizer. After invocation of the destructor,
+  the object is considered no longer readable. Facilitates the
+  detection of use-after-destroy bugs.
+
+  Setting the MSAN_OPTIONS=poison_in_dtor=1 enables the poisoning of
+  memory at runtime. Any subsequent access to the destroyed object
+  fails at runtime. This feature is still experimental, but this
+  environment variable must be set to 1 in order for the above flag
+  to have any effect.
 
The ``-fsanitize=`` argument must also be provided when linking, in
order to link to the appropriate runtime library. When using


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive

[Sean Silva via cfe-commits]

silvas requested changes to this revision.
silvas added a reviewer: silvas.
This revision now requires changes to proceed.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7774
@@ -7771,1 +7773,3 @@
   "@import of module '%0' in implementation of '%1'; use #import">;
+def note_module_need_top_level : Note<"consider moving the import to top 
level">;
+def note_module_need_textual : Note<"consider marking the header as textual">;

rsmith wrote:
> We'll already have said something like:
> 
>   error: import of module 'foo' appears within class X
>   note: class X begins here
> 
> There are two likely situations here: either the header was intended to be 
> included textually, or the user forgot a `}` or similar and didn't expect for 
> the import to be inside the class. Either way, this note is not useful 
> (either they thought it *was* at the top level, or the note is not 
> applicable).
> 
> How about dropping this note? The "begins here" note seems to have already 
> covered the interesting case.
I think this makes sense. `error: ... import ... appears within ...` seems like 
sufficient information about the nature of the error.


Comment at: lib/Sema/SemaDecl.cpp:14372
@@ +14371,3 @@
+ diag::note_module_import_not_at_top_level) << DC;
+  S.Diag(ImportLoc, diag::note_module_need_textual);
+}

rsmith wrote:
> I think we should only suggest changing the module map if our current module 
> and the module of the included file are within the same top-level module. It 
> would be unfortunate for us to suggest modifying, say, the libc module map if 
> we see some end user's code including a C library header in the wrong context.
> 
> You can check `M->getTopLevelModuleName()` against 
> `S.getLangOpts().CurrentModule` and `S.getLangOpts().ImplementationOfModule` 
> to see whether it's a submodule of the current module.
From my experience, your suggested heuristic is insufficient; we definitely 
need to cover the case that crosses top-level modules (or is from a .cpp file 
to a module it imports); actually, that is the only case that I have run into 
in practice.

In this patch, I think the right thing is to just not emit 
`note_module_need_top_level` nor `note_module_need_textual`. A separate patch 
can use a flag `-Wintial-modules-migration` (or whatever) to emit these notes a 
bit more aggressively (the flag would be off by default).


Comment at: test/Modules/misplaced.cpp:1-12
@@ +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I 
%S/Inputs %s -verify
+
+namespace N1 {  // expected-note{{namespace 'N1' begins here}}
+#include "dummy.h"  // expected-error{{import of module 'dummy' appears within 
namespace 'N1'}} \
+// expected-note{{consider moving the import to top level}}
+}
+
+void func1() {  // expected-note{{function 'func1' begins here}}
+#include "dummy.h"  // expected-error{{import of module 'dummy' appears within 
function 'func1'}} \
+// expected-note{{consider marking the header as textual}}
+}

This test does not cover the `appears within class` case.


http://reviews.llvm.org/D11844



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12379: Fix the bugs in the mapDiagnosticRanges (Still in progress)

[Zhengkai Wu via cfe-commits]

zhengkai added inline comments.


Comment at: lib/Frontend/DiagnosticRenderer.cpp:312
@@ +311,3 @@
+
+static bool retrieveBeginLocation(SourceLocation &Begin,
+ FileID &BeginFileID,

rtrieu wrote:
> Why not return a SourceLocation instead of passing one in by reference?
Because this function has two recursive calls inside.
So if return a SourceLocation here, I will need to use a temporal variable to 
store the result of the first call.
And this doesn't make it easier.


Comment at: lib/Frontend/DiagnosticRenderer.cpp:323
@@ +322,3 @@
+  else
+Backup = SM->getImmediateSpellingLoc(Backup);
+  BeginFileID = SM->getFileID(Begin);

rtrieu wrote:
> In the other function, there is "End = 
> SM->getImmediateExpansionRange(Backup).second;"  Why is there not a "Begin = 
> SM->getImmediateExpansionRange(Backup).first;" here?
  Begin = SM->getImmediateMacroCallerLoc(Begin);
Use getImmediateMacroCallerLoc function here.


Comment at: lib/Frontend/DiagnosticRenderer.cpp:331
@@ +330,3 @@
+
+static bool retrieveEndLocation(SourceLocation &End,
+FileID &EndFileID,

rtrieu wrote:
> There is a lot of duplicated code between these two functions.  Can they 
> merged together with an option to select whether to check the start or end 
> locations?
I tried it but the code doesn't seem easier.


Comment at: lib/Frontend/DiagnosticRenderer.cpp:352-367
@@ -309,1 +351,18 @@
+
+/// Helper function to print out all the backtrace locations 
+/// for a source location.
+
+static void retrieveAllBacktraces(SourceLocation Loc,
+  const SourceManager *SM) {
+  llvm::errs() << "New level\n";
+  llvm::errs() << Loc.printToString(*SM) << " " << 
+  SM->getFileID(Loc).getHashValue() << "\n";
+  if (!Loc.isMacroID()) return;
+  if (SM->isMacroArgExpansion(Loc)) llvm::errs() << "is Macro Arg Expansion\n";
+  llvm::errs() << "Down Spelling Loc\n";
+  retrieveAllBacktraces(SM->getImmediateSpellingLoc(Loc),SM);
+  llvm::errs() << "Down Expansion Range\n";
+  retrieveAllBacktraces(SM->getImmediateExpansionRange(Loc).first,SM);
+}
+
 // Helper function to fix up source ranges.  It takes in an array of ranges,

rtrieu wrote:
> Function not used; remove it.
deleted. But I wonder this function is important for those who want to debug 
this code.


Comment at: lib/Frontend/DiagnosticRenderer.cpp:417-418
@@ -374,1 +416,4 @@
+// Do the backtracking.
+if (!retrieveBeginLocation(Begin,BeginFileID,CaretLocFileID,SM)) continue;
+if (!retrieveEndLocation(End,EndFileID,CaretLocFileID,SM)) continue;
 

rtrieu wrote:
> Can these function be rewritten to return SourceLocation's?  Then it would be:
> Begin = retrieveBeginLocation(Begin, BeginFileID, CaretLocFileID, SM);
> End = retrieveEndLocation(End, EndFileID, CaretLocFileID, SM);
> 
> if (Begin.invalid() || End.invalid()) continue;
> 
Replied before.


Comment at: lib/Frontend/DiagnosticRenderer.cpp:466
@@ -419,1 +465,3 @@
 
+static bool checkLocForMacroArgExpansion(SourceLocation Loc,
+ const SourceManager &SM,

rtrieu wrote:
> What is this function doing that is not captured by the old 
> isMacroArgExpansion function?
I don't understand what you mean here.


Comment at: lib/Frontend/DiagnosticRenderer.cpp:504
@@ -441,2 +503,3 @@
 
-  if (!SM.isMacroArgExpansion(Loc))
+  /// Count all valid ranges.
+  unsigned ValidCount = 0;

rtrieu wrote:
> Why does the range count matter?
Because some ranges passed in are invalid.
And if the mapDiagnosticRanges function drops some ranges, this means this 
macro call doesn't contain all the information needed of all the ranges.


Comment at: test/Misc/caret-diags-macros.c:210
@@ -215,3 +209,3 @@
 void f(char* pMsgBuf, char* pKeepBuf) {
-Csprintf(pMsgBuf,"\nEnter minimum anagram length (2-%1d): ", 
Cstrlen(pKeepBuf));
+Csprintf(pMsgBuf,"\nEnter minimum anagram length (2-%1d): ", 
strlen_test(pKeepBuf));
 }

rtrieu wrote:
> Why is Cstrlen changed to strlen_test?
The preprocessor can't get right when the argument itself is a function-like 
macro. This bug is not due to the high-level macro case but due to this problem.


http://reviews.llvm.org/D12379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12916: [Static Analyzer] Use generics related information to infer dynamic types.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, jordan_rose.
xazax.hun added a subscriber: cfe-commits.

This patch makes the DynamicTypePropagation checker utilize the information 
about generics. Using this additional information more precise inlining can be 
done. It also fixes an XFAIL test.

The same stored information will be used by a separate general type checker. 
That checker will make it possible to get rid of the isReturnValueMisused 
function.

http://reviews.llvm.org/D12916

Files:
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  test/Analysis/DynamicTypePropagation.m

Index: test/Analysis/DynamicTypePropagation.m
===
--- test/Analysis/DynamicTypePropagation.m
+++ test/Analysis/DynamicTypePropagation.m
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -analyze 
-analyzer-checker=core,alpha.osx.cocoa.ObjCGenerics -verify %s
-// XFAIL: *
 
 #if !__has_feature(objc_generics)
 #  error Compiler does not support Objective-C generics?
Index: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
===
--- lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
+++ lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
@@ -665,38 +665,36 @@
 /// Get the returned ObjCObjectPointerType by a method based on the tracked 
type
 /// information, or null pointer when the returned type is not an
 /// ObjCObjectPointerType.
-static const ObjCObjectPointerType *getReturnTypeForMethod(
+static QualType getReturnTypeForMethod(
 const ObjCMethodDecl *Method, ArrayRef TypeArgs,
 const ObjCObjectPointerType *SelfType, ASTContext &C) {
   QualType StaticResultType = Method->getReturnType();
 
   // Is the return type declared as instance type?
   if (StaticResultType == C.getObjCInstanceType())
-return SelfType;
+return QualType(SelfType, 0);
 
   // Check whether the result type depends on a type parameter.
   if (!isObjCTypeParamDependent(StaticResultType))
-return nullptr;
+return QualType();
 
   QualType ResultType = StaticResultType.substObjCTypeArgs(
   C, TypeArgs, ObjCSubstitutionContext::Result);
 
-  return ResultType->getAs();
+  return ResultType;
 }
 
 /// Validate that the return type of a message expression is used correctly.
 /// Returns true in case an error is detected.
 bool DynamicTypePropagation::isReturnValueMisused(
 const ObjCMessageExpr *MessageExpr,
-const ObjCObjectPointerType *SeflType, SymbolRef Sym,
+const ObjCObjectPointerType *ResultPtrType, SymbolRef Sym,
 const ObjCMethodDecl *Method, ArrayRef TypeArgs,
 bool SubscriptOrProperty, CheckerContext &C) const {
-  ASTContext &ASTCtxt = C.getASTContext();
-  const auto *ResultPtrType =
-  getReturnTypeForMethod(Method, TypeArgs, SeflType, ASTCtxt);
   if (!ResultPtrType)
 return false;
 
+  ASTContext &ASTCtxt = C.getASTContext();
   const Stmt *Parent =
   C.getCurrentAnalysisDeclContext()->getParentMap().getParent(MessageExpr);
   if (SubscriptOrProperty) {
@@ -861,20 +859,34 @@
   if (!TypeArgs)
 return;
 
-  if (isReturnValueMisused(MessageExpr, *TrackedType, RecSym, Method, 
*TypeArgs,
-   M.getMessageKind() != OCM_Message, C))
+  QualType ResultType =
+  getReturnTypeForMethod(Method, *TypeArgs, *TrackedType, ASTCtxt);
+  // The static type is the same as the deduced type.
+  if (ResultType.isNull())
+return;
+
+  const MemRegion *RetRegion = M.getReturnValue().getAsRegion();
+  ExplodedNode *Pred = C.getPredecessor();
+  if (RetRegion) {
+State = setDynamicTypeInfo(State, RetRegion, ResultType,
+   /*CanBeSubclass=*/true);
+Pred = C.addTransition(State);
+  }
+
+  const auto *ResultPtrType = ResultType->getAs();
+
+  if (isReturnValueMisused(MessageExpr, ResultPtrType, RecSym, Method,
+   *TypeArgs, M.getMessageKind() != OCM_Message, C))
 return;
 
-  const auto *ResultPtrType =
-  getReturnTypeForMethod(Method, *TypeArgs, *TrackedType, ASTCtxt);
   if (!ResultPtrType || ResultPtrType->isUnspecialized())
 return;
 
   // When the result is a specialized type and it is not tracked yet, track it
   // for the result symbol.
   if (!State->get(RetSym)) {
 State = State->set(RetSym, ResultPtrType);
-C.addTransition(State);
+C.addTransition(State, Pred);
   }
 }
 


Index: test/Analysis/DynamicTypePropagation.m
===
--- test/Analysis/DynamicTypePropagation.m
+++ test/Analysis/DynamicTypePropagation.m
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.osx.cocoa.ObjCGenerics -verify %s
-// XFAIL: *
 
 #if !__has_feature(objc_generics)
 #  error Compiler does not support Objective-C generics?
Index: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
===
--- 

Re: [PATCH] D12379: Fix the bugs in the mapDiagnosticRanges (Still in progress)

[Zhengkai Wu via cfe-commits]

zhengkai updated this revision to Diff 34943.
zhengkai marked 8 inline comments as done.

http://reviews.llvm.org/D12379

Files:
  include/clang/Basic/SourceManager.h
  lib/Basic/SourceManager.cpp
  lib/Frontend/DiagnosticRenderer.cpp
  test/Index/fix-its.m
  test/Misc/caret-diags-macros.c
  test/Misc/diag-macro-backtrace2.c
  test/Misc/reduced-diags-macros.cpp
  test/Misc/serialized-diags.c

Index: test/Misc/serialized-diags.c
===
--- test/Misc/serialized-diags.c
+++ test/Misc/serialized-diags.c
@@ -55,7 +55,6 @@
 // CHECK: Range: {{.*[/\\]}}serialized-diags.c:22:3 {{.*[/\\]}}serialized-diags.c:22:6
 // CHECK: Range: {{.*[/\\]}}serialized-diags.c:22:13 {{.*[/\\]}}serialized-diags.c:22:18
 // CHECK: +-{{.*[/\\]}}serialized-diags.c:20:15: note: expanded from macro 'false' []
-// CHECK: +-Range: {{.*[/\\]}}serialized-diags.c:22:3 {{.*[/\\]}}serialized-diags.c:22:6
 // CHECK: +-Range: {{.*[/\\]}}serialized-diags.c:20:15 {{.*[/\\]}}serialized-diags.c:20:16
 // CHECK: +-{{.*[/\\]}}serialized-diags.c:19:1: note: 'taz' declared here []
 // CHECK: {{.*[/\\]}}serialized-diags.h:5:7: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
Index: test/Misc/reduced-diags-macros.cpp
===
--- test/Misc/reduced-diags-macros.cpp
+++ test/Misc/reduced-diags-macros.cpp
@@ -11,7 +11,7 @@
 // CHECK-NEXT: ~~^~
 // CHECK-NEXT: {{.*}}:3:34: note: expanded from macro 'NO_INITIATION'
 // CHECK-NEXT: #define NO_INITIATION(x) int a = x * 2
-// CHECK-NEXT:  ^
+// CHECK-NEXT:  ~   ^
 
 // CHECK: {{.*}}:7:15: error: use of undeclared identifier 'b'
 // CHECK-NEXT: NO_DEFINITION(b);
@@ -27,3 +27,18 @@
 // CHECK: {{.*}}:25:23: error: use of undeclared identifier 'x'
 // CHECK-NEXT: int  p = SWAP_ARGU(3, x);
 // CHECK-NEXT:   ^
+
+#define APPLY(f,x,y) x f y
+
+struct node {
+};
+
+node ff;
+
+int r = APPLY(+,ff,1);
+// CHECK: {{.*}}:38:15: error: invalid operands to binary expression ('node' and 'int')
+// CHECK-NEXT: int r = APPLY(+,ff,1);
+// CHECK-NEXT:   ^ ~~ ~
+// CHECK-NEXT: {{.*}}:31:24: note: expanded from macro 'APPLY'
+// CHECK-NEXT: #define APPLY(f,x,y) x f y
+// CHECK-NEXT:  ~ ^ ~
\ No newline at end of file
Index: test/Misc/diag-macro-backtrace2.c
===
--- test/Misc/diag-macro-backtrace2.c
+++ test/Misc/diag-macro-backtrace2.c
@@ -16,7 +16,7 @@
   // CHECK: :15:3: error: invalid operands to binary expression
   // CHECK:   ('const char *' and 'int')
   // CHECK:   a(str);
-  // CHECK:   ^ ~~~
+  // CHECK:   ^~
   // CHECK: :3:11: note: expanded from macro 'a'
   // CHECK: #define a b
   // CHECK:   ^
@@ -28,7 +28,7 @@
   // CHECK:  ^~~~
   // CHECK: :6:15: note: expanded from macro 'd'
   // CHECK: #define d(x) x*1
-  // CHECK:   ^~
+  // CHECK:  ~^~
 
   e(str);
   // CHECK: :33:5: warning: expression result unused
Index: test/Misc/caret-diags-macros.c
===
--- test/Misc/caret-diags-macros.c
+++ test/Misc/caret-diags-macros.c
@@ -16,9 +16,6 @@
 void bar() {
   C(1);
   // CHECK: {{.*}}:17:5: warning: expression result unused
-  // CHECK: {{.*}}:15:16: note: expanded from macro 'C'
-  // CHECK: {{.*}}:14:16: note: expanded from macro 'B'
-  // CHECK: {{.*}}:13:14: note: expanded from macro 'A'
 }
 
 // rdar://7597492
@@ -41,62 +38,59 @@
 
 void test() {
   macro_args3(11);
-  // CHECK: {{.*}}:43:15: warning: expression result unused
+  // CHECK: {{.*}}:40:15: warning: expression result unused
   // Also check that the 'caret' printing agrees with the location here where
   // its easy to FileCheck.
   // CHECK-NEXT:  macro_args3(11);
   // CHECK-NEXT: {{^  \^~}}
-  // CHECK: {{.*}}:36:36: note: expanded from macro 'macro_args3'
-  // CHECK: {{.*}}:35:36: note: expanded from macro 'macro_args2'
-  // CHECK: {{.*}}:34:24: note: expanded from macro 'macro_args1'
 
   macro_many_args3(
 1,
 2,
 3);
-  // CHECK: {{.*}}:55:5: warning: expression result unused
-  // CHECK: {{.*}}:40:55: note: expanded from macro 'macro_many_args3'
-  // CHECK: {{.*}}:39:55: note: expanded from macro 'macro_many_args2'
-  // CHECK: {{.*}}:38:35: note: expanded from macro 'macro_many_args1'
+  // CHECK: {{.*}}:49:5: warning: expression result unused
+  // CHECK: {{.*}}:37:55: note: expanded from macro 'macro_many_args3'
+  // CHECK: {{.*}}:36:55: note: expanded from macro 'macro_many_args2'
+  // CHECK: {{.*}}:35:35: note: expanded from macro 'macro_many_args1'
 
   macro_many_args3(
 1,
 M2,
 3);
-  // CHECK: {{.*}}:64:5: warning: expression result unused
+  // CHECK: {{.*}}:58:5: warning: expression result unused
   // C

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

LG, ship it.

On Wed, Sep 16, 2015 at 2:03 PM Aaron Ballman 
wrote:

> Attached is an updated patch for clang-tools-extra that does not have
> my in-progress, not-related-to-any-of-this code in it. ;-)
>
> ~Aaron
>
> On Wed, Sep 16, 2015 at 4:04 PM, Aaron Ballman 
> wrote:
> > Quick ping. I know this is a fairly gigantic patch, but I'm hoping for
> > a relatively quick review turnaround because of potential merge
> > conflicts with people doing a fair amount of work on clang-tidy
> > lately. Everything should be pretty straight-forward (it's all just
> > renames, no semantic changes intended aside from
> > recordDecl/cxxRecordDecl and the narrowing matchers.
> >
> > ~Aaron
> >
> > On Tue, Sep 15, 2015 at 1:32 PM, Aaron Ballman 
> wrote:
> >> Here are the complete patches to solve the issues we've discussed:
> >>
> >> 1) splits recordDecl into recordDecl and cxxRecordDecl
> >> 2) adds isStruct, isUnion, isClass to identify what kind of
> >> recordDecl() you may be looking at
> >> 3) prefixes all of the node matchers with cxx that should have it
> >> 4) fixes a similar issue with CUDAKernelCallExpr (the prefix needs to
> >> be cuda instead of CUDA to distinguish the matcher name from the type
> >> name).
> >> 5) updates all of the documentation and code that broke.
> >>
> >> One patch is for changes to clang, the other is for changes to
> >> clang-tools-extra.
> >>
> >> ~Aaron
> >>
> >> On Mon, Sep 14, 2015 at 5:49 PM, Aaron Ballman 
> wrote:
> >>> On Mon, Sep 14, 2015 at 5:47 PM, Daniel Jasper 
> wrote:
>  Btw, I think generating them, potentially into several different
> headers to
>  work around the compile time issue isn't such a bad idea.
> >>>
> >>> I'm not going to start with this approach, but think it may be worth
> >>> exploring at some point. ;-)
> >>>
> >>> ~Aaron
> >>>
> 
>  On Mon, Sep 14, 2015 at 11:45 PM, Manuel Klimek 
> wrote:
> >
> > Feel free to rename the AST nodes :)
> >
> >
> > On Mon, Sep 14, 2015, 2:44 PM Daniel Jasper 
> wrote:
> >>
> >> Ok. I am happy with this then.
> >>
> >> (Just personally grumpy having to write
> >> cxxRecordDecl(has(cxxConstructorDecl(..))) in the future ;-) ).
> >>
> >> On Mon, Sep 14, 2015 at 11:41 PM, Manuel Klimek 
> >> wrote:
> >>>
> >>>
> >>>
> >>> On Mon, Sep 14, 2015 at 2:29 PM Aaron Ballman <
> aa...@aaronballman.com>
> >>> wrote:
> 
>  On Mon, Sep 14, 2015 at 4:38 PM, Manuel Klimek  >
>  wrote:
>  >
>  >
>  > On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman
>  > 
>  > wrote:
>  >>
>  >> On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper <
> djas...@google.com>
>  >> wrote:
>  >> > By this point, I see that change might be profitable overall.
>  >> > However,
>  >> > lets
>  >> > completely map this out. Changing just cxxRecordDecl() can
>  >> > actually
>  >> > increase
>  >> > confusion in other areas. Right now, not a single AST
> matcher has
>  >> > the
>  >> > cxx
>  >> > prefix (although a total of 28 stand for the corresponding
> CXX..
>  >> > AST
>  >> > node).
>  >> > This is consistent and people knowing this will never try to
> write
>  >> > cxxConstructExpr(). As soon as people have used
> cxxRecordDecl(),
>  >> > the
>  >> > chance
>  >> > of them trying cxxConstructExpr() increases. You have spent
> a long
>  >> > time
>  >> > figuring out that recordDecl means cxxRecordDecl(), which is
> one
>  >> > datapoint,
>  >> > but I am not aware of anyone else having this specific
> issue. And
>  >> > we
>  >> > could
>  >> > make this less bad with better documentation, I think.
>  >> >
>  >> > So, for me, the questions are:
>  >> > 1) Do we want/need this change?
>  >>
>  >> We definitely need *a* change because there currently is no
> way to
>  >> match a C struct or union when compiling in C mode. I
> discovered
>  >> this
>  >> because I was trying to write a new checker for clang-tidy that
>  >> focuses on C code and it would fail to match when compiling in
> C
>  >> mode.
>  >> Whether we decide to go with cxxRecordDecl vs recordDecl vs
>  >> structDecl
>  >> (etc) is less important to me than the ability to write
> clang-tidy
>  >> checks for C code.
>  >>
>  >> > 2) Do we want to be consistent and change the other 27
> matchers as
>  >> > well?
>  >>
>  >> I'm on the fence about this for all the reasons you point out.
>  >>
>  >> > A fundamental question is whether we want AST matchers to
> match
>  >> > AST
>  >> > nodes
>  >> > 1:1 or whether they should be an abstraction from some
> >>

Re: [PATCH] D7714: missing-namespace-std check for clang-tidy

[Jonathan B Coe via cfe-commits]

jbcoe abandoned this revision.
jbcoe added a comment.

This work needs such extensive revision that I'll submit a new version when I 
start work on it again.


Repository:
  rL LLVM

http://reviews.llvm.org/D7714



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[Devin Coughlin via cfe-commits]

> On Sep 16, 2015, at 4:01 PM, NAKAMURA Takumi  wrote:
> 
> Did you forget to update examples/analyzer-plugin? Fixed in r247862.

Yes — thank you!

Devin

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[NAKAMURA Takumi via cfe-commits]

chapuni added a subscriber: chapuni.
chapuni added a comment.

Did you forget to update examples/analyzer-plugin? Fixed in r247862.


Repository:
  rL LLVM

http://reviews.llvm.org/D12780



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r247862 - analyzer-plugin/MainCallChecker.cpp: s/generateSink/generateErrorNode/, corresponding to r247859.

[NAKAMURA Takumi via cfe-commits]

Author: chapuni
Date: Wed Sep 16 17:57:23 2015
New Revision: 247862

URL: http://llvm.org/viewvc/llvm-project?rev=247862&view=rev
Log:
analyzer-plugin/MainCallChecker.cpp: s/generateSink/generateErrorNode/, 
corresponding to r247859.

Modified:
cfe/trunk/examples/analyzer-plugin/MainCallChecker.cpp

Modified: cfe/trunk/examples/analyzer-plugin/MainCallChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/examples/analyzer-plugin/MainCallChecker.cpp?rev=247862&r1=247861&r2=247862&view=diff
==
--- cfe/trunk/examples/analyzer-plugin/MainCallChecker.cpp (original)
+++ cfe/trunk/examples/analyzer-plugin/MainCallChecker.cpp Wed Sep 16 17:57:23 
2015
@@ -30,7 +30,7 @@ void MainCallChecker::checkPreStmt(const
 return;
 
   if (II->isStr("main")) {
-ExplodedNode *N = C.generateSink();
+ExplodedNode *N = C.generateErrorNode();
 if (!N)
   return;
 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12871: [OpenMP] Target directive host codegen - rebased

[Samuel Antao via cfe-commits]

sfantao added inline comments.


Comment at: lib/CodeGen/CGStmtOpenMP.cpp:121-123
@@ -103,4 +120,5 @@
+ArgLVal.getAddress(), ArgLVal.getType()->castAs());
   auto *ExprArg =
   EmitLoadOfLValue(ArgLVal, SourceLocation()).getScalarVal();
   auto VAT = FD->getCapturedVLAType();
   VLASizeMap[VAT->getSizeExpr()] = ExprArg;

I am actually doing `ArgLVal.getType()` instead  `FD->getType()`. FD doesn't 
have a reference type here.


http://reviews.llvm.org/D12871



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] __attribute__((enable_if)) and non-overloaded member functions

[George Burgess via cfe-commits]

Sure. :)  Review is based off the attachment I grabbed from here:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150824/136904.html

A few nits:
- test/Sema/enable_if.cpp line 24: Please use __attribute__(( instead of
__attribute((
- Can we have a similar test for a function that returns an Incomplete?

George

On Wed, Sep 16, 2015 at 1:30 PM, Nick Lewycky  wrote:

> +gbiv, would you be able to review this?
> On Sep 14, 2015 7:42 AM, "Ettore Speziale" 
> wrote:
>
>> Ping
>>
>> > Gently ping.
>> >
>> >> On Aug 26, 2015, at 2:40 PM, Ettore Speziale <
>> speziale.ett...@gmail.com> wrote:
>> >>
>> >> Forward to the right ML:
>> >>
>>  Sorry about the extreme delay. This patch slipped through the
>> cracks, and I only noticed it again when searching my email for enable_if.
>> Committed in r245985! In the future, please feel free to continue pinging
>> weekly!
>> >>>
>> >>> NP, thank you for committing the patch.
>> >>>
>> >>> Unfortunately it contains a little error in the case of no candidate
>> has been found. For instance consider the following test case:
>> >>>
>> >>> struct Incomplete;
>> >>>
>> >>> struct X {
>> >>> void hidden_by_argument_conversion(Incomplete n, int m = 0)
>> __attribute((enable_if(m == 10, "chosen when 'm' is ten")));
>> >>> };
>> >>>
>> >>> x.hidden_by_argument_conversion(10);
>> >>>
>> >>> I would expect to get an error about Incomplete, as the compiler
>> cannot understand how to convert 10 into an instance of Incomplete. However
>> right now the enable_if diagnostic is emitted, thus masking the more useful
>> message about Incomplete.
>> >>>
>> >>> The attached patch solved the problem by delaying the point where the
>> enable_if diagnostic is issued.
>> >>>
>> >>> Thanks,
>> >>> Ettore Speziale
>> >>
>> >>
>> >> 
>> >
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12871: [OpenMP] Target directive host codegen - rebased

[Samuel Antao via cfe-commits]

sfantao updated this revision to Diff 34936.
sfantao added a comment.

Fix imprecision in previous diff.


http://reviews.llvm.org/D12871

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGOpenMPRuntime.h
  lib/CodeGen/CGStmtOpenMP.cpp
  lib/CodeGen/CodeGenFunction.h
  test/OpenMP/target_codegen.cpp

Index: test/OpenMP/target_codegen.cpp
===
--- /dev/null
+++ test/OpenMP/target_codegen.cpp
@@ -0,0 +1,606 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple i386-unknown-unknown -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple i386-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// expected-no-diagnostics
+#ifndef HEADER
+#define HEADER
+
+// CHECK-DAG: [[TT:%.+]] = type { i64, i8 }
+// CHECK-DAG: [[S1:%.+]] = type { double }
+
+// We have 8 target regions, but only 7 that actually will generate offloading
+// code, only 6 will have mapped arguments, and only 4 have all-constant map
+// sizes.
+
+// CHECK-DAG: [[SIZET2:@.+]] = private unnamed_addr constant [1 x i{{32|64}}] [i[[SZ:32|64]] 2]
+// CHECK-DAG: [[MAPT2:@.+]] = private unnamed_addr constant [1 x i32] [i32 3]
+// CHECK-DAG: [[SIZET3:@.+]] = private unnamed_addr constant [2 x i[[SZ]]] [i[[SZ]] 4, i[[SZ]] 2]
+// CHECK-DAG: [[MAPT3:@.+]] = private unnamed_addr constant [2 x i32] [i32 3, i32 3]
+// CHECK-DAG: [[MAPT4:@.+]] = private unnamed_addr constant [9 x i32] [i32 3, i32 3, i32 1, i32 3, i32 3, i32 1, i32 1, i32 3, i32 3]
+// CHECK-DAG: [[SIZET5:@.+]] = private unnamed_addr constant [3 x i[[SZ]]] [i[[SZ]] 4, i[[SZ]] 2, i[[SZ]] 40]
+// CHECK-DAG: [[MAPT5:@.+]] = private unnamed_addr constant [3 x i32] [i32 3, i32 3, i32 3]
+// CHECK-DAG: [[SIZET6:@.+]] = private unnamed_addr constant [4 x i[[SZ]]] [i[[SZ]] 4, i[[SZ]] 2, i[[SZ]] 1, i[[SZ]] 40]
+// CHECK-DAG: [[MAPT6:@.+]] = private unnamed_addr constant [4 x i32] [i32 3, i32 3, i32 3, i32 3]
+// CHECK-DAG: [[MAPT7:@.+]] = private unnamed_addr constant [5 x i32] [i32 3, i32 3, i32 1, i32 1, i32 3]
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+
+template
+struct TT{
+  tx X;
+  ty Y;
+};
+
+// CHECK: define {{.*}}[[FOO:@.+]](
+int foo(int n) {
+  int a = 0;
+  short aa = 0;
+  float b[10];
+  float bn[n];
+  double c[5][10];
+  double cn[5][n];
+  TT d;
+
+  // CHECK:   br label %[[TRY:[^,]+]]
+  // CHECK:   [[TRY]]
+  // CHECK:   [[RET:%.+]] = call i32 @__tgt_target(i32 -1, i8* @{{[^,]+}}, i32 0, i8** null, i8** null, i[[SZ]]* null, i32* null)
+  // CHECK-NEXT:  [[ERROR:%.+]] = icmp ne i32 [[RET]], 0
+  // CHECK-NEXT:  br i1 [[ERROR]], label %[[FAIL:[^,]+]], label %[[END:[^,]+]]
+  // CHECK:   [[FAIL]]
+  // CHECK:   call void [[HVT0:@.+]]()
+  // CHECK-NEXT:  br label %[[END]]
+  // CHECK:   [[END]]
+  #pragma omp target
+  {
+  }
+
+  // CHECK:   call void [[HVT1:@.+]](i32* {{[^,]+}})
+  #pragma omp target if(0)
+  {
+a += 1;
+  }
+
+  // CHECK:   br label %[[TRY:[^,]+]]
+  // CHECK:   [[TRY]]
+  // CHECK-DAG:   [[RET:%.+]] = call i32 @__tgt_target(i32 -1, i8* @{{[^,]+}}, i32 1, i8** [[BP:%[^,]+]], i8** [[P:%[^,]+]], i[[SZ]]* getelementptr inbounds ([1 x i[[SZ]]], [1 x i[[SZ]]]* [[SIZET2]], i32 0, i32 0), i32* getelementptr inbounds ([1 x i32], [1 x i32]* [[MAPT2]], i32 0, i32 0))
+  // CHECK-DAG:   [[BP]] = getelementptr inbounds [1 x i8*], [1 x i8*]* [[BPR:%[^,]+]], i32 0, i32 0
+  // CHECK-DAG:   [[P]] = getelementptr inbounds [1 x i8*], [1 x i8*]* [[PR:%[^,]+]], i32 0, i32 0
+  // CHECK-DAG:   [[BPADDR0:%.+]] = getelementptr inbounds [1 x i8*], [1 x i8*]* [[BPR]], i32 0, i32 [[IDX0:[0-9]+]]
+  // CHECK-DAG:   [[PADDR0:%.+]] = getelementptr inbounds [1 x i8*], [1 x i8*]* [[PR]], i32 0, i32 [[IDX0]]
+  // CHECK-DAG:   store i8* [[BP0:%[^,]+]], i8** [[BPADDR0]]
+  // CHECK-DAG:   store i8* [[P0:%[^,]+]], i8** [[PADDR0]]
+  // CHECK-DAG:   [[BP0]] = bitcast i16* %{{.+}} to i8*
+  // CHECK-DAG:   [[P0]] = bitcast i16* %{{.+}} to i8*
+
+  // CHECK:   [[ERROR:%.+]] = icmp ne i32 [[RET]], 0
+  // CHECK-NEXT:  br i1 [[ERROR]], label %[[FAIL:[^,]+]], label %[[END:[^,]+]]
+  // CHECK:   [[FAIL]]
+  // CHECK:   call void [[HVT2:@.+]](i16* {{[^,]+}})
+  // CHECK-NEXT:  br label %[[END]]
+  // CHECK

Re: [PATCH] D11279: Initial patch for PS4 toolchain

[Eric Christopher via cfe-commits]

echristo added a comment.

In http://reviews.llvm.org/D11279#247402, @kromanova wrote:

> Hi Eric,
>  I don't mind changing .keep into README. However, originally we did it for 
> consistency purpose. If you grep for .keep in 
> /llvm/tools/clang/test/Driver/Inputs directory, you will find hundreds of 
> instances of these files that were created for similar reasons (checking SDK 
> layout), but you won't find a single README file there. Do you still want us 
> to make this change?


Huh. I guess not.

(Well, I mean yes, but if I want that done I can do it myself and keep 
consistency in this commit).

-eric


http://reviews.llvm.org/D11279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12871: [OpenMP] Target directive host codegen - rebased

[Samuel Antao via cfe-commits]

sfantao added inline comments.


Comment at: lib/CodeGen/CGStmtOpenMP.cpp:80-81
@@ -67,1 +79,4 @@
   II = &getContext().Idents.get("vla");
+  if (UseOnlyReferences)
+ArgType = getContext().getLValueReferenceType(
+ArgType, /*SpelledAsLValue=*/false);

Done!


http://reviews.llvm.org/D12871



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12871: [OpenMP] Target directive host codegen - rebased

[Samuel Antao via cfe-commits]

sfantao added inline comments.


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3044-3054
@@ +3043,13 @@
+
+  if (auto *VAT = dyn_cast(ElementType.getTypePtr())) {
+auto VATInfo = CGF.getVLASize(VAT);
+Size = llvm::ConstantInt::get(
+CGM.SizeTy,
+CGM.getContext().getTypeSizeInChars(VATInfo.second).getQuantity());
+Size = CGF.Builder.CreateNUWMul(Size, VATInfo.first);
+  } else {
+uint64_t ElementTypeSize =
+CGM.getContext().getTypeSizeInChars(ElementType).getQuantity();
+Size = llvm::ConstantInt::get(CGM.SizeTy, ElementTypeSize);
+  }
+

ABataev wrote:
> Use `getTypeSize(CGF, ElementType)` instead
In some previous review of this patch there was a suggestion to change from 
`getTypeSize(CGF, ElementType)/8` to 
`getTypeSizeInChars(ElementType).getQuantity()`. Just double checking if you 
really want me to revert that.


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3082-3128
@@ +3081,49 @@
+
+  // Generate the code to launch the target region. The pattern is the
+  // following:
+  //
+  //   ...
+  //   br IfCond (if any), omp_offload, omp_offload_fail
+  //
+  // omp_offload.try:
+  //   ; create arrays for offloading
+  //   error = __tgt_target(...)
+  //   br error, omp_offload_fail, omp_offload_end
+  //
+  // omp_offload.fail:
+  //   host_version(...)
+  //
+  // omp_offload.end:
+  //   ...
+  //
+
+  auto OffloadTryBlock = CGF.createBasicBlock("omp_offload.try");
+  auto OffloadFailBlock = CGF.createBasicBlock("omp_offload.fail");
+  auto ContBlock = CGF.createBasicBlock("omp_offload.end");
+
+  if (IfCond)
+CGF.EmitBranchOnBoolExpr(IfCond, OffloadTryBlock, OffloadFailBlock,
+ /*TrueCount=*/0);
+
+  CGF.EmitBlock(OffloadTryBlock);
+
+  unsigned PointerNumVal = BasePointers.size();
+  llvm::Value *PointerNum = CGF.Builder.getInt32(PointerNumVal);
+  llvm::Value *BasePointersArray;
+  llvm::Value *PointersArray;
+  llvm::Value *SizesArray;
+  llvm::Value *MapTypesArray;
+
+  if (PointerNumVal) {
+llvm::APInt PointerNumAP(32, PointerNumVal, /*isSigned=*/true);
+QualType PointerArrayType = CGF.getContext().getConstantArrayType(
+CGF.getContext().VoidPtrTy, PointerNumAP, ArrayType::Normal,
+/*IndexTypeQuals=*/0);
+
+BasePointersArray =
+CGF.CreateMemTemp(PointerArrayType, ".offload_baseptrs").getPointer();
+PointersArray =
+CGF.CreateMemTemp(PointerArrayType, ".offload_ptrs").getPointer();
+
+// If we don't have any VLA types, we can use a constant array for the map
+// sizes, otherwise we need to fill up the arrays as we do for the 
pointers.

ABataev wrote:
> Could you use `emitOMPIfClause()` function instead of this long block?
I don't think I can reuse the current implementation. The reason is that the 
"else" basic block is always emitted and the "if" basic block needs to have a 
branch to the "else" basic block, and all these basic blocks are only visible 
inside `emitOMPIfClause()`. Basically, the if clause codegen would have to be 
combined with the testing of the return code of the offloading call.


Comment at: lib/CodeGen/CGStmtOpenMP.cpp:38
@@ +37,3 @@
+  if (UseOnlyReferences) {
+LValue LV = MakeNaturalAlignAddrLValue(
+CreateMemTemp(CurField->getType(), "__vla_size_ref").getPointer(),

ABataev wrote:
> Use `MakeAddrLValue(CreateMemTemp(CurField->getType(), "__vla_size_ref"), 
> CurField->getType())` instead.
Done!


Comment at: lib/CodeGen/CGStmtOpenMP.cpp:117-122
@@ -102,4 +116,8 @@
 if (FD->hasCapturedVLAType()) {
   auto *ExprArg =
   EmitLoadOfLValue(ArgLVal, SourceLocation()).getScalarVal();
+  if (UseOnlyReferences) {
+auto ExprArgRef = MakeNaturalAlignAddrLValue(ExprArg, FD->getType());
+ExprArg = EmitLoadOfLValue(ExprArgRef, 
SourceLocation()).getScalarVal();
+  }
   auto VAT = FD->getCapturedVLAType();

ABataev wrote:
> ```
> if (UseOnlyReferences)
>   ArgLVal = CGF.EmitLoadOfReferenceLValue(ArgLVal.getAddress(), 
> FD->getType()->casAs());
> auto *ExprArg = EmitLoadOfLValue(ArgLVal, SourceLocation()).getScalarVal();
> ```
Done!


http://reviews.llvm.org/D12871



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11279: Initial patch for PS4 toolchain

[Katya Romanova via cfe-commits]

kromanova added a comment.

Hi Eric,
I don't mind changing .keep into README. However, originally we did it for 
consistency purpose. If you grep for .keep in 
/llvm/tools/clang/test/Driver/Inputs directory, you will find hundreds of 
instances of these files that were created for similar reasons (checking SDK 
layout), but you won't find a single README file there. Do you still want us to 
make this change?


http://reviews.llvm.org/D11279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12871: [OpenMP] Target directive host codegen - rebased

[Samuel Antao via cfe-commits]

sfantao updated this revision to Diff 34933.
sfantao added a comment.

Minor changes to address Alexey's remarks.


http://reviews.llvm.org/D12871

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGOpenMPRuntime.h
  lib/CodeGen/CGStmtOpenMP.cpp
  lib/CodeGen/CodeGenFunction.h
  test/OpenMP/target_codegen.cpp

Index: test/OpenMP/target_codegen.cpp
===
--- /dev/null
+++ test/OpenMP/target_codegen.cpp
@@ -0,0 +1,606 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple i386-unknown-unknown -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple i386-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// expected-no-diagnostics
+#ifndef HEADER
+#define HEADER
+
+// CHECK-DAG: [[TT:%.+]] = type { i64, i8 }
+// CHECK-DAG: [[S1:%.+]] = type { double }
+
+// We have 8 target regions, but only 7 that actually will generate offloading
+// code, only 6 will have mapped arguments, and only 4 have all-constant map
+// sizes.
+
+// CHECK-DAG: [[SIZET2:@.+]] = private unnamed_addr constant [1 x i{{32|64}}] [i[[SZ:32|64]] 2]
+// CHECK-DAG: [[MAPT2:@.+]] = private unnamed_addr constant [1 x i32] [i32 3]
+// CHECK-DAG: [[SIZET3:@.+]] = private unnamed_addr constant [2 x i[[SZ]]] [i[[SZ]] 4, i[[SZ]] 2]
+// CHECK-DAG: [[MAPT3:@.+]] = private unnamed_addr constant [2 x i32] [i32 3, i32 3]
+// CHECK-DAG: [[MAPT4:@.+]] = private unnamed_addr constant [9 x i32] [i32 3, i32 3, i32 1, i32 3, i32 3, i32 1, i32 1, i32 3, i32 3]
+// CHECK-DAG: [[SIZET5:@.+]] = private unnamed_addr constant [3 x i[[SZ]]] [i[[SZ]] 4, i[[SZ]] 2, i[[SZ]] 40]
+// CHECK-DAG: [[MAPT5:@.+]] = private unnamed_addr constant [3 x i32] [i32 3, i32 3, i32 3]
+// CHECK-DAG: [[SIZET6:@.+]] = private unnamed_addr constant [4 x i[[SZ]]] [i[[SZ]] 4, i[[SZ]] 2, i[[SZ]] 1, i[[SZ]] 40]
+// CHECK-DAG: [[MAPT6:@.+]] = private unnamed_addr constant [4 x i32] [i32 3, i32 3, i32 3, i32 3]
+// CHECK-DAG: [[MAPT7:@.+]] = private unnamed_addr constant [5 x i32] [i32 3, i32 3, i32 1, i32 1, i32 3]
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+// CHECK-DAG: @{{.*}} = private constant i8 0
+
+template
+struct TT{
+  tx X;
+  ty Y;
+};
+
+// CHECK: define {{.*}}[[FOO:@.+]](
+int foo(int n) {
+  int a = 0;
+  short aa = 0;
+  float b[10];
+  float bn[n];
+  double c[5][10];
+  double cn[5][n];
+  TT d;
+
+  // CHECK:   br label %[[TRY:[^,]+]]
+  // CHECK:   [[TRY]]
+  // CHECK:   [[RET:%.+]] = call i32 @__tgt_target(i32 -1, i8* @{{[^,]+}}, i32 0, i8** null, i8** null, i[[SZ]]* null, i32* null)
+  // CHECK-NEXT:  [[ERROR:%.+]] = icmp ne i32 [[RET]], 0
+  // CHECK-NEXT:  br i1 [[ERROR]], label %[[FAIL:[^,]+]], label %[[END:[^,]+]]
+  // CHECK:   [[FAIL]]
+  // CHECK:   call void [[HVT0:@.+]]()
+  // CHECK-NEXT:  br label %[[END]]
+  // CHECK:   [[END]]
+  #pragma omp target
+  {
+  }
+
+  // CHECK:   call void [[HVT1:@.+]](i32* {{[^,]+}})
+  #pragma omp target if(0)
+  {
+a += 1;
+  }
+
+  // CHECK:   br label %[[TRY:[^,]+]]
+  // CHECK:   [[TRY]]
+  // CHECK-DAG:   [[RET:%.+]] = call i32 @__tgt_target(i32 -1, i8* @{{[^,]+}}, i32 1, i8** [[BP:%[^,]+]], i8** [[P:%[^,]+]], i[[SZ]]* getelementptr inbounds ([1 x i[[SZ]]], [1 x i[[SZ]]]* [[SIZET2]], i32 0, i32 0), i32* getelementptr inbounds ([1 x i32], [1 x i32]* [[MAPT2]], i32 0, i32 0))
+  // CHECK-DAG:   [[BP]] = getelementptr inbounds [1 x i8*], [1 x i8*]* [[BPR:%[^,]+]], i32 0, i32 0
+  // CHECK-DAG:   [[P]] = getelementptr inbounds [1 x i8*], [1 x i8*]* [[PR:%[^,]+]], i32 0, i32 0
+  // CHECK-DAG:   [[BPADDR0:%.+]] = getelementptr inbounds [1 x i8*], [1 x i8*]* [[BPR]], i32 0, i32 [[IDX0:[0-9]+]]
+  // CHECK-DAG:   [[PADDR0:%.+]] = getelementptr inbounds [1 x i8*], [1 x i8*]* [[PR]], i32 0, i32 [[IDX0]]
+  // CHECK-DAG:   store i8* [[BP0:%[^,]+]], i8** [[BPADDR0]]
+  // CHECK-DAG:   store i8* [[P0:%[^,]+]], i8** [[PADDR0]]
+  // CHECK-DAG:   [[BP0]] = bitcast i16* %{{.+}} to i8*
+  // CHECK-DAG:   [[P0]] = bitcast i16* %{{.+}} to i8*
+
+  // CHECK:   [[ERROR:%.+]] = icmp ne i32 [[RET]], 0
+  // CHECK-NEXT:  br i1 [[ERROR]], label %[[FAIL:[^,]+]], label %[[END:[^,]+]]
+  // CHECK:   [[FAIL]]
+  // CHECK:   call void [[HVT2:@.+]](i16* {{[^,]+}})
+  // CHECK-NEXT:  br label %[[END]]
+ 

Re: [PATCH] D11279: Initial patch for PS4 toolchain

[Eric Christopher via cfe-commits]

echristo added a comment.

In http://reviews.llvm.org/D11279#247377, @kromanova wrote:

> test/Driver/Inputs/scei-ps4_tree/target/include_common/ and  
> test/Driver/Inputs/scei-ps4_tree/target/include/ are expected to be present 
> by a test ps4-header-search.c. This test checks that these directories are 
> found in the header search path. If these directories are not found, the 
> compiler complains and the test fails.
>  .keep file is needed to keep these directories in place.


How about just a README file in the directories saying this rather than the 
.keep file? :)

-eric



Comment at: lib/Driver/Tools.cpp:10025-10029
@@ +10024,7 @@
+  const char *Exec =
+#ifdef LLVM_ON_WIN32
+  Args.MakeArgString(ToolChain.GetProgramPath("ps4-ld.gold.exe"));
+#else
+  Args.MakeArgString(ToolChain.GetProgramPath("ps4-ld"));
+#endif
+

kromanova wrote:
> echristo wrote:
> > How does this work with the things below that check for linker name?
> > 
> > Also seems like we'd really want to be able to check our host using the 
> > normal ways rather than an ifdef. This can be done as a followup, but do 
> > please check into this.
> We will check for the host without the ifdef, but we will do it in a 
> follow-up commit.
> 
> 
> 
That's fine, thanks.


http://reviews.llvm.org/D11279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[Devin Coughlin via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL247859: [analyzer] Add generateErrorNode() APIs to 
CheckerContext. (authored by dcoughlin).

Changed prior to commit:
  http://reviews.llvm.org/D12780?vs=34917&id=34930#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D12780

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/TaintTesterChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
  cfe/trunk/test/Analysis/PR24184.cpp
  cfe/trunk/test/Analysis/malloc.c

Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -3224,6 +3224,11 @@
 
 void BugReporter::emitReport(std::unique_ptr R) {
   if (const ExplodedNode *E = R->getErrorNode()) {
+// An error node must either be a sink or have a tag, otherwise
+// it could get reclaimed before the path diagnostic is created.
+assert((E->isSink() || E->getLocation().getTag()) &&
+"Error node must either be a sink or have a tag");
+
 const AnalysisDeclContext *DeclCtx =
 E->getLocationContext()->getAnalysisDeclContext();
 // The source of autosynthesized body can be handcrafted AST or a model
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
@@ -51,7 +51,7 @@
   if (isa(LR) || isa(LR) ||
   isa(LR)) {
 
-if (ExplodedNode *N = C.addTransition()) {
+if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
   if (!BT)
 BT.reset(
 new BuiltinBug(this, "Dangerous pointer arithmetic",
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1592,7 +1592,7 @@
   if (!CheckKind.hasValue())
 return;
 
-  if (ExplodedNode *N = C.generateSink()) {
+  if (ExplodedNode *N = C.generateErrorNode()) {
 if (

r247859 - [analyzer] Add generateErrorNode() APIs to CheckerContext.

[Devin Coughlin via cfe-commits]

Author: dcoughlin
Date: Wed Sep 16 17:03:05 2015
New Revision: 247859

URL: http://llvm.org/viewvc/llvm-project?rev=247859&view=rev
Log:
[analyzer] Add generateErrorNode() APIs to CheckerContext.

The analyzer trims unnecessary nodes from the exploded graph before reporting
path diagnostics. However, in some cases it can trim all nodes (including the
error node), leading to an assertion failure (see
https://llvm.org/bugs/show_bug.cgi?id=24184).

This commit addresses the issue by adding two new APIs to CheckerContext to
explicitly create error nodes. Unless the client provides a custom tag, these
APIs tag the node with the checker's tag -- preventing it from being trimmed.
The generateErrorNode() method creates a sink error node, while
generateNonFatalErrorNode() creates an error node for a path that should
continue being explored.

The intent is that one of these two methods should be used whenever a checker
creates an error node.

This commit updates the checkers to use these APIs. These APIs
(unlike addTransition() and generateSink()) do not take an explicit Pred node.
This is because there are not any error nodes in the checkers that were created
with an explicit different than the default (the CheckerContext's Pred node).

It also changes generateSink() to require state and pred nodes (previously
these were optional) to reduce confusion.

Additionally, there were several cases where checkers did check whether a
generated node could be null; we now explicitly check for null in these places.

This commit also includes a test case written by Ying Yi as part of
http://reviews.llvm.org/D12163 (that patch originally addressed this issue but
was reverted because it introduced false positive regressions).

Differential Revision: http://reviews.llvm.org/D12780

Added:
cfe/trunk/test/Analysis/PR24184.cpp
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/TaintTesterChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
cfe/trunk/test/Analysis/malloc.c

Modified: 
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h?

Re: [PATCH] D11279: Initial patch for PS4 toolchain

[Katya Romanova via cfe-commits]

kromanova added a comment.

test/Driver/Inputs/scei-ps4_tree/target/include_common/ and  
test/Driver/Inputs/scei-ps4_tree/target/include/ are expected to be present by 
a test ps4-header-search.c. This test checks that these directories are found 
in the header search path. If these directories are not found, the compiler 
complains and the test fails.
.keep file is needed to keep these directories in place.



Comment at: lib/Driver/Tools.cpp:10025-10029
@@ +10024,7 @@
+  const char *Exec =
+#ifdef LLVM_ON_WIN32
+  Args.MakeArgString(ToolChain.GetProgramPath("ps4-ld.gold.exe"));
+#else
+  Args.MakeArgString(ToolChain.GetProgramPath("ps4-ld"));
+#endif
+

echristo wrote:
> How does this work with the things below that check for linker name?
> 
> Also seems like we'd really want to be able to check our host using the 
> normal ways rather than an ifdef. This can be done as a followup, but do 
> please check into this.
We will check for the host without the ifdef, but we will do it in a follow-up 
commit.





http://reviews.llvm.org/D11279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

[Aaron Ballman via cfe-commits]

On Wed, Sep 16, 2015 at 5:33 PM, Alexander Kornienko  wrote:
> It was about a hundred in a huge codebase. It's definitely manageable, but
> the experiment has shown that this kind of a mistake is not likely to
> happen.

Fair enough, let's axe it until we see evidence it may catch real bugs.

~Aaron

>
> On 16 Sep 2015 23:25, "Aaron Ballman"  wrote:
>>
>> On Wed, Sep 16, 2015 at 5:21 PM, Alexander Kornienko 
>> wrote:
>> > All found results were intended usages of sizeof on containers. 100%
>> > false
>> > positive rate that is.
>>
>> Yes, but is that 4 results in 10MM LoC, or 4000 results in 40k LoC?
>> ;-) I guess I just don't have a good feel for how large the codebase
>> is, and how many times it resulted in matching sane code. If it's
>> really low (like 4 out of 10MM LoC), I think the checker may possibly
>> be useful (just not in that code base). If it's anything remotely
>> high, then I would agree, let's ditch it and not look back.
>>
>> ~Aaron
>>
>> >
>> > On 16 Sep 2015 21:23, "Aaron Ballman"  wrote:
>> >>
>> >> On Wed, Sep 16, 2015 at 2:21 PM, Alexander Kornienko
>> >> 
>> >> wrote:
>> >> > An update: I didn't find a single bug with this check in a large
>> >> > codebase.
>> >> > Turns out that it's rather useless. I'm inclined to kill it.
>> >>
>> >> How bad is the false positive rate?
>> >>
>> >> ~Aaron
>> >>
>> >> >
>> >> >
>> >> > On Sun, Sep 13, 2015 at 11:22 AM, Alexander Kornienko
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> I've also found a bunch of similar cases in our codebase, and I'm
>> >> >> trying
>> >> >> to figure out whether the pattern can be narrowed down to just
>> >> >> dangerous
>> >> >> cases. If we don't find a way to do so, we'll probably have to
>> >> >> resort
>> >> >> to "//
>> >> >> NOLINT" to shut clang-tidy up.
>> >> >>
>> >> >> On 13 Sep 2015 10:52, "Kim Gräsman"  wrote:
>> >> >>>
>> >> >>> Late to the party, but I wanted to ask: is there a way to indicate
>> >> >>> to
>> >> >>> the checker that we really *did* mean sizeof()?
>> >> >>>
>> >> >>> I think I've stumbled over code in our code base that uses
>> >> >>> sizeof(container) to report memory usage statistics and it seems
>> >> >>> valid, so it'd be nice if this checker could be silenced on a
>> >> >>> case-by-case basis.
>> >> >>>
>> >> >>> Thanks,
>> >> >>> - Kim
>> >> >>>
>> >> >>> On Sat, Sep 12, 2015 at 12:09 AM, Alexander Kornienko via
>> >> >>> cfe-commits
>> >> >>>  wrote:
>> >> >>> > Indeed. But this has been fixed before I could get to it.
>> >> >>> >
>> >> >>> >
>> >> >>> > On Thu, Sep 10, 2015 at 10:47 PM, Aaron Ballman via cfe-commits
>> >> >>> >  wrote:
>> >> >>> >>
>> >> >>> >> aaron.ballman added a comment.
>> >> >>> >>
>> >> >>> >> This appears to have broken one of the bots:
>> >> >>> >>
>> >> >>> >> http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/15065
>> >> >>> >>
>> >> >>> >>
>> >> >>> >> http://reviews.llvm.org/D12759
>> >> >>> >>
>> >> >>> >>
>> >> >>> >>
>> >> >>> >> ___
>> >> >>> >> cfe-commits mailing list
>> >> >>> >> cfe-commits@lists.llvm.org
>> >> >>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >> >>> >
>> >> >>> >
>> >> >>> > ___
>> >> >>> > cfe-commits mailing list
>> >> >>> > cfe-commits@lists.llvm.org
>> >> >>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >> >>> >
>> >> >
>> >> >
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

[Alexander Kornienko via cfe-commits]

It was about a hundred in a huge codebase. It's definitely manageable, but
the experiment has shown that this kind of a mistake is not likely to
happen.
On 16 Sep 2015 23:25, "Aaron Ballman"  wrote:

> On Wed, Sep 16, 2015 at 5:21 PM, Alexander Kornienko 
> wrote:
> > All found results were intended usages of sizeof on containers. 100%
> false
> > positive rate that is.
>
> Yes, but is that 4 results in 10MM LoC, or 4000 results in 40k LoC?
> ;-) I guess I just don't have a good feel for how large the codebase
> is, and how many times it resulted in matching sane code. If it's
> really low (like 4 out of 10MM LoC), I think the checker may possibly
> be useful (just not in that code base). If it's anything remotely
> high, then I would agree, let's ditch it and not look back.
>
> ~Aaron
>
> >
> > On 16 Sep 2015 21:23, "Aaron Ballman"  wrote:
> >>
> >> On Wed, Sep 16, 2015 at 2:21 PM, Alexander Kornienko  >
> >> wrote:
> >> > An update: I didn't find a single bug with this check in a large
> >> > codebase.
> >> > Turns out that it's rather useless. I'm inclined to kill it.
> >>
> >> How bad is the false positive rate?
> >>
> >> ~Aaron
> >>
> >> >
> >> >
> >> > On Sun, Sep 13, 2015 at 11:22 AM, Alexander Kornienko
> >> > 
> >> > wrote:
> >> >>
> >> >> I've also found a bunch of similar cases in our codebase, and I'm
> >> >> trying
> >> >> to figure out whether the pattern can be narrowed down to just
> >> >> dangerous
> >> >> cases. If we don't find a way to do so, we'll probably have to resort
> >> >> to "//
> >> >> NOLINT" to shut clang-tidy up.
> >> >>
> >> >> On 13 Sep 2015 10:52, "Kim Gräsman"  wrote:
> >> >>>
> >> >>> Late to the party, but I wanted to ask: is there a way to indicate
> to
> >> >>> the checker that we really *did* mean sizeof()?
> >> >>>
> >> >>> I think I've stumbled over code in our code base that uses
> >> >>> sizeof(container) to report memory usage statistics and it seems
> >> >>> valid, so it'd be nice if this checker could be silenced on a
> >> >>> case-by-case basis.
> >> >>>
> >> >>> Thanks,
> >> >>> - Kim
> >> >>>
> >> >>> On Sat, Sep 12, 2015 at 12:09 AM, Alexander Kornienko via
> cfe-commits
> >> >>>  wrote:
> >> >>> > Indeed. But this has been fixed before I could get to it.
> >> >>> >
> >> >>> >
> >> >>> > On Thu, Sep 10, 2015 at 10:47 PM, Aaron Ballman via cfe-commits
> >> >>> >  wrote:
> >> >>> >>
> >> >>> >> aaron.ballman added a comment.
> >> >>> >>
> >> >>> >> This appears to have broken one of the bots:
> >> >>> >>
> >> >>> >> http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/15065
> >> >>> >>
> >> >>> >>
> >> >>> >> http://reviews.llvm.org/D12759
> >> >>> >>
> >> >>> >>
> >> >>> >>
> >> >>> >> ___
> >> >>> >> cfe-commits mailing list
> >> >>> >> cfe-commits@lists.llvm.org
> >> >>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >> >>> >
> >> >>> >
> >> >>> > ___
> >> >>> > cfe-commits mailing list
> >> >>> > cfe-commits@lists.llvm.org
> >> >>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >> >>> >
> >> >
> >> >
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

[Aaron Ballman via cfe-commits]

On Wed, Sep 16, 2015 at 5:21 PM, Alexander Kornienko  wrote:
> All found results were intended usages of sizeof on containers. 100% false
> positive rate that is.

Yes, but is that 4 results in 10MM LoC, or 4000 results in 40k LoC?
;-) I guess I just don't have a good feel for how large the codebase
is, and how many times it resulted in matching sane code. If it's
really low (like 4 out of 10MM LoC), I think the checker may possibly
be useful (just not in that code base). If it's anything remotely
high, then I would agree, let's ditch it and not look back.

~Aaron

>
> On 16 Sep 2015 21:23, "Aaron Ballman"  wrote:
>>
>> On Wed, Sep 16, 2015 at 2:21 PM, Alexander Kornienko 
>> wrote:
>> > An update: I didn't find a single bug with this check in a large
>> > codebase.
>> > Turns out that it's rather useless. I'm inclined to kill it.
>>
>> How bad is the false positive rate?
>>
>> ~Aaron
>>
>> >
>> >
>> > On Sun, Sep 13, 2015 at 11:22 AM, Alexander Kornienko
>> > 
>> > wrote:
>> >>
>> >> I've also found a bunch of similar cases in our codebase, and I'm
>> >> trying
>> >> to figure out whether the pattern can be narrowed down to just
>> >> dangerous
>> >> cases. If we don't find a way to do so, we'll probably have to resort
>> >> to "//
>> >> NOLINT" to shut clang-tidy up.
>> >>
>> >> On 13 Sep 2015 10:52, "Kim Gräsman"  wrote:
>> >>>
>> >>> Late to the party, but I wanted to ask: is there a way to indicate to
>> >>> the checker that we really *did* mean sizeof()?
>> >>>
>> >>> I think I've stumbled over code in our code base that uses
>> >>> sizeof(container) to report memory usage statistics and it seems
>> >>> valid, so it'd be nice if this checker could be silenced on a
>> >>> case-by-case basis.
>> >>>
>> >>> Thanks,
>> >>> - Kim
>> >>>
>> >>> On Sat, Sep 12, 2015 at 12:09 AM, Alexander Kornienko via cfe-commits
>> >>>  wrote:
>> >>> > Indeed. But this has been fixed before I could get to it.
>> >>> >
>> >>> >
>> >>> > On Thu, Sep 10, 2015 at 10:47 PM, Aaron Ballman via cfe-commits
>> >>> >  wrote:
>> >>> >>
>> >>> >> aaron.ballman added a comment.
>> >>> >>
>> >>> >> This appears to have broken one of the bots:
>> >>> >>
>> >>> >> http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/15065
>> >>> >>
>> >>> >>
>> >>> >> http://reviews.llvm.org/D12759
>> >>> >>
>> >>> >>
>> >>> >>
>> >>> >> ___
>> >>> >> cfe-commits mailing list
>> >>> >> cfe-commits@lists.llvm.org
>> >>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >>> >
>> >>> >
>> >>> > ___
>> >>> > cfe-commits mailing list
>> >>> > cfe-commits@lists.llvm.org
>> >>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >>> >
>> >
>> >
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

[Alexander Kornienko via cfe-commits]

All found results were intended usages of sizeof on containers. 100% false
positive rate that is.
On 16 Sep 2015 21:23, "Aaron Ballman"  wrote:

> On Wed, Sep 16, 2015 at 2:21 PM, Alexander Kornienko 
> wrote:
> > An update: I didn't find a single bug with this check in a large
> codebase.
> > Turns out that it's rather useless. I'm inclined to kill it.
>
> How bad is the false positive rate?
>
> ~Aaron
>
> >
> >
> > On Sun, Sep 13, 2015 at 11:22 AM, Alexander Kornienko  >
> > wrote:
> >>
> >> I've also found a bunch of similar cases in our codebase, and I'm trying
> >> to figure out whether the pattern can be narrowed down to just dangerous
> >> cases. If we don't find a way to do so, we'll probably have to resort
> to "//
> >> NOLINT" to shut clang-tidy up.
> >>
> >> On 13 Sep 2015 10:52, "Kim Gräsman"  wrote:
> >>>
> >>> Late to the party, but I wanted to ask: is there a way to indicate to
> >>> the checker that we really *did* mean sizeof()?
> >>>
> >>> I think I've stumbled over code in our code base that uses
> >>> sizeof(container) to report memory usage statistics and it seems
> >>> valid, so it'd be nice if this checker could be silenced on a
> >>> case-by-case basis.
> >>>
> >>> Thanks,
> >>> - Kim
> >>>
> >>> On Sat, Sep 12, 2015 at 12:09 AM, Alexander Kornienko via cfe-commits
> >>>  wrote:
> >>> > Indeed. But this has been fixed before I could get to it.
> >>> >
> >>> >
> >>> > On Thu, Sep 10, 2015 at 10:47 PM, Aaron Ballman via cfe-commits
> >>> >  wrote:
> >>> >>
> >>> >> aaron.ballman added a comment.
> >>> >>
> >>> >> This appears to have broken one of the bots:
> >>> >>
> >>> >> http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/15065
> >>> >>
> >>> >>
> >>> >> http://reviews.llvm.org/D12759
> >>> >>
> >>> >>
> >>> >>
> >>> >> ___
> >>> >> cfe-commits mailing list
> >>> >> cfe-commits@lists.llvm.org
> >>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >>> >
> >>> >
> >>> > ___
> >>> > cfe-commits mailing list
> >>> > cfe-commits@lists.llvm.org
> >>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >>> >
> >
> >
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r247851 - [WinEH] Fix a build issue in CGException.cpp

[Reid Kleckner via cfe-commits]

Author: rnk
Date: Wed Sep 16 16:06:09 2015
New Revision: 247851

URL: http://llvm.org/viewvc/llvm-project?rev=247851&view=rev
Log:
[WinEH] Fix a build issue in CGException.cpp

I was constructing an object without filling in all the fields.

Modified:
cfe/trunk/lib/CodeGen/CGException.cpp

Modified: cfe/trunk/lib/CodeGen/CGException.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGException.cpp?rev=247851&r1=247850&r2=247851&view=diff
==
--- cfe/trunk/lib/CodeGen/CGException.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGException.cpp Wed Sep 16 16:06:09 2015
@@ -1845,8 +1845,7 @@ void CodeGenFunction::EnterSEHTryStmt(co
   HelperCGF.GenerateSEHFilterFunction(*this, *Except);
   llvm::Constant *OpaqueFunc =
   llvm::ConstantExpr::getBitCast(FilterFunc, Int8PtrTy);
-  CatchScope->setHandler(0, CatchTypeInfo{OpaqueFunc},
- createBasicBlock("__except.ret"));
+  CatchScope->setHandler(0, OpaqueFunc, createBasicBlock("__except.ret"));
 }
 
 void CodeGenFunction::ExitSEHTryStmt(const SEHTryStmt &S) {


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12747: Implement [depr.c.headers]

[Richard Smith via cfe-commits]

On Mon, Sep 14, 2015 at 7:07 AM, Marshall Clow 
wrote:

> mclow.lists added a comment.
>
> I have two concerns about this patch (w/o commenting on the actual code).
>
> 1. Until very recently, I was under the impression that C libraries
> _either_ defined a macro, or had a function. I was quite surprised to find
> that glibc did both.


Yes, this is required by the C standard. C11 7.1.4/1 says:

"Any function declared in a header may be additionally implemented as a
function-like macro defined in the header [...]. Any macro definition of a
function can be suppressed locally by enclosing the name of the function in
parentheses, because the name is then not followed by the left parenthesis
that indicates expansion of a macro function name. For the same syntactic
reason, it is permitted to take the address of a library function even if
it is also defined as a macro. [Footnote: This means that an implementation
shall provide an actual function for each library function, even if it also
provides a macro for that function.]"

Have you checked other C libraries (Apple, FreeBSD, Android, Windows) to
> see if they also define both?


No, but libstdc++ does the same #undef thing, so any platform it supports
must have a non-broken C standard library.


> 2. This adds a lot of header files. Each header file slows down
> compilation, and standard library header files get included *a lot*. We may
> not be able to avoid this, but we should think about the costs here.


I created a .cpp file that includes all of the <*.h> headers and does
nothing else (which should maximize the performance difference), and built
it with and without this change. I could not measure any difference (the
average compile time with this change was slightly lower, but that is
almost certainly noise).
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

Attached is an updated patch for clang-tools-extra that does not have
my in-progress, not-related-to-any-of-this code in it. ;-)

~Aaron

On Wed, Sep 16, 2015 at 4:04 PM, Aaron Ballman  wrote:
> Quick ping. I know this is a fairly gigantic patch, but I'm hoping for
> a relatively quick review turnaround because of potential merge
> conflicts with people doing a fair amount of work on clang-tidy
> lately. Everything should be pretty straight-forward (it's all just
> renames, no semantic changes intended aside from
> recordDecl/cxxRecordDecl and the narrowing matchers.
>
> ~Aaron
>
> On Tue, Sep 15, 2015 at 1:32 PM, Aaron Ballman  wrote:
>> Here are the complete patches to solve the issues we've discussed:
>>
>> 1) splits recordDecl into recordDecl and cxxRecordDecl
>> 2) adds isStruct, isUnion, isClass to identify what kind of
>> recordDecl() you may be looking at
>> 3) prefixes all of the node matchers with cxx that should have it
>> 4) fixes a similar issue with CUDAKernelCallExpr (the prefix needs to
>> be cuda instead of CUDA to distinguish the matcher name from the type
>> name).
>> 5) updates all of the documentation and code that broke.
>>
>> One patch is for changes to clang, the other is for changes to
>> clang-tools-extra.
>>
>> ~Aaron
>>
>> On Mon, Sep 14, 2015 at 5:49 PM, Aaron Ballman  
>> wrote:
>>> On Mon, Sep 14, 2015 at 5:47 PM, Daniel Jasper  wrote:
 Btw, I think generating them, potentially into several different headers to
 work around the compile time issue isn't such a bad idea.
>>>
>>> I'm not going to start with this approach, but think it may be worth
>>> exploring at some point. ;-)
>>>
>>> ~Aaron
>>>

 On Mon, Sep 14, 2015 at 11:45 PM, Manuel Klimek  wrote:
>
> Feel free to rename the AST nodes :)
>
>
> On Mon, Sep 14, 2015, 2:44 PM Daniel Jasper  wrote:
>>
>> Ok. I am happy with this then.
>>
>> (Just personally grumpy having to write
>> cxxRecordDecl(has(cxxConstructorDecl(..))) in the future ;-) ).
>>
>> On Mon, Sep 14, 2015 at 11:41 PM, Manuel Klimek 
>> wrote:
>>>
>>>
>>>
>>> On Mon, Sep 14, 2015 at 2:29 PM Aaron Ballman 
>>> wrote:

 On Mon, Sep 14, 2015 at 4:38 PM, Manuel Klimek 
 wrote:
 >
 >
 > On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman
 > 
 > wrote:
 >>
 >> On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper 
 >> wrote:
 >> > By this point, I see that change might be profitable overall.
 >> > However,
 >> > lets
 >> > completely map this out. Changing just cxxRecordDecl() can
 >> > actually
 >> > increase
 >> > confusion in other areas. Right now, not a single AST matcher has
 >> > the
 >> > cxx
 >> > prefix (although a total of 28 stand for the corresponding CXX..
 >> > AST
 >> > node).
 >> > This is consistent and people knowing this will never try to write
 >> > cxxConstructExpr(). As soon as people have used cxxRecordDecl(),
 >> > the
 >> > chance
 >> > of them trying cxxConstructExpr() increases. You have spent a long
 >> > time
 >> > figuring out that recordDecl means cxxRecordDecl(), which is one
 >> > datapoint,
 >> > but I am not aware of anyone else having this specific issue. And
 >> > we
 >> > could
 >> > make this less bad with better documentation, I think.
 >> >
 >> > So, for me, the questions are:
 >> > 1) Do we want/need this change?
 >>
 >> We definitely need *a* change because there currently is no way to
 >> match a C struct or union when compiling in C mode. I discovered
 >> this
 >> because I was trying to write a new checker for clang-tidy that
 >> focuses on C code and it would fail to match when compiling in C
 >> mode.
 >> Whether we decide to go with cxxRecordDecl vs recordDecl vs
 >> structDecl
 >> (etc) is less important to me than the ability to write clang-tidy
 >> checks for C code.
 >>
 >> > 2) Do we want to be consistent and change the other 27 matchers as
 >> > well?
 >>
 >> I'm on the fence about this for all the reasons you point out.
 >>
 >> > A fundamental question is whether we want AST matchers to match
 >> > AST
 >> > nodes
 >> > 1:1 or whether they should be an abstraction from some
 >> > implementation
 >> > details of the AST.
 >>
 >> I absolutely agree that this is a fundamental question. I think a
 >> higher priority fundamental question that goes along with it is: are
 >> we okay with breaking a lot of user code (are these meant to be
 >> stable
 >> APIs like the LLVM C APIs)? If we want these APIs to be stable, 

Re: [PATCH] D10305: [Clang Static Analyzer] Bug identification

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Thanks! Will do.


http://reviews.llvm.org/D10305



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] __attribute__((enable_if)) and non-overloaded member functions

[Nick Lewycky via cfe-commits]

+gbiv, would you be able to review this?
On Sep 14, 2015 7:42 AM, "Ettore Speziale" 
wrote:

> Ping
>
> > Gently ping.
> >
> >> On Aug 26, 2015, at 2:40 PM, Ettore Speziale 
> wrote:
> >>
> >> Forward to the right ML:
> >>
>  Sorry about the extreme delay. This patch slipped through the cracks,
> and I only noticed it again when searching my email for enable_if.
> Committed in r245985! In the future, please feel free to continue pinging
> weekly!
> >>>
> >>> NP, thank you for committing the patch.
> >>>
> >>> Unfortunately it contains a little error in the case of no candidate
> has been found. For instance consider the following test case:
> >>>
> >>> struct Incomplete;
> >>>
> >>> struct X {
> >>> void hidden_by_argument_conversion(Incomplete n, int m = 0)
> __attribute((enable_if(m == 10, "chosen when 'm' is ten")));
> >>> };
> >>>
> >>> x.hidden_by_argument_conversion(10);
> >>>
> >>> I would expect to get an error about Incomplete, as the compiler
> cannot understand how to convert 10 into an instance of Incomplete. However
> right now the enable_if diagnostic is emitted, thus masking the more useful
> message about Incomplete.
> >>>
> >>> The attached patch solved the problem by delaying the point where the
> enable_if diagnostic is issued.
> >>>
> >>> Thanks,
> >>> Ettore Speziale
> >>
> >>
> >> 
> >
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12547: Add support for function attribute "disable_tail_calls"

[Akira Hatanaka via cfe-commits]

ahatanak updated this revision to Diff 34919.
ahatanak added a comment.

Made the following changes to address review comments:

- Use checkAttrMutualExclusion.
- Removed spaces in the check strings in test case attr-disable-tail-calls.c.


http://reviews.llvm.org/D12547

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/CodeGen/CGCall.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/attr-disable-tail-calls.c
  test/Sema/disable-tail-calls-attr.c

Index: test/Sema/disable-tail-calls-attr.c
===
--- /dev/null
+++ test/Sema/disable-tail-calls-attr.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void __attribute__((disable_tail_calls,naked)) foo1(int a) { // expected-error {{'disable_tail_calls' and 'naked' attributes are not compatible}}
+  __asm__("");
+}
Index: test/CodeGen/attr-disable-tail-calls.c
===
--- test/CodeGen/attr-disable-tail-calls.c
+++ test/CodeGen/attr-disable-tail-calls.c
@@ -1,11 +1,19 @@
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -mdisable-tail-calls -o - | FileCheck %s -check-prefix=CHECK -check-prefix=DISABLE
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -o - | FileCheck %s -check-prefix=CHECK -check-prefix=ENABLE
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -mdisable-tail-calls -o - | FileCheck %s -check-prefix=DISABLE
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -o - | FileCheck %s -check-prefix=ENABLE
 
-// CHECK: define i32 @f1() [[ATTR:#[0-9]+]] {
+// DISABLE: define i32 @f1() [[ATTRTRUE:#[0-9]+]] {
+// DISABLE: define i32 @f2() [[ATTRTRUE]] {
+// ENABLE: define i32 @f1() [[ATTRFALSE:#[0-9]+]] {
+// ENABLE: define i32 @f2() [[ATTRTRUE:#[0-9]+]] {
 
 int f1() {
   return 0;
 }
 
-// DISABLE: attributes [[ATTR]] = { {{.*}} "disable-tail-calls"="true" {{.*}} }
-// ENABLE: attributes [[ATTR]] = { {{.*}} "disable-tail-calls"="false" {{.*}} }
+int f2() __attribute__((disable_tail_calls)) {
+  return 0;
+}
+
+// DISABLE: attributes [[ATTRTRUE]] = { {{.*}}"disable-tail-calls"="true"{{.*}} }
+// ENABLE: attributes [[ATTRFALSE]] = { {{.*}}"disable-tail-calls"="false"{{.*}} }
+// ENABLE: attributes [[ATTRTRUE]] = { {{.*}}"disable-tail-calls"="true"{{.*}} }
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -1696,6 +1696,15 @@
Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleDisableTailCallsAttr(Sema &S, Decl *D,
+   const AttributeList &Attr) {
+  if (checkAttrMutualExclusion(S, D, Attr))
+return;
+
+  D->addAttr(::new (S.Context) DisableTailCallsAttr(
+  Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
+}
+
 static void handleUsedAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   if (const VarDecl *VD = dyn_cast(D)) {
 if (VD->hasLocalStorage()) {
@@ -4886,6 +4895,9 @@
   case AttributeList::AT_ReturnsTwice:
 handleSimpleAttribute(S, D, Attr);
 break;
+  case AttributeList::AT_DisableTailCalls:
+handleDisableTailCallsAttr(S, D, Attr);
+break;
   case AttributeList::AT_Used:
 handleUsedAttr(S, D, Attr);
 break;
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -1480,8 +1480,12 @@
   FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
 }
 
+bool DisableTailCalls =
+(TargetDecl && TargetDecl->hasAttr()) ||
+CodeGenOpts.DisableTailCalls;
 FuncAttrs.addAttribute("disable-tail-calls",
-   llvm::toStringRef(CodeGenOpts.DisableTailCalls));
+   llvm::toStringRef(DisableTailCalls));
+
 FuncAttrs.addAttribute("less-precise-fpmad",
llvm::toStringRef(CodeGenOpts.LessPreciseFPMAD));
 FuncAttrs.addAttribute("no-infs-fp-math",
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -1612,3 +1612,10 @@
 arguments, with arbitrary offsets.
   }];
 }
+
+def DisableTailCallsDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The ``disable_tail_calls`` attribute instructs the backend to not perform tail call optimization.
+  }];
+}
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -881,6 +881,13 @@
   let Documentation = [Undocumented];
 }
 
+def DisableTailCalls : InheritableAttr {
+  let Spellings = [GNU<"disable_tail_calls">,
+   CXX11<"clang", "disable_tail_calls">];
+  let Subjects = Subject

Re: [PATCH] D12547: Add support for function attribute "disable_tail_calls"

[Akira Hatanaka via cfe-commits]

On Wed, Sep 16, 2015 at 8:05 AM, Aaron Ballman 
wrote:

> On Wed, Sep 16, 2015 at 12:47 AM, Akira Hatanaka 
> wrote:
> > ahatanak added inline comments.
> >
> > 
> > Comment at: include/clang/Basic/Attr.td:894
> > @@ -893,1 +893,3 @@
> >
> > +def DisableTailCalls : InheritableAttr {
> > +  let Spellings = [GNU<"disable_tail_calls">,
> > 
> > aaron.ballman wrote:
> >> Pardon me if this is obvious, but -- are there times when you would
> want to disable tail calls but leave other optimizations in place? I'm
> trying to understand why these attributes are orthogonal.
> > Yes, that's correct. The new function attribute we are adding shouldn't
> disable other optimizations that are normally run at -O2 or -O3.
>
> Okay -- with this and your other explanations, I now understand why
> this attribute is orthogonal to optnone. Thanks!
>
> > 
> > Comment at: include/clang/Basic/Attr.td:896
> > @@ +895,3 @@
> > +  let Spellings = [GNU<"disable_tail_calls">,
> > +   CXX11<"clang", "disable_tail_calls">];
> > +  let Subjects = SubjectList<[Function, ObjCMethod]>;
> > 
> > aaron.ballman wrote:
> >> Should this attribute be plural? Are there multiple tail calls within a
> single method that can be optimized away?
> > I named it after the existing IR function attribute and I'm guessing the
> plural name was chosen because functions can have multiple tail call sites.
> I think the singular name is better if we want this attribute to mean
> "disable tail call optimization".
>
> I'm holding off on having an opinion on the name until I have an idea
> as to the other planned attribute's name, but I basically think the
> plural is reasonable if the purpose is to disable TCO on the functions
> called within the marked function. But we may want to consider making
> the name more obvious. The majority of the attributes we support
> appertain to the behavior of the declaration itself, not to entities
> *within* the declaration. However, it seems like no_sanitize,
> no_sanitize_thread, et al are similar concepts. Perhaps we want this
> to be named "no_tail_calls" instead? Just something to think about.
>
>
I think something similar to no_sanitize makes sense.

The tentative names I had were notailcall for this attribute and notail for
the other, but I'm open to suggestions if there are better names.

I'll send a patch to support the other attribute today.

> 
> Comment at: include/clang/Basic/AttrDocs.td:1619
> @@ +1618,3 @@
> +  let Content = [{
> +Use ``__attribute__((disable_tail_calls)))`` to instruct the backend not
to do tail call optimization.
> +  }];
> 
> aaron.ballman wrote:
>> I would avoid using the exact syntax here because this is a GNU and C++
attribute. Could just say:
>>
>> The ``disable_tail_calls`` attribute instructs the backend to not
perform tail call optimization.
> OK, done.
>
> 
> Comment at: lib/Sema/SemaDeclAttr.cpp:4882
> @@ +4881,3 @@
> +  case AttributeList::AT_DisableTailCalls:
> +handleSimpleAttribute(S, D, Attr);
> +break;
> 
> aaron.ballman wrote:
>> Okay, that makes sense. I can contrive examples of noreturn where TCO
could happen, it just took me a while to think of them. ;-)
>>
>> What about other semantic checks, like warning the user about disabling
TCO when TCO could never be enabled in the first place? Can you disable TCO
for functions that are marked __attribute__((naked))? What about
returns_twice?
>>
>> Unfortunately, we have a lot of attributes for which we've yet to write
documentation, so you may need to look through Attr.td.
> Since "naked" allows only inline assembly statements, it should be an
error to have disable-tail-calls and naked on the same function. I made
changes in Sema to detect that case.

__declspec(naked) behaves differently than __attribute__((naked)), but
I'm not certain whether that changes the reasoning
(https://msdn.microsoft.com/en-us/library/h5w10wxs.aspx).

gcc seems to take a stricter stance on what is allowed inside naked
functions, but I think it's still safe to say it's a mistake to attach
disable-tail-calls to a naked function (because you have no control over
it), regardless of whether it's an msvc or gcc attribute.

https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html#ARM-Function-Attributes


> > My understanding is that you can't do tail call optimization on call
> sites of a function that calls a "return_twice" function. However,
> attaching "return_twice" on the calling function doesn't block tail call
> optimizations on the call sites inside the function.
>
> That makes sense to me.
>
> > I didn't find any other attributes that seemed incompatible with
> disable-tail-calls.
>
> If a user finds something, we can handle it at that time. Thank you
> for looking into this!
>
> > Index: include/clang/Basic/AttrDocs.td
> > ===
> > ---

r247843 - [WinEH] Pass the catch adjectives to catchpad directly

[Reid Kleckner via cfe-commits]

Author: rnk
Date: Wed Sep 16 15:15:55 2015
New Revision: 247843

URL: http://llvm.org/viewvc/llvm-project?rev=247843&view=rev
Log:
[WinEH] Pass the catch adjectives to catchpad directly

This avoids building a fake LLVM IR global variable just to ferry an i32
down into LLVM codegen. It also puts a nail in the coffin of using MS
ABI C++ EH with landingpads, since now we'll assert in the lpad code
when flags are present.

Modified:
cfe/trunk/lib/CodeGen/CGCXXABI.cpp
cfe/trunk/lib/CodeGen/CGCXXABI.h
cfe/trunk/lib/CodeGen/CGCleanup.h
cfe/trunk/lib/CodeGen/CGException.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/test/CodeGenCXX/exceptions-cxx-new.cpp
cfe/trunk/test/CodeGenCXX/microsoft-abi-eh-catch.cpp
cfe/trunk/test/CodeGenCXX/microsoft-abi-try-throw.cpp

Modified: cfe/trunk/lib/CodeGen/CGCXXABI.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCXXABI.cpp?rev=247843&r1=247842&r2=247843&view=diff
==
--- cfe/trunk/lib/CodeGen/CGCXXABI.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCXXABI.cpp Wed Sep 16 15:15:55 2015
@@ -13,6 +13,7 @@
 
//===--===//
 
 #include "CGCXXABI.h"
+#include "CGCleanup.h"
 
 using namespace clang;
 using namespace CodeGen;
@@ -321,3 +322,7 @@ CGCXXABI::emitTerminateForUnexpectedExce
   // Just call std::terminate and ignore the violating exception.
   return CGF.EmitNounwindRuntimeCall(CGF.CGM.getTerminateFn());
 }
+
+CatchTypeInfo CGCXXABI::getCatchAllTypeInfo() {
+  return CatchTypeInfo{nullptr, 0};
+}

Modified: cfe/trunk/lib/CodeGen/CGCXXABI.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCXXABI.h?rev=247843&r1=247842&r2=247843&view=diff
==
--- cfe/trunk/lib/CodeGen/CGCXXABI.h (original)
+++ cfe/trunk/lib/CodeGen/CGCXXABI.h Wed Sep 16 15:15:55 2015
@@ -37,6 +37,7 @@ class MangleContext;
 namespace CodeGen {
 class CodeGenFunction;
 class CodeGenModule;
+struct CatchTypeInfo;
 
 /// \brief Implements C++ ABI-specific code generation functions.
 class CGCXXABI {
@@ -236,8 +237,9 @@ public:
   llvm::Value *Exn);
 
   virtual llvm::Constant *getAddrOfRTTIDescriptor(QualType Ty) = 0;
-  virtual llvm::Constant *
+  virtual CatchTypeInfo
   getAddrOfCXXCatchHandlerType(QualType Ty, QualType CatchHandlerType) = 0;
+  virtual CatchTypeInfo getCatchAllTypeInfo();
 
   virtual bool shouldTypeidBeNullChecked(bool IsDeref,
  QualType SrcRecordTy) = 0;

Modified: cfe/trunk/lib/CodeGen/CGCleanup.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCleanup.h?rev=247843&r1=247842&r2=247843&view=diff
==
--- cfe/trunk/lib/CodeGen/CGCleanup.h (original)
+++ cfe/trunk/lib/CodeGen/CGCleanup.h Wed Sep 16 15:15:55 2015
@@ -33,6 +33,13 @@ namespace CodeGen {
 class CodeGenModule;
 class CodeGenFunction;
 
+/// The MS C++ ABI needs a pointer to RTTI data plus some flags to describe the
+/// type of a catch handler, so we use this wrapper.
+struct CatchTypeInfo {
+  llvm::Constant *RTTI;
+  unsigned Flags;
+};
+
 /// A protected scope for zero-cost EH handling.
 class EHScope {
   llvm::BasicBlock *CachedLandingPad;
@@ -153,12 +160,12 @@ public:
   struct Handler {
 /// A type info value, or null (C++ null, not an LLVM null pointer)
 /// for a catch-all.
-llvm::Constant *Type;
+CatchTypeInfo Type;
 
 /// The catch handler for this type.
 llvm::BasicBlock *Block;
 
-bool isCatchAll() const { return Type == nullptr; }
+bool isCatchAll() const { return Type.RTTI == nullptr; }
   };
 
 private:
@@ -188,11 +195,17 @@ public:
   }
 
   void setCatchAllHandler(unsigned I, llvm::BasicBlock *Block) {
-setHandler(I, /*catchall*/ nullptr, Block);
+setHandler(I, CatchTypeInfo{nullptr, 0}, Block);
   }
 
   void setHandler(unsigned I, llvm::Constant *Type, llvm::BasicBlock *Block) {
 assert(I < getNumHandlers());
+getHandlers()[I].Type = CatchTypeInfo{Type, 0};
+getHandlers()[I].Block = Block;
+  }
+
+  void setHandler(unsigned I, CatchTypeInfo Type, llvm::BasicBlock *Block) {
+assert(I < getNumHandlers());
 getHandlers()[I].Type = Type;
 getHandlers()[I].Block = Block;
   }

Modified: cfe/trunk/lib/CodeGen/CGException.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGException.cpp?rev=247843&r1=247842&r2=247843&view=diff
==
--- cfe/trunk/lib/CodeGen/CGException.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGException.cpp Wed Sep 16 15:15:55 2015
@@ -554,16 +554,16 @@ void CodeGenFunct

Re: [PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:328
@@ -290,1 +327,3 @@
+// DereferenceChecker, CallAndMessageChecker, and DynamicTypePropagation)
+// rely upon the defensive behavior and would need to be updated.
 if (!State || (State == Pred->getState() && !Tag && !MarkAsSink))

I think the most common form of relying on this branch is adding a transition 
to an unchanged state.


http://reviews.llvm.org/D12780



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

Quick ping. I know this is a fairly gigantic patch, but I'm hoping for
a relatively quick review turnaround because of potential merge
conflicts with people doing a fair amount of work on clang-tidy
lately. Everything should be pretty straight-forward (it's all just
renames, no semantic changes intended aside from
recordDecl/cxxRecordDecl and the narrowing matchers.

~Aaron

On Tue, Sep 15, 2015 at 1:32 PM, Aaron Ballman  wrote:
> Here are the complete patches to solve the issues we've discussed:
>
> 1) splits recordDecl into recordDecl and cxxRecordDecl
> 2) adds isStruct, isUnion, isClass to identify what kind of
> recordDecl() you may be looking at
> 3) prefixes all of the node matchers with cxx that should have it
> 4) fixes a similar issue with CUDAKernelCallExpr (the prefix needs to
> be cuda instead of CUDA to distinguish the matcher name from the type
> name).
> 5) updates all of the documentation and code that broke.
>
> One patch is for changes to clang, the other is for changes to
> clang-tools-extra.
>
> ~Aaron
>
> On Mon, Sep 14, 2015 at 5:49 PM, Aaron Ballman  wrote:
>> On Mon, Sep 14, 2015 at 5:47 PM, Daniel Jasper  wrote:
>>> Btw, I think generating them, potentially into several different headers to
>>> work around the compile time issue isn't such a bad idea.
>>
>> I'm not going to start with this approach, but think it may be worth
>> exploring at some point. ;-)
>>
>> ~Aaron
>>
>>>
>>> On Mon, Sep 14, 2015 at 11:45 PM, Manuel Klimek  wrote:

 Feel free to rename the AST nodes :)


 On Mon, Sep 14, 2015, 2:44 PM Daniel Jasper  wrote:
>
> Ok. I am happy with this then.
>
> (Just personally grumpy having to write
> cxxRecordDecl(has(cxxConstructorDecl(..))) in the future ;-) ).
>
> On Mon, Sep 14, 2015 at 11:41 PM, Manuel Klimek 
> wrote:
>>
>>
>>
>> On Mon, Sep 14, 2015 at 2:29 PM Aaron Ballman 
>> wrote:
>>>
>>> On Mon, Sep 14, 2015 at 4:38 PM, Manuel Klimek 
>>> wrote:
>>> >
>>> >
>>> > On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman
>>> > 
>>> > wrote:
>>> >>
>>> >> On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper 
>>> >> wrote:
>>> >> > By this point, I see that change might be profitable overall.
>>> >> > However,
>>> >> > lets
>>> >> > completely map this out. Changing just cxxRecordDecl() can
>>> >> > actually
>>> >> > increase
>>> >> > confusion in other areas. Right now, not a single AST matcher has
>>> >> > the
>>> >> > cxx
>>> >> > prefix (although a total of 28 stand for the corresponding CXX..
>>> >> > AST
>>> >> > node).
>>> >> > This is consistent and people knowing this will never try to write
>>> >> > cxxConstructExpr(). As soon as people have used cxxRecordDecl(),
>>> >> > the
>>> >> > chance
>>> >> > of them trying cxxConstructExpr() increases. You have spent a long
>>> >> > time
>>> >> > figuring out that recordDecl means cxxRecordDecl(), which is one
>>> >> > datapoint,
>>> >> > but I am not aware of anyone else having this specific issue. And
>>> >> > we
>>> >> > could
>>> >> > make this less bad with better documentation, I think.
>>> >> >
>>> >> > So, for me, the questions are:
>>> >> > 1) Do we want/need this change?
>>> >>
>>> >> We definitely need *a* change because there currently is no way to
>>> >> match a C struct or union when compiling in C mode. I discovered
>>> >> this
>>> >> because I was trying to write a new checker for clang-tidy that
>>> >> focuses on C code and it would fail to match when compiling in C
>>> >> mode.
>>> >> Whether we decide to go with cxxRecordDecl vs recordDecl vs
>>> >> structDecl
>>> >> (etc) is less important to me than the ability to write clang-tidy
>>> >> checks for C code.
>>> >>
>>> >> > 2) Do we want to be consistent and change the other 27 matchers as
>>> >> > well?
>>> >>
>>> >> I'm on the fence about this for all the reasons you point out.
>>> >>
>>> >> > A fundamental question is whether we want AST matchers to match
>>> >> > AST
>>> >> > nodes
>>> >> > 1:1 or whether they should be an abstraction from some
>>> >> > implementation
>>> >> > details of the AST.
>>> >>
>>> >> I absolutely agree that this is a fundamental question. I think a
>>> >> higher priority fundamental question that goes along with it is: are
>>> >> we okay with breaking a lot of user code (are these meant to be
>>> >> stable
>>> >> APIs like the LLVM C APIs)? If we want these APIs to be stable, that
>>> >> changes the answer of what kind of mapping we can have.
>>> >
>>> >
>>> > I think the AST matchers are so closely coupled to the AST that it
>>> > trying to
>>> > be more stable than the AST doesn't help. Basically all uses of AST
>>> > matchers
>>> > do something with the AST nodes 

Re: [PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[Devin Coughlin via cfe-commits]

dcoughlin added inline comments.


Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:321
@@ +320,3 @@
+// sink, we assume that a client requesting a transition to a state that is
+// the same as the predecessor state has made a mistake. We return the
+// predecessor and rather than cache out.

zaks.anna wrote:
> dcoughlin wrote:
> > jordan_rose wrote:
> > > What does "has made a mistake" mean? What is the mistake and how will 
> > > they discover it?
> > The mistake this guard protects against is adding a transition from a state 
> > to itself, which would normally cause a cache out. My understanding is that 
> > the whole point of this guard is to silently swallow that mistake. I found 
> > that surprising, which is why I added the comment.
> Should we add an assert here? I wonder if/how much it will get triggered.
I tried adding an assert to the inverse of the 'if' condition here. The 
DereferenceChecker, CallAndMessageChecker, and DynamicTypePropagation all 
trigger it. Added a note about this in a comment.


Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:322
@@ -290,1 +321,3 @@
+// the same as the predecessor state has made a mistake. We return the
+// predecessor and rather than cache out.
 if (!State || (State == Pred->getState() && !Tag && !MarkAsSink))

xazax.hun wrote:
> As a slightly related note: is it documented anywhere what "cache out" means? 
> Maybe it would be great to refer to that document or write it if it is not 
> written yet.
I've defined "cache out" in the comment.


Comment at: lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp:53
@@ -52,3 +52,3 @@
 
-  if (ExplodedNode *N = C.addTransition()) {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
 if (!BT)

dcoughlin wrote:
> zaks.anna wrote:
> > jordan_rose wrote:
> > > zaks.anna wrote:
> > > > Can this ever fail? In some cases we just assume it won't in others we 
> > > > tests..
> > > > 
> > > > Maybe it only fails when we cache out?
> > > It does fail when we cache out, and I think we can still cache out if 
> > > Pred has a different tag the second time around.
> > There some instances in this patch where we do not check if the returned 
> > node is null. We should be consistent. 
> Ok, I'll go through and add checks where they are missing.
There were 9 locations where checks for null error generated error nodes were 
missing. I added them.


http://reviews.llvm.org/D12780



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[Devin Coughlin via cfe-commits]

dcoughlin updated this revision to Diff 34917.
dcoughlin added a comment.

Added checks for null generated error nodes in the few cases in checkers were 
they were missing and updated comments.


http://reviews.llvm.org/D12780

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
  lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp
  lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
  lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
  lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
  lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  lib/StaticAnalyzer/Checkers/TaintTesterChecker.cpp
  lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
  lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  lib/StaticAnalyzer/Core/BugReporter.cpp
  test/Analysis/PR24184.cpp
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1386,7 +1386,9 @@
   int *s;
   char *b = realloc(a->p, size);
   char *m = realloc(a->p, size); // expected-warning {{Attempt to free released memory}}
-  return a->p;
+  // We don't expect a use-after-free for a->P here because the warning above
+  // is a sink.
+  return a->p; // no-warning
 }
 
 // We should not warn in this case since the caller will presumably free a->p in all cases.
Index: test/Analysis/PR24184.cpp
===
--- /dev/null
+++ test/Analysis/PR24184.cpp
@@ -0,0 +1,97 @@
+// RUN: %clang_cc1 -w -analyze -analyzer-eagerly-assume -fcxx-exceptions -analyzer-checker=core -analyzer-checker=alpha.core.PointerArithm,alpha.core.CastToStruct -analyzer-max-loop 64 -verify %s
+// RUN: %clang_cc1 -w -analyze -analyzer-checker=core -analyzer-checker=cplusplus -fcxx-exceptions -analyzer-checker alpha.core.PointerArithm,alpha.core.CastToStruct -analyzer-max-loop 63 -verify %s
+
+// These tests used to hit an assertion in the bug report. Test case from http://llvm.org/PR24184.
+typedef struct {
+  int cbData;
+  unsigned pbData;
+} CRYPT_DATA_BLOB;
+
+typedef enum { DT_NONCE_FIXED } DATA_TYPE;
+int a;
+typedef int *vcreate_t(int *, DATA_TYPE, int, int);
+void fn1(unsigned, unsigned) {
+  char b = 0;
+  for (; 1; a++, &b + a * 0) // expected-warning{{Pointer arithmetic done on non-array variables means reliance on memory layout, which is dangerous}}
+;
+}
+
+vcreate_t fn2;
+struct A {
+  CRYPT_DATA_BLOB value;
+  int m_fn1() {
+int c;
+value.pbData == 0;
+fn1(0, 0);
+  }
+};
+struct B {
+  A IkeHashAlg;
+  A IkeGType;
+  A NoncePhase1_r;
+};
+class C {
+  int m_fn2(B *);
+  void m_fn3(B *, int, int, int);
+};
+int C::m_fn2(B *p1) {
+  int *d;
+  int e = p1->IkeHashAlg.m_fn1();
+  unsigned f = p1->IkeGType.m_fn1(), h;
+  int g;
+  d = fn2(0, DT_NONCE_FIXED, (char)0, p1->NoncePhase1_r.value.cbData);
+  h = 0 | 0;
+  m_fn3(p1, 0, 0, 0);
+}
+
+// case 2:
+typedef struct {
+  int cbData;
+  unsigned char *pbData;
+} CRYPT_DATA_BLOB_1;
+typedef unsigned uint32_t;
+void fn1_1(void *p1, const void *p2) { p1 != p2; }
+
+void fn2_1(uint32_t *p1, unsigned char *p2, uint32_t p3) {
+  unsigned i = 0;
+  for (0; i < p3; i++)
+

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

[Aaron Ballman via cfe-commits]

On Wed, Sep 16, 2015 at 2:21 PM, Alexander Kornienko  wrote:
> An update: I didn't find a single bug with this check in a large codebase.
> Turns out that it's rather useless. I'm inclined to kill it.

How bad is the false positive rate?

~Aaron

>
>
> On Sun, Sep 13, 2015 at 11:22 AM, Alexander Kornienko 
> wrote:
>>
>> I've also found a bunch of similar cases in our codebase, and I'm trying
>> to figure out whether the pattern can be narrowed down to just dangerous
>> cases. If we don't find a way to do so, we'll probably have to resort to "//
>> NOLINT" to shut clang-tidy up.
>>
>> On 13 Sep 2015 10:52, "Kim Gräsman"  wrote:
>>>
>>> Late to the party, but I wanted to ask: is there a way to indicate to
>>> the checker that we really *did* mean sizeof()?
>>>
>>> I think I've stumbled over code in our code base that uses
>>> sizeof(container) to report memory usage statistics and it seems
>>> valid, so it'd be nice if this checker could be silenced on a
>>> case-by-case basis.
>>>
>>> Thanks,
>>> - Kim
>>>
>>> On Sat, Sep 12, 2015 at 12:09 AM, Alexander Kornienko via cfe-commits
>>>  wrote:
>>> > Indeed. But this has been fixed before I could get to it.
>>> >
>>> >
>>> > On Thu, Sep 10, 2015 at 10:47 PM, Aaron Ballman via cfe-commits
>>> >  wrote:
>>> >>
>>> >> aaron.ballman added a comment.
>>> >>
>>> >> This appears to have broken one of the bots:
>>> >>
>>> >> http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/15065
>>> >>
>>> >>
>>> >> http://reviews.llvm.org/D12759
>>> >>
>>> >>
>>> >>
>>> >> ___
>>> >> cfe-commits mailing list
>>> >> cfe-commits@lists.llvm.org
>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>> >
>>> >
>>> > ___
>>> > cfe-commits mailing list
>>> > cfe-commits@lists.llvm.org
>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>> >
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11963: Create a __config_site file to capture configuration decisions.

[Evgeniy Stepanov via cfe-commits]

eugenis added a comment.

Hi Eric, have you had a chance to take another look at this? It is blocking 
http://reviews.llvm.org/D11740.


http://reviews.llvm.org/D11963



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: PATCH: Expose the 'file' that is associated with a compile database command

[Manuel Klimek via cfe-commits]

Thanks!

On Wed, Sep 16, 2015 at 11:30 AM Argyrios Kyrtzidis 
wrote:

> In r247832.
>
> On Sep 12, 2015, at 5:19 AM, Manuel Klimek  wrote:
>
> Test would be awesome :) Thx!
>
> On Fri, Sep 11, 2015 at 10:44 PM Argyrios Kyrtzidis 
> wrote:
>
>> In r247468, thanks for reviewing!
>>
>> On Sep 11, 2015, at 10:24 AM, Manuel Klimek via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>> Ok, looked at the original patch again, and if we're fixing the
>> FixedCompilationDatabase to only insert the file when it actually produces
>> a CompileCommand it seems to be fine.
>>
>> On Fri, Sep 11, 2015 at 6:32 PM Argyrios Kyrtzidis 
>> wrote:
>>
>>> On Sep 11, 2015, at 12:21 AM, Manuel Klimek  wrote:
>>>
>>> On Thu, Sep 10, 2015 at 8:36 PM Argyrios Kyrtzidis 
>>> wrote:
>>>
 On Sep 10, 2015, at 1:48 AM, Manuel Klimek  wrote:

 @@ -179,11 +185,13 @@ public:
/// \param Directory The base directory used in the
 FixedCompilationDatabase.
static FixedCompilationDatabase *loadFromCommandLine(int &Argc,
 const char
 *const *Argv,
 -   Twine Directory
 = ".");
 +   Twine Directory
 = ".",
 +   Twine File =
 Twine());

 A fixed compilation database returns the same command lines for all
 files, thus having a file in the function seems strange.


 Ah ok, thanks for clarifying.


 What exactly is the use case? So far, the compilation database has been
 designed for 2 use cases:
 1. for a file, get the compile commands; the user already knows the
 file, no need to get the file
 2. get all compile commands; for that, we have the getAllFiles()
 method, so a user can get all known files (for compilation databases that
 support that), and then get the compile command line.


 It’s #2, I want to get all compile commands. But it seems really
 strange to me that the ‘file’ starts as a property of the compile command
 in the json file but then it gets dropped and I need to do work to
 re-associate the files with the compile commands again.

>>>
>>> The JSON file format is one possible implementation for the
>>> compilation-database interface. The FixedCompilationDatabase is another
>>> one, that doesn't have any information on files.
>>>
>>>
 I need to get a list of all the files and then for each one do a lookup
 to get the associated commands. I then have to maintain this association
 myself, passing a command along with its file separately or the structure
 that keeps track of the association.

 It seems simpler to me to include the file that was associated with the
 command (if the compilation database supports that) along with the command,
 is there a downside I’m missing ?

>>>
>>> Well, to me, it's a design question - if it also makes sense to have a
>>> CompileCommand without a file associated with it, putting the file in
>>> there, while convenient, is a design smell.
>>>
>>>
>>> It can be optional to communicate that it may not be there. Note that,
>>> IMO, having multiple files and compile commands for them is the
>>> overwhelmingly most common use of the compilation database.
>>>
>>> That said, I'm happy to be convinced that I'm wrong :) I guess I don't
>>> see yet that keeping track of the files outside is more than one line of
>>> extra code.
>>>
>>>
>>> Not sure what *one* line this is, I have to declare a map and then
>>> populate it, no ? And to do it in c-index-test it would take way more that
>>> one line.
>>> But it is also beyond populating a map, this has an effect on APIs using
>>> a CompileCommand. For example:
>>>
>>> void doSomethingWithCompileCommand(const CompileCommand &cmd);
>>>
>>> Ah it would be useful to know the file that this command was associated
>>> with:
>>>
>>> void doSomethingWithCompileCommand(const CompileCommand &cmd, StringRef
>>> filename);
>>>
>>> What do I have now ? This is a function taking a command and a string
>>> for the filename separately.
>>> Is this flexibility useful ? Does it make sense to pass any filename
>>> there ? No there’s only one filename that the command was associated with
>>> so this ‘flexibility’ only increases the complexity of using the function.
>>> And this can propagate to other function’s callees.
>>> This seems like a design smell to me.
>>>
>>>
>>> Cheers,
>>> /Manuel
>>>
>>>


 Thoughts?
 /Manuel

 On Wed, Sep 9, 2015 at 9:36 PM Argyrios Kyrtzidis 
 wrote:

> Hi,
>
> The attached patch exposes the ‘file’ entry in a compilation database
> command, via the CompileCommand structure.

 ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.o

Re: [PATCH] D12774: createUniqueFile() is documented to create the file in the temporary directory unless it's supplied an absolute path.Make sure the output filepath supplied to createUniqueFile() in

[Argyrios Kyrtzidis via cfe-commits]

+ Anna

> On Sep 14, 2015, at 1:25 PM, Cameron Esfahani  wrote:
> 
> dirty added a comment.
> 
> Ping?
> 
> 
> http://reviews.llvm.org/D12774
> 
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: PATCH: Expose the 'file' that is associated with a compile database command

[Argyrios Kyrtzidis via cfe-commits]

In r247832.

> On Sep 12, 2015, at 5:19 AM, Manuel Klimek  wrote:
> 
> Test would be awesome :) Thx!
> 
> On Fri, Sep 11, 2015 at 10:44 PM Argyrios Kyrtzidis  > wrote:
> In r247468, thanks for reviewing!
> 
> 
>> On Sep 11, 2015, at 10:24 AM, Manuel Klimek via cfe-commits 
>> mailto:cfe-commits@lists.llvm.org>> wrote:
>> 
> 
>> Ok, looked at the original patch again, and if we're fixing the 
>> FixedCompilationDatabase to only insert the file when it actually produces a 
>> CompileCommand it seems to be fine.
>> 
>> On Fri, Sep 11, 2015 at 6:32 PM Argyrios Kyrtzidis > > wrote:
>>> On Sep 11, 2015, at 12:21 AM, Manuel Klimek >> > wrote:
>>> 
>>> On Thu, Sep 10, 2015 at 8:36 PM Argyrios Kyrtzidis >> > wrote:
 On Sep 10, 2015, at 1:48 AM, Manuel Klimek >>> > wrote:
 
 @@ -179,11 +185,13 @@ public:
/// \param Directory The base directory used in the 
 FixedCompilationDatabase.
static FixedCompilationDatabase *loadFromCommandLine(int &Argc,
 const char *const 
 *Argv,
 -   Twine Directory = 
 ".");
 +   Twine Directory = 
 ".",
 +   Twine File = 
 Twine());
  
 A fixed compilation database returns the same command lines for all files, 
 thus having a file in the function seems strange.
>>> 
>>> Ah ok, thanks for clarifying.
>>> 
 
 What exactly is the use case? So far, the compilation database has been 
 designed for 2 use cases:
 1. for a file, get the compile commands; the user already knows the file, 
 no need to get the file
 2. get all compile commands; for that, we have the getAllFiles() method, 
 so a user can get all known files (for compilation databases that support 
 that), and then get the compile command line.
>>> 
>>> It’s #2, I want to get all compile commands. But it seems really strange to 
>>> me that the ‘file’ starts as a property of the compile command in the json 
>>> file but then it gets dropped and I need to do work to re-associate the 
>>> files with the compile commands again.
>>> 
>>> The JSON file format is one possible implementation for the 
>>> compilation-database interface. The FixedCompilationDatabase is another 
>>> one, that doesn't have any information on files.
>>>  
>>> I need to get a list of all the files and then for each one do a lookup to 
>>> get the associated commands. I then have to maintain this association 
>>> myself, passing a command along with its file separately or the structure 
>>> that keeps track of the association.
>>> 
>>> It seems simpler to me to include the file that was associated with the 
>>> command (if the compilation database supports that) along with the command, 
>>> is there a downside I’m missing ?
>>> 
>>> Well, to me, it's a design question - if it also makes sense to have a 
>>> CompileCommand without a file associated with it, putting the file in 
>>> there, while convenient, is a design smell.
>> 
>> It can be optional to communicate that it may not be there. Note that, IMO, 
>> having multiple files and compile commands for them is the overwhelmingly 
>> most common use of the compilation database.
>> 
>>> That said, I'm happy to be convinced that I'm wrong :) I guess I don't see 
>>> yet that keeping track of the files outside is more than one line of extra 
>>> code.
>> 
>> Not sure what one line this is, I have to declare a map and then populate 
>> it, no ? And to do it in c-index-test it would take way more that one line.
>> But it is also beyond populating a map, this has an effect on APIs using a 
>> CompileCommand. For example:
>> 
>>  void doSomethingWithCompileCommand(const CompileCommand &cmd); 
>> 
>> Ah it would be useful to know the file that this command was associated with:
>> 
>>  void doSomethingWithCompileCommand(const CompileCommand &cmd, StringRef 
>> filename); 
>> 
>> What do I have now ? This is a function taking a command and a string for 
>> the filename separately.
>> Is this flexibility useful ? Does it make sense to pass any filename there ? 
>> No there’s only one filename that the command was associated with so this 
>> ‘flexibility’ only increases the complexity of using the function. And this 
>> can propagate to other function’s callees.
>> This seems like a design smell to me.
>> 
>>> 
>>> Cheers,
>>> /Manuel
>>>  
>>> 
 
 Thoughts?
 /Manuel
 
 On Wed, Sep 9, 2015 at 9:36 PM Argyrios Kyrtzidis >>> > wrote:
 Hi,
 
 The attached patch exposes the ‘file’ entry in a compilation database 
 command, via the CompileCommand structure.
> 
>> ___

r247832 - [tooling] Add unit tests for change in r247468.

[Argyrios Kyrtzidis via cfe-commits]

Author: akirtzidis
Date: Wed Sep 16 13:28:42 2015
New Revision: 247832

URL: http://llvm.org/viewvc/llvm-project?rev=247832&view=rev
Log:
[tooling] Add unit tests for change in r247468.

Modified:
cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp

Modified: cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp?rev=247832&r1=247831&r2=247832&view=diff
==
--- cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp Wed Sep 16 13:28:42 
2015
@@ -98,8 +98,8 @@ TEST(JSONCompilationDatabase, GetAllComp
   StringRef FileName1("file1");
   StringRef Command1("command1");
   StringRef Directory2("//net/dir2");
-  StringRef FileName2("file1");
-  StringRef Command2("command1");
+  StringRef FileName2("file2");
+  StringRef Command2("command2");
 
   std::vector Commands = getAllCompileCommands(
   ("[{\"directory\":\"" + Directory1 + "\"," +
@@ -111,9 +111,11 @@ TEST(JSONCompilationDatabase, GetAllComp
   ErrorMessage);
   EXPECT_EQ(2U, Commands.size()) << ErrorMessage;
   EXPECT_EQ(Directory1, Commands[0].Directory) << ErrorMessage;
+  EXPECT_EQ(FileName1, Commands[0].Filename) << ErrorMessage;
   ASSERT_EQ(1u, Commands[0].CommandLine.size());
   EXPECT_EQ(Command1, Commands[0].CommandLine[0]) << ErrorMessage;
   EXPECT_EQ(Directory2, Commands[1].Directory) << ErrorMessage;
+  EXPECT_EQ(FileName2, Commands[1].Filename) << ErrorMessage;
   ASSERT_EQ(1u, Commands[1].CommandLine.size());
   EXPECT_EQ(Command2, Commands[1].CommandLine[0]) << ErrorMessage;
 }
@@ -427,14 +429,16 @@ TEST(FixedCompilationDatabase, ReturnsFi
   CommandLine.push_back("one");
   CommandLine.push_back("two");
   FixedCompilationDatabase Database(".", CommandLine);
+  StringRef FileName("source");
   std::vector Result =
-Database.getCompileCommands("source");
+Database.getCompileCommands(FileName);
   ASSERT_EQ(1ul, Result.size());
   std::vector ExpectedCommandLine(1, "clang-tool");
   ExpectedCommandLine.insert(ExpectedCommandLine.end(),
  CommandLine.begin(), CommandLine.end());
   ExpectedCommandLine.push_back("source");
   EXPECT_EQ(".", Result[0].Directory);
+  EXPECT_EQ(FileName, Result[0].Filename);
   EXPECT_EQ(ExpectedCommandLine, Result[0].CommandLine);
 }
 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

[Alexander Kornienko via cfe-commits]

An update: I didn't find a single bug with this check in a large codebase.
Turns out that it's rather useless. I'm inclined to kill it.

On Sun, Sep 13, 2015 at 11:22 AM, Alexander Kornienko 
wrote:

> I've also found a bunch of similar cases in our codebase, and I'm trying
> to figure out whether the pattern can be narrowed down to just dangerous
> cases. If we don't find a way to do so, we'll probably have to resort to
> "// NOLINT" to shut clang-tidy up.
> On 13 Sep 2015 10:52, "Kim Gräsman"  wrote:
>
>> Late to the party, but I wanted to ask: is there a way to indicate to
>> the checker that we really *did* mean sizeof()?
>>
>> I think I've stumbled over code in our code base that uses
>> sizeof(container) to report memory usage statistics and it seems
>> valid, so it'd be nice if this checker could be silenced on a
>> case-by-case basis.
>>
>> Thanks,
>> - Kim
>>
>> On Sat, Sep 12, 2015 at 12:09 AM, Alexander Kornienko via cfe-commits
>>  wrote:
>> > Indeed. But this has been fixed before I could get to it.
>> >
>> >
>> > On Thu, Sep 10, 2015 at 10:47 PM, Aaron Ballman via cfe-commits
>> >  wrote:
>> >>
>> >> aaron.ballman added a comment.
>> >>
>> >> This appears to have broken one of the bots:
>> >>
>> >> http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/15065
>> >>
>> >>
>> >> http://reviews.llvm.org/D12759
>> >>
>> >>
>> >>
>> >> ___
>> >> cfe-commits mailing list
>> >> cfe-commits@lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >
>> >
>> > ___
>> > cfe-commits mailing list
>> > cfe-commits@lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r247828 - [analyzer] Improved behavior if Clang was not found, part II

[Anton Yartsev via cfe-commits]

Author: ayartsev
Date: Wed Sep 16 13:12:15 2015
New Revision: 247828

URL: http://llvm.org/viewvc/llvm-project?rev=247828&view=rev
Log:
[analyzer] Improved behavior if Clang was not found, part II

- scan-build help: display 'Could not query Clang for the list of available 
checkers.' + the reason why it happened so if clang was not found.
- display requested/forced help in case of --use-analyzer=Xcode.

Modified:
cfe/trunk/tools/scan-build/scan-build

Modified: cfe/trunk/tools/scan-build/scan-build
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/scan-build/scan-build?rev=247828&r1=247827&r2=247828&view=diff
==
--- cfe/trunk/tools/scan-build/scan-build (original)
+++ cfe/trunk/tools/scan-build/scan-build Wed Sep 16 13:12:15 2015
@@ -1100,6 +1100,7 @@ sub RunBuildCommand {
 
 sub DisplayHelp {
 
+  my $ArgClangNotFoundErrMsg = shift;
 print < [build options]
 
@@ -1348,6 +1349,12 @@ ENDTEXT
 }
 close PS;
   }
+  else {
+print "  *** Could not query Clang for the list of available checkers.\n";
+if (defined  $ArgClangNotFoundErrMsg) {
+  print "  *** Reason: $ArgClangNotFoundErrMsg\n";
+}
+  }
 
 print 

[libcxx] r247827 - Add endianness configuration block for GCC.

[Dan Albert via cfe-commits]

Author: danalbert
Date: Wed Sep 16 13:10:47 2015
New Revision: 247827

URL: http://llvm.org/viewvc/llvm-project?rev=247827&view=rev
Log:
Add endianness configuration block for GCC.

Previously GCC using libc++ would just leak endian.h for every
include.

Modified:
libcxx/trunk/include/__config

Modified: libcxx/trunk/include/__config
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__config?rev=247827&r1=247826&r2=247827&view=diff
==
--- libcxx/trunk/include/__config (original)
+++ libcxx/trunk/include/__config Wed Sep 16 13:10:47 2015
@@ -64,6 +64,16 @@
 #endif  // __BIG_ENDIAN__
 #endif  // __BIG_ENDIAN__
 
+#ifdef __BYTE_ORDER__
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+#define _LIBCPP_LITTLE_ENDIAN 1
+#define _LIBCPP_BIG_ENDIAN 0
+#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#define _LIBCPP_LITTLE_ENDIAN 0
+#define _LIBCPP_BIG_ENDIAN 1
+#endif // __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#endif // __BYTE_ORDER__
+
 #ifdef __FreeBSD__
 # include 
 #  if _BYTE_ORDER == _LITTLE_ENDIAN


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9040: [analyzer] Make realloc(ptr, 0) handling equivalent to malloc(0).

[Антон Ярцев via cfe-commits]

ayartsev added a comment.

Ping.


http://reviews.llvm.org/D9040



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12889: [Static Analyzer] Generics Checker: When an ObjC method returns a specialized object, track it properly.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:680
@@ +679,3 @@
+return nullptr;
+
+  QualType ResultType = Method->getReturnType().substObjCTypeArgs(

zaks.anna wrote:
> From above:
>  QualType StaticResultType = Method->getReturnType();
> 
> You could do 
>  QualType ResultType = StaticResultType.substObjCTypeArgs(
>   C, TypeArgs, ObjCSubstitutionContext::Result);
Oh, I see. Sorry for misunderstanding.


http://reviews.llvm.org/D12889



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive

[Richard Smith via cfe-commits]

rsmith added a comment.

Sorry for the delay. I'm generally happy with the direction of this patch; I 
think this will substantially improve how we diagnose this set of problems.



Comment at: include/clang/Basic/DiagnosticParseKinds.td:1016
@@ -1015,2 +1015,3 @@
   "expected ';' after module name">;
+def err_missing_before_module_end : Error<"expected %0 at the end of module">;
 }

I would drop the "the" in this error.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7774
@@ -7771,1 +7773,3 @@
   "@import of module '%0' in implementation of '%1'; use #import">;
+def note_module_need_top_level : Note<"consider moving the import to top 
level">;
+def note_module_need_textual : Note<"consider marking the header as textual">;

We'll already have said something like:

  error: import of module 'foo' appears within class X
  note: class X begins here

There are two likely situations here: either the header was intended to be 
included textually, or the user forgot a `}` or similar and didn't expect for 
the import to be inside the class. Either way, this note is not useful (either 
they thought it *was* at the top level, or the note is not applicable).

How about dropping this note? The "begins here" note seems to have already 
covered the interesting case.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7775
@@ -7772,1 +7774,3 @@
+def note_module_need_top_level : Note<"consider moving the import to top 
level">;
+def note_module_need_textual : Note<"consider marking the header as textual">;
 }

Textual headers are not the common case, so "consider" seems too strong; how 
about weakening this to something like:

  "if this header is intended to be included textually, mark it 'textual' in 
the module map"


Comment at: lib/Parse/ParseDeclCXX.cpp:213
@@ -212,2 +212,3 @@
   if (index == Ident.size()) {
-while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
+while (tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
+   Tok.isNot(tok::eof)) {

Move this `try...` call to the end of the `&&` here and elsewhere; it's much 
more likely that we'll hit an `r_brace`.


Comment at: lib/Parse/Parser.cpp:2023-2024
@@ +2022,4 @@
+/// be found.
+/// \returns true if the recover was successful and parsing may be continued, 
or
+/// false if parser must bail out to top level and handle the token there.
+bool Parser::tryParseMisplacedModuleImport() {

This is the opposite of how all the other `try` functions behave: they return 
`true` if they consumed the thing they were asked to consume.


Comment at: lib/Parse/Parser.cpp:2025-2027
@@ +2024,5 @@
+/// false if parser must bail out to top level and handle the token there.
+bool Parser::tryParseMisplacedModuleImport() {
+  while (true) {
+switch (Tok.getKind()) {
+case tok::annot_module_end:

Refactor this as follows:

Rename this to `parseMisplacedModuleImport`. Add an inline 
`tryParseMisplacedModuleImport` to Parser.h, which checks if the current token 
is an EOM token. If it is, it calls this function. If it is not, it returns 
immediately.

The purpose of having these `try` functions with thin wrappers in the .h file 
is so that we can inline the cheap "are we looking at the right kind of token" 
check (and allow it to be combined with other nearby checks on the token kind).


Comment at: lib/Sema/SemaDecl.cpp:14361-14366
@@ -14359,6 +14360,8 @@
   if (!isa(DC)) {
-S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
-  << M->getFullModuleName() << DC;
-S.Diag(cast(DC)->getLocStart(),
-   diag::note_module_import_not_at_top_level)
-  << DC;
+if (DC->isNamespace()) {
+  S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
+<< M->getFullModuleName() << DC;
+  S.Diag(cast(DC)->getLocStart(),
+ diag::note_module_import_not_at_top_level) << DC;
+  S.Diag(ImportLoc, diag::note_module_need_top_level);
+} else {

It looks like this will currently produce repeated diagnostics for an import 
within multiple namespaces:

module.map:
  module Foo { module X { header "foo1.h" } module Y { header "foo2.h" } }

foo1.h:
  namespace A {
namespace B {
  #include "foo2.h"
}
  }

I believe this will diagnose:
1) import of "Foo.Y" within "B"
2) expected } at end of "B"
3) import of "Foo.Y" within "A"
4) expected } at end of "A"

I think we should treat this case as fatal too.


Comment at: lib/Sema/SemaDecl.cpp:14372
@@ +14371,3 @@
+ diag::note_module_import_not_at_top_level) << DC;
+  S.Diag(ImportLoc, diag::note_module_need_textual);
+}

I think we should only suggest changing the module map if our current module 
and the module of the included file are within the same top-level module. 

Re: [PATCH] D12901: [Static Analyzer] Intersecting ranges and 64 bit to 32 bit truncations causing "System is over constrained." assertions.

[pierre gousseau via cfe-commits]

pgousseau updated this revision to Diff 34900.
pgousseau added a comment.

Added some comments.


http://reviews.llvm.org/D12901

Files:
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
  lib/StaticAnalyzer/Core/SimpleConstraintManager.h
  test/Analysis/range_casts.c

Index: test/Analysis/range_casts.c
===
--- /dev/null
+++ test/Analysis/range_casts.c
@@ -0,0 +1,147 @@
+// This test checks that intersecting ranges does not cause 'system is over constrained' assertions in the case of eg: 32 bits unsigned integers getting their range from 64 bits signed integers.
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-store=region -verify %s
+
+void clang_analyzer_warnIfReached();
+
+void f1(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index + 1 == 0) // because of foo range, index is in range [0; UINT_MAX]
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f2(unsigned long foo)
+{
+  int index = -1;
+  if (index < foo) index = foo; // index equals ULONG_MAX
+  if (index + 1 == 0)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  else
+clang_analyzer_warnIfReached(); // no-warning
+}
+
+void f3(unsigned long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index + 1 == 0)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f4(long foo)
+{
+  int index = -1;
+  if (index < foo) index = foo;
+  if (index + 1 == 0)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f5(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index == -1)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f6(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index == -1)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f7(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index - 1 == 0)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f8(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index + 1L == 0L)
+clang_analyzer_warnIfReached(); // no-warning
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f9(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index - 1L == 0L)
+// FIXME: should be reached
+clang_analyzer_warnIfReached(); // no-warning
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f10(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index + 1 == 0L)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f11(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index + 1UL == 0L)
+clang_analyzer_warnIfReached(); // no-warning
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f12(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  if (index - 1UL == 0L)
+// FIXME: should be reached
+clang_analyzer_warnIfReached(); // no-warning
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f13(int foo)
+{
+  unsigned short index = -1;
+  if (index < foo) index = foo;
+  if (index + 1 == 0)
+clang_analyzer_warnIfReached(); // no-warning
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void f14(long foo)
+{
+  unsigned index = -1;
+  if (index < foo) index = foo;
+  long bar = foo;
+  if (index + 1 == 0)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  else
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
Index: lib/StaticAnalyzer/Core/SimpleConstraintManager.h
===
--- lib/StaticAnalyzer/Core/SimpleConstraintManager.h
+++ lib/StaticAnalyzer/Core/SimpleConstraintManager.h
@@ -53,11 +53,13 @@
   // operation for the method being invoked.
   virtual ProgramStateRef assumeSymNE(ProgramStateRef state, SymbolRef sym,
  const llvm::APSInt& V,
- const llvm::

[clang-tools-extra] r247812 - [clang-tidy] Ignore spaces in -checks=

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Wed Sep 16 11:16:53 2015
New Revision: 247812

URL: http://llvm.org/viewvc/llvm-project?rev=247812&view=rev
Log:
[clang-tidy] Ignore spaces in -checks=

Modified:
clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp

Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp?rev=247812&r1=247811&r2=247812&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp Wed Sep 
16 11:16:53 2015
@@ -130,7 +130,7 @@ static bool ConsumeNegativeIndicator(Str
 // Converts first glob from the comma-separated list of globs to Regex and
 // removes it and the trailing comma from the GlobList.
 static llvm::Regex ConsumeGlob(StringRef &GlobList) {
-  StringRef Glob = GlobList.substr(0, GlobList.find(','));
+  StringRef Glob = GlobList.substr(0, GlobList.find(',')).trim();
   GlobList = GlobList.substr(Glob.size() + 1);
   llvm::SmallString<128> RegexText("^");
   StringRef MetaChars("()^$|*+?.[]\\{}");


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12839: Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[Aaron Ballman via cfe-commits]

On Wed, Sep 16, 2015 at 11:41 AM, Alexander Kornienko  wrote:
> On Wed, Sep 16, 2015 at 5:15 PM, Aaron Ballman 
> wrote:
>>
>> Okay, I'm sold on this being a clang-tidy check instead, thank you
>> both for weighing in!
>>
>> I would love to see a check that covers more than just constructor
>> initialization contexts, which suggests a separate checker from
>> misc-move-constructor-init so that we don't have the same general idea
>> split over two checkers. For instance, I would love to see this sort
>> of code flagged along with the cases you've identified:
>>
>> struct SomethingBig { /* Stuff */ };
>> struct S {
>>   std::vector V;
>> };
>>
>> void f(S &s) {
>>   std::vector V;
>>   // Fill out a bunch of elements in V
>>   s.V = V; // suggest std::move()
>> }
>
>
> Detecting that the local variable is not used after something is initialized
> with it will require analysis of the control flow graph in the general case,
> which I'm not sure is currently possible in a clang-tidy check
> (specifically, I'm not sure whether we can make Sema available to checks).

Oye, the number of times I've wished Sema was exposed to clang-tidy is
*huge*. But this is a good point. I'll have to think on it further.

~Aaron

>
>>
>> Btw, I'm not suggesting you have to implement this sort of
>> functionality as part of your patch (but it would be awesome if you
>> were able to!). I'm only thinking about separation of concerns for
>> this.
>>
>> ~Aaron
>>
>> On Wed, Sep 16, 2015 at 3:44 AM, Alexander Kornienko 
>> wrote:
>> > I also think this suits better for a clang-tidy check, but for a
>> > different
>> > reason: adding `std::move` calls can require adding `#include
>> > `,
>> > which is fine for a clang-tidy check (and we have facilities for that),
>> > but
>> > it's a questionable functionality for a compiler diagnostic.
>> >
>> > On 16 Sep 2015 09:20, "Richard Smith"  wrote:
>> >>
>> >> On Tue, Sep 15, 2015 at 12:50 PM, Aaron Ballman
>> >> 
>> >> wrote:
>> >>>
>> >>> aaron.ballman added reviewers: rtrieu, rsmith.
>> >>> aaron.ballman added a comment.
>> >>>
>> >>> There is a -Wpessimizing-move frontend warning that Richard added not
>> >>> too
>> >>> long ago, which tells the user to remove calls to std::move() because
>> >>> they
>> >>> pessimize the code. The new functionality you are looking to add is
>> >>> basically the opposite: it tells the user to add std::move() because
>> >>> the
>> >>> code is currently pessimized due to copies. Given how general that
>> >>> concept
>> >>> is (it doesn't just appertain to constructor initializations), I
>> >>> wonder if
>> >>> this makes more sense as a frontend warning like -Wpessimizing-copy.
>> >>>
>> >>> Richard (both of you), what do you think?
>> >>
>> >>
>> >> I think this is in the grey area between what's appropriate for
>> >> clang-tidy
>> >> and what's appropriate as a warning, where both options have merit; the
>> >> same
>> >> is true with -Wpessimizing-move. I think the difference between the two
>> >> cases is that -Wpessimizing-move detects a case where you wrote
>> >> something
>> >> that doesn't do what (we think) you meant, whereas this check detects a
>> >> case
>> >> where you /didn't/ write something that (we think) would make your code
>> >> better (even though both changes have similar effects, of safely
>> >> turning a
>> >> copy into a move or a move into an elided move). It's a fine line, but
>> >> for
>> >> me that nudges -Wpessimizing-move into a warning, and this check into
>> >> clang-tidy territory.
>> >>
>> >>> ~Aaron
>> >>>
>> >>>
>> >>> 
>> >>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:32
>> >>> @@ +31,3 @@
>> >>> +  // excessive copying, we'll warn on them.
>> >>> +  if (Node->isDependentType()) {
>> >>> +return false;
>> >>> 
>> >>> Elide braces, here and elsewhere.
>> >>>
>> >>> 
>> >>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:36
>> >>> @@ +35,3 @@
>> >>> +  // Ignore trivially copyable types.
>> >>> +  if (Node->isScalarType() ||
>> >>> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
>> >>> 
>> >>> Can turn this into a return statement directly instead of an if.
>> >>>
>> >>> 
>> >>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38
>> >>> @@ +37,3 @@
>> >>> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
>> >>> +  classHasTrivialCopyAndDestroy(Node)) {
>> >>> +return false;
>> >>> 
>> >>> Why do you need classHasTrivialCopyAndDestroy() when you're already
>> >>> checking if it's a trivially copyable type?
>> >>>
>> >>> 
>> >>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44
>> >>> @@ +43,3 @@
>> >>> +
>> >>> +int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
>> >>> + const CXXConstructorDecl
>> >>> &ConstructorDecl,
>> >>> 
>> >>> Should return unsigned, please.
>> >>>

Re: [PATCH] D12839: Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[Alexander Kornienko via cfe-commits]

On Wed, Sep 16, 2015 at 5:15 PM, Aaron Ballman 
wrote:

> Okay, I'm sold on this being a clang-tidy check instead, thank you
> both for weighing in!
>
> I would love to see a check that covers more than just constructor
> initialization contexts, which suggests a separate checker from
> misc-move-constructor-init so that we don't have the same general idea
> split over two checkers. For instance, I would love to see this sort
> of code flagged along with the cases you've identified:
>
> struct SomethingBig { /* Stuff */ };
> struct S {
>   std::vector V;
> };
>
> void f(S &s) {
>   std::vector V;
>   // Fill out a bunch of elements in V
>   s.V = V; // suggest std::move()
> }
>

Detecting that the local variable is not used after something is
initialized with it will require analysis of the control flow graph in the
general case, which I'm not sure is currently possible in a clang-tidy
check (specifically, I'm not sure whether we can make Sema available to
checks).


> Btw, I'm not suggesting you have to implement this sort of
> functionality as part of your patch (but it would be awesome if you
> were able to!). I'm only thinking about separation of concerns for
> this.
>
> ~Aaron
>
> On Wed, Sep 16, 2015 at 3:44 AM, Alexander Kornienko 
> wrote:
> > I also think this suits better for a clang-tidy check, but for a
> different
> > reason: adding `std::move` calls can require adding `#include `,
> > which is fine for a clang-tidy check (and we have facilities for that),
> but
> > it's a questionable functionality for a compiler diagnostic.
> >
> > On 16 Sep 2015 09:20, "Richard Smith"  wrote:
> >>
> >> On Tue, Sep 15, 2015 at 12:50 PM, Aaron Ballman <
> aaron.ball...@gmail.com>
> >> wrote:
> >>>
> >>> aaron.ballman added reviewers: rtrieu, rsmith.
> >>> aaron.ballman added a comment.
> >>>
> >>> There is a -Wpessimizing-move frontend warning that Richard added not
> too
> >>> long ago, which tells the user to remove calls to std::move() because
> they
> >>> pessimize the code. The new functionality you are looking to add is
> >>> basically the opposite: it tells the user to add std::move() because
> the
> >>> code is currently pessimized due to copies. Given how general that
> concept
> >>> is (it doesn't just appertain to constructor initializations), I
> wonder if
> >>> this makes more sense as a frontend warning like -Wpessimizing-copy.
> >>>
> >>> Richard (both of you), what do you think?
> >>
> >>
> >> I think this is in the grey area between what's appropriate for
> clang-tidy
> >> and what's appropriate as a warning, where both options have merit; the
> same
> >> is true with -Wpessimizing-move. I think the difference between the two
> >> cases is that -Wpessimizing-move detects a case where you wrote
> something
> >> that doesn't do what (we think) you meant, whereas this check detects a
> case
> >> where you /didn't/ write something that (we think) would make your code
> >> better (even though both changes have similar effects, of safely
> turning a
> >> copy into a move or a move into an elided move). It's a fine line, but
> for
> >> me that nudges -Wpessimizing-move into a warning, and this check into
> >> clang-tidy territory.
> >>
> >>> ~Aaron
> >>>
> >>>
> >>> 
> >>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:32
> >>> @@ +31,3 @@
> >>> +  // excessive copying, we'll warn on them.
> >>> +  if (Node->isDependentType()) {
> >>> +return false;
> >>> 
> >>> Elide braces, here and elsewhere.
> >>>
> >>> 
> >>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:36
> >>> @@ +35,3 @@
> >>> +  // Ignore trivially copyable types.
> >>> +  if (Node->isScalarType() ||
> >>> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
> >>> 
> >>> Can turn this into a return statement directly instead of an if.
> >>>
> >>> 
> >>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38
> >>> @@ +37,3 @@
> >>> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
> >>> +  classHasTrivialCopyAndDestroy(Node)) {
> >>> +return false;
> >>> 
> >>> Why do you need classHasTrivialCopyAndDestroy() when you're already
> >>> checking if it's a trivially copyable type?
> >>>
> >>> 
> >>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44
> >>> @@ +43,3 @@
> >>> +
> >>> +int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
> >>> + const CXXConstructorDecl
> >>> &ConstructorDecl,
> >>> 
> >>> Should return unsigned, please.
> >>>
> >>> 
> >>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:50
> >>> @@ +49,3 @@
> >>> +
> findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam);
> >>> +  auto Matches = match(AllDeclRefs, *ConstructorDecl.getBody(),
> >>> Context);
> >>> +  Occurrences += Matches.size();
> >>> 
> >>> You don't actually need Matches,

Re: [PATCH] D12839: Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[Aaron Ballman via cfe-commits]

Okay, I'm sold on this being a clang-tidy check instead, thank you
both for weighing in!

I would love to see a check that covers more than just constructor
initialization contexts, which suggests a separate checker from
misc-move-constructor-init so that we don't have the same general idea
split over two checkers. For instance, I would love to see this sort
of code flagged along with the cases you've identified:

struct SomethingBig { /* Stuff */ };
struct S {
  std::vector V;
};

void f(S &s) {
  std::vector V;
  // Fill out a bunch of elements in V
  s.V = V; // suggest std::move()
}

Btw, I'm not suggesting you have to implement this sort of
functionality as part of your patch (but it would be awesome if you
were able to!). I'm only thinking about separation of concerns for
this.

~Aaron

On Wed, Sep 16, 2015 at 3:44 AM, Alexander Kornienko  wrote:
> I also think this suits better for a clang-tidy check, but for a different
> reason: adding `std::move` calls can require adding `#include `,
> which is fine for a clang-tidy check (and we have facilities for that), but
> it's a questionable functionality for a compiler diagnostic.
>
> On 16 Sep 2015 09:20, "Richard Smith"  wrote:
>>
>> On Tue, Sep 15, 2015 at 12:50 PM, Aaron Ballman 
>> wrote:
>>>
>>> aaron.ballman added reviewers: rtrieu, rsmith.
>>> aaron.ballman added a comment.
>>>
>>> There is a -Wpessimizing-move frontend warning that Richard added not too
>>> long ago, which tells the user to remove calls to std::move() because they
>>> pessimize the code. The new functionality you are looking to add is
>>> basically the opposite: it tells the user to add std::move() because the
>>> code is currently pessimized due to copies. Given how general that concept
>>> is (it doesn't just appertain to constructor initializations), I wonder if
>>> this makes more sense as a frontend warning like -Wpessimizing-copy.
>>>
>>> Richard (both of you), what do you think?
>>
>>
>> I think this is in the grey area between what's appropriate for clang-tidy
>> and what's appropriate as a warning, where both options have merit; the same
>> is true with -Wpessimizing-move. I think the difference between the two
>> cases is that -Wpessimizing-move detects a case where you wrote something
>> that doesn't do what (we think) you meant, whereas this check detects a case
>> where you /didn't/ write something that (we think) would make your code
>> better (even though both changes have similar effects, of safely turning a
>> copy into a move or a move into an elided move). It's a fine line, but for
>> me that nudges -Wpessimizing-move into a warning, and this check into
>> clang-tidy territory.
>>
>>> ~Aaron
>>>
>>>
>>> 
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:32
>>> @@ +31,3 @@
>>> +  // excessive copying, we'll warn on them.
>>> +  if (Node->isDependentType()) {
>>> +return false;
>>> 
>>> Elide braces, here and elsewhere.
>>>
>>> 
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:36
>>> @@ +35,3 @@
>>> +  // Ignore trivially copyable types.
>>> +  if (Node->isScalarType() ||
>>> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
>>> 
>>> Can turn this into a return statement directly instead of an if.
>>>
>>> 
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38
>>> @@ +37,3 @@
>>> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
>>> +  classHasTrivialCopyAndDestroy(Node)) {
>>> +return false;
>>> 
>>> Why do you need classHasTrivialCopyAndDestroy() when you're already
>>> checking if it's a trivially copyable type?
>>>
>>> 
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44
>>> @@ +43,3 @@
>>> +
>>> +int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
>>> + const CXXConstructorDecl
>>> &ConstructorDecl,
>>> 
>>> Should return unsigned, please.
>>>
>>> 
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:50
>>> @@ +49,3 @@
>>> +  findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam);
>>> +  auto Matches = match(AllDeclRefs, *ConstructorDecl.getBody(),
>>> Context);
>>> +  Occurrences += Matches.size();
>>> 
>>> You don't actually need Matches, you can call match().size() instead.
>>>
>>> 
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:52
>>> @@ +51,3 @@
>>> +  Occurrences += Matches.size();
>>> +  for (const auto* Initializer : ConstructorDecl.inits()) {
>>> +Matches = match(AllDeclRefs, *Initializer->getInit(), Context);
>>> 
>>> Should be *Initializer instead of * Initializer.
>>>
>>> 
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120
>>> @@ +119,3 @@
>>> +  }
>>> +  diag(InitArg->getLocStart(), "value parameter can be moved to avoid
>>> copy.");
>>> +}
>>> 

r247808 - [sanitizers] Enable memory sanitizer on clang

[Adhemerval Zanella via cfe-commits]

Author: azanella
Date: Wed Sep 16 10:11:21 2015
New Revision: 247808

URL: http://llvm.org/viewvc/llvm-project?rev=247808&view=rev
Log:
[sanitizers] Enable memory sanitizer on clang

This patch enables MSan for aarch64/linux

Modified:
cfe/trunk/lib/Driver/ToolChains.cpp

Modified: cfe/trunk/lib/Driver/ToolChains.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains.cpp?rev=247808&r1=247807&r2=247808&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains.cpp Wed Sep 16 10:11:21 2015
@@ -3761,7 +3761,7 @@ SanitizerMask Linux::getSupportedSanitiz
 Res |= SanitizerKind::Leak;
   if (IsX86_64 || IsMIPS64 || IsAArch64)
 Res |= SanitizerKind::Thread;
-  if (IsX86_64 || IsMIPS64 || IsPowerPC64)
+  if (IsX86_64 || IsMIPS64 || IsPowerPC64 || IsAArch64)
 Res |= SanitizerKind::Memory;
   if (IsX86 || IsX86_64) {
 Res |= SanitizerKind::Function;


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12903: Allow the -load option in the driver and pass it through to -cc1

[Renato Golin via cfe-commits]

rengolin added a comment.

In http://reviews.llvm.org/D12903#247075, @john.brawn wrote:

> The use-case is loading llvm plugins that add optimization passes via the 
> PassManagerBuilder::addGlobalExtension mechanism (i.e. just loading the 
> plugin is enough and you don't have to add anything extra to the 
> command-line). Currently you have to do clang -Xclang -load -Xclang 
> plugin.so, and it would be nicer to be able to do clang -load plugin.so.


Right, I'm not familiar with that extension, so I can't really give an opinion. 
Maybe some of the Clang folks could help? Feel free to find the appropriate 
people and add them as reviewers, so they're aware of it.

> I'm not sure how to best add a test for this. Maybe I should add another pass 
> to llvm that's built as a plugin like llvm/lib/Transforms/Hello but that uses 
> PassManagerBuilder::addGlobalExtension and have a clang test that runs that?


Well, since this is Clang, I think just a simple -### test checking that the 
flag was passed correctly down to CC1 would be enough, as I assume there are 
tests already for the flag in CC1 mode.


Repository:
  rL LLVM

http://reviews.llvm.org/D12903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r247806 - [clang-tidy] google-runtime-int: made the matcher more restricting, added a test for a false positive

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Wed Sep 16 10:08:46 2015
New Revision: 247806

URL: http://llvm.org/viewvc/llvm-project?rev=247806&view=rev
Log:
[clang-tidy] google-runtime-int: made the matcher more restricting, added a 
test for a false positive

This should be NFC.

Modified:
clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/google-runtime-int.cpp

Modified: clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp?rev=247806&r1=247805&r2=247806&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp Wed Sep 16 
10:08:46 2015
@@ -36,7 +36,7 @@ void IntegerTypesCheck::storeOptions(Cla
 void IntegerTypesCheck::registerMatchers(MatchFinder *Finder) {
   // Find all TypeLocs. The relevant Style Guide rule only applies to C++.
   if (getLangOpts().CPlusPlus)
-Finder->addMatcher(typeLoc().bind("tl"), this);
+Finder->addMatcher(typeLoc(loc(isInteger())).bind("tl"), this);
 }
 
 void IntegerTypesCheck::check(const MatchFinder::MatchResult &Result) {

Modified: clang-tools-extra/trunk/test/clang-tidy/google-runtime-int.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/google-runtime-int.cpp?rev=247806&r1=247805&r2=247806&view=diff
==
--- clang-tools-extra/trunk/test/clang-tidy/google-runtime-int.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/google-runtime-int.cpp Wed Sep 16 
10:08:46 2015
@@ -49,6 +49,7 @@ short bar(const short, unsigned short) {
 
   tmpl();
 // CHECK-MESSAGES: [[@LINE-1]]:8: warning: consider replacing 'short' with 
'int16'
+  return 0;
 }
 
 void p(unsigned short port);
@@ -57,3 +58,10 @@ void qux() {
   short port;
 // CHECK-MESSAGES: [[@LINE-1]]:3: warning: consider replacing 'short' with 
'int16'
 }
+
+// FIXME: This shouldn't warn, as UD-literal operators require one of a handful
+// of types as an argument.
+struct some_value {};
+constexpr some_value operator"" _some_literal(unsigned long long int i);
+// CHECK-MESSAGES: [[@LINE-1]]:47: warning: consider replacing 'unsigned long 
long'
+


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12906: [RFC] Bug identification("issue_hash") change for CmpRuns.py

[Honggyu Kim via cfe-commits]

honggyu.kim added a comment.

I think we can enhance the bug comparison method one by one rather than making 
a big change at once.
The point of this patch is to make "issue_hash" field in plist more robust 
while still using existing CmpRuns.py for comparison.

I would like to hear any kind of comment.
Thank you.


http://reviews.llvm.org/D12906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10305: [Clang Static Analyzer] Bug identification

[Honggyu Kim via cfe-commits]

honggyu.kim added a comment.

In http://reviews.llvm.org/D10305#242159, @zaks.anna wrote:

> honggyu.kim,
>
> Uniquing HTML reports is out of the scope of this patch and should be 
> discussed elsewhere (either send a design idea to cfe-dev, send a patch for 
> review, or file a bugzilla request).
>
> I agree that this patch is a definite improvement to issue identification; 
> however, as I mentioned earlier, it needs tests.


Hi Anna,

I have created another patch based on Babati's work in order to use the 
existing infrastructure(CmpRuns.py).
Please have a look at http://reviews.llvm.org/D12906 and give me your feedback.
Thank you very much for your comment.


http://reviews.llvm.org/D10305



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12547: Add support for function attribute "disable_tail_calls"

[Aaron Ballman via cfe-commits]

On Wed, Sep 16, 2015 at 12:47 AM, Akira Hatanaka  wrote:
> ahatanak added inline comments.
>
> 
> Comment at: include/clang/Basic/Attr.td:894
> @@ -893,1 +893,3 @@
>
> +def DisableTailCalls : InheritableAttr {
> +  let Spellings = [GNU<"disable_tail_calls">,
> 
> aaron.ballman wrote:
>> Pardon me if this is obvious, but -- are there times when you would want to 
>> disable tail calls but leave other optimizations in place? I'm trying to 
>> understand why these attributes are orthogonal.
> Yes, that's correct. The new function attribute we are adding shouldn't 
> disable other optimizations that are normally run at -O2 or -O3.

Okay -- with this and your other explanations, I now understand why
this attribute is orthogonal to optnone. Thanks!

> 
> Comment at: include/clang/Basic/Attr.td:896
> @@ +895,3 @@
> +  let Spellings = [GNU<"disable_tail_calls">,
> +   CXX11<"clang", "disable_tail_calls">];
> +  let Subjects = SubjectList<[Function, ObjCMethod]>;
> 
> aaron.ballman wrote:
>> Should this attribute be plural? Are there multiple tail calls within a 
>> single method that can be optimized away?
> I named it after the existing IR function attribute and I'm guessing the 
> plural name was chosen because functions can have multiple tail call sites. I 
> think the singular name is better if we want this attribute to mean "disable 
> tail call optimization".

I'm holding off on having an opinion on the name until I have an idea
as to the other planned attribute's name, but I basically think the
plural is reasonable if the purpose is to disable TCO on the functions
called within the marked function. But we may want to consider making
the name more obvious. The majority of the attributes we support
appertain to the behavior of the declaration itself, not to entities
*within* the declaration. However, it seems like no_sanitize,
no_sanitize_thread, et al are similar concepts. Perhaps we want this
to be named "no_tail_calls" instead? Just something to think about.

> 
> Comment at: include/clang/Basic/AttrDocs.td:1619
> @@ +1618,3 @@
> +  let Content = [{
> +Use ``__attribute__((disable_tail_calls)))`` to instruct the backend not to 
> do tail call optimization.
> +  }];
> 
> aaron.ballman wrote:
>> I would avoid using the exact syntax here because this is a GNU and C++ 
>> attribute. Could just say:
>>
>> The ``disable_tail_calls`` attribute instructs the backend to not perform 
>> tail call optimization.
> OK, done.
>
> 
> Comment at: lib/Sema/SemaDeclAttr.cpp:4882
> @@ +4881,3 @@
> +  case AttributeList::AT_DisableTailCalls:
> +handleSimpleAttribute(S, D, Attr);
> +break;
> 
> aaron.ballman wrote:
>> Okay, that makes sense. I can contrive examples of noreturn where TCO could 
>> happen, it just took me a while to think of them. ;-)
>>
>> What about other semantic checks, like warning the user about disabling TCO 
>> when TCO could never be enabled in the first place? Can you disable TCO for 
>> functions that are marked __attribute__((naked))? What about returns_twice?
>>
>> Unfortunately, we have a lot of attributes for which we've yet to write 
>> documentation, so you may need to look through Attr.td.
> Since "naked" allows only inline assembly statements, it should be an error 
> to have disable-tail-calls and naked on the same function. I made changes in 
> Sema to detect that case.

__declspec(naked) behaves differently than __attribute__((naked)), but
I'm not certain whether that changes the reasoning
(https://msdn.microsoft.com/en-us/library/h5w10wxs.aspx).

> My understanding is that you can't do tail call optimization on call sites of 
> a function that calls a "return_twice" function. However, attaching 
> "return_twice" on the calling function doesn't block tail call optimizations 
> on the call sites inside the function.

That makes sense to me.

> I didn't find any other attributes that seemed incompatible with 
> disable-tail-calls.

If a user finds something, we can handle it at that time. Thank you
for looking into this!

> Index: include/clang/Basic/AttrDocs.td
> ===
> --- include/clang/Basic/AttrDocs.td
> +++ include/clang/Basic/AttrDocs.td
> @@ -1612,3 +1612,10 @@
>  arguments, with arbitrary offsets.
>}];
>  }
> +
> +def DisableTailCallsDocs : Documentation {
> +  let Category = DocCatFunction;
> +  let Content = [{
> +The ``disable_tail_calls`` attribute instructs the backend to not perform 
> tail call optimization.

The documentation should be updated to clarify the semantics of the
attribute. Right now it's ambiguous as to which TCOs are disabled.

> +  }];
> +}
> Index: lib/Sema/SemaDeclAttr.cpp
> ===
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -1696,6 +1696,18 @@
>   

Re: [PATCH] D12906: [RFC] Bug identification("issue_hash") change for CmpRuns.py

[Honggyu Kim via cfe-commits]

honggyu.kim added a comment.

issue_hash is generated with hash value rather than line offset from function 
as below:

  issue_hash765b04830932fef5631bf3c48c4d2db2


http://reviews.llvm.org/D12906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12906: [RFC] Bug identification("issue_hash") change for CmpRuns.py

[Honggyu Kim via cfe-commits]

honggyu.kim created this revision.
honggyu.kim added reviewers: jordan_rose, krememek, zaks.anna, danielmarjamaki, 
babati, dcoughlin.
honggyu.kim added subscribers: cfe-commits, phillip.power, seaneveson, 
j.trofimovich, hk.kang, eszasip, dkrupp, o.gyorgy, xazax.hun, premalatha_mvs.

This patch brings bug identification method from D10305 to the existing 
infrastructure.
By applying this patch, two different bug reports can be compared with existing 
CmpRuns.py.

Currently, "issue_hash" in plist file is just line offset from the beginning of 
function.
But it even cannot distinguish those kind of simple cases that are completely 
different bugs.

BUG 1. garbage return value
```
1 int main()
2 {
3   int a;
4   return a;
5 }

test.c:4:3: warning: Undefined or garbage value returned to caller
  return a;
  ^~~~
```
BUG 2. garbage assignment
```
1 int main()
2 {
3   int a;
4   int b = a;
5   return b;
6 }

test.c:4:3: warning: Assigned value is garbage or undefined
  int b = a;
  ^   ~
```

Moreover, The following case are regarded as different bugs when it is compared 
with BUG 1.

BUG 3. a single line of comment is added based on BUG 1 code.
```
1 int main()
2 {
3   // main function
4   int a;
5   return a;
6 }

test.c:5:3: warning: Undefined or garbage value returned to caller
  return a;
  ^~~~
```
The comparison result is as follows:
```
REMOVED: 'test.c:4:3, Logic error: Undefined or garbage value returned to 
caller'
ADDED: 'test.c:5:3, Logic error: Undefined or garbage value returned to caller'
TOTAL REPORTS: 1
TOTAL DIFFERENCES: 2
```

This patch brought the bug identification method and code from D10305, and it 
generates the "issue_hash" with the following information:
1. column number
2. source line string after removing whitespace
3. bug type (bug message)

This patch is not the final solution, but it enhances "issue_hash" to 
distinguish such kind of cases by generating stronger hash value.

http://reviews.llvm.org/D12906

Files:
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -22,6 +22,11 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/LineIterator.h"
+#include "clang/AST/ASTContext.h"
+#include "llvm/Support/MD5.h"
+#include 
+
 using namespace clang;
 using namespace ento;
 using namespace markup;
@@ -285,6 +290,57 @@
   }
 }
 
+static std::string GetNthLineOfFile(llvm::MemoryBuffer *Buffer, int Line) {
+  if (!Buffer)
+return "";
+
+  llvm::line_iterator LI(*Buffer, false);
+  for (; !LI.is_at_eof() && LI.line_number() != Line; ++LI)
+;
+
+  return LI->str();
+}
+
+static std::string NormalizeLine(const SourceManager *SM, FullSourceLoc &L,
+ const Decl *D) {
+  static const std::string whitespaces = " \t\n";
+
+  const LangOptions &Opts = D->getASTContext().getLangOpts();
+  std::string str = GetNthLineOfFile(SM->getBuffer(L.getFileID(), L), L.getExpansionLineNumber());
+  unsigned col = str.find_first_not_of(whitespaces);
+
+  SourceLocation StartOfLine = SM->translateLineCol(SM->getFileID(L), L.getExpansionLineNumber(), col);
+  llvm::MemoryBuffer *Buffer = SM->getBuffer(SM->getFileID(StartOfLine), StartOfLine);
+  if (!Buffer) return {};
+
+  const char *BufferPos = SM->getCharacterData(StartOfLine);
+
+  Token Token;
+  Lexer Lexer(SM->getLocForStartOfFile(SM->getFileID(StartOfLine)), Opts,
+  Buffer->getBufferStart(), BufferPos, Buffer->getBufferEnd());
+
+  size_t nextStart = 0;
+  std::ostringstream lineBuff;
+  while (!Lexer.LexFromRawLexer(Token) && nextStart < 2) {
+if (Token.isAtStartOfLine() && nextStart++ > 0) continue;
+lineBuff << std::string(SM->getCharacterData(Token.getLocation()), Token.getLength());
+  }
+
+  return lineBuff.str();
+}
+
+static llvm::SmallString<32> GetHashOfContent(StringRef Content) {
+  llvm::MD5 Hash;
+  llvm::MD5::MD5Result MD5Res;
+  llvm::SmallString<32> Res;
+
+  Hash.update(Content);
+  Hash.final(MD5Res);
+  llvm::MD5::stringifyResult(MD5Res, Res);
+
+  return Res;
+}
+
 void PlistDiagnostics::FlushDiagnosticsImpl(
 std::vector &Diags,
 FilesMade *filesMade) {
@@ -420,9 +476,12 @@
   EmitString(o, declName) << '\n';
 }
 
-// Output the bug hash for issue unique-ing. Currently, it's just an
-// offset from the beginning of the function.
-if (const Stmt *Body = DeclWithIssue->getBody()) {
+// Output the bug hash for issue unique-ing.
+// Currently, it contains the following information:
+//   1. column number
+//   2. source line string after removing whitespace
+//   3. bug type
+if (DeclWithIssue->getBody()) {
 
 

Re: [PATCH] D12903: Allow the -load option in the driver and pass it through to -cc1

[John Brawn via cfe-commits]

john.brawn added a comment.

In http://reviews.llvm.org/D12903#247055, @rengolin wrote:

> Hi John,
>
> Can you expand a bit more on why you need this, what's the use case, and 
> hopefully attach a test for them?


The use-case is loading llvm plugins that add optimization passes via the 
PassManagerBuilder::addGlobalExtension mechanism (i.e. just loading the plugin 
is enough and you don't have to add anything extra to the command-line). 
Currently you have to do clang -Xclang -load -Xclang plugin.so, and it would be 
nicer to be able to do clang -load plugin.so.

I'm not sure how to best add a test for this. Maybe I should add another pass 
to llvm that's built as a plugin like llvm/lib/Transforms/Hello but that uses 
PassManagerBuilder::addGlobalExtension and have a clang test that runs that?


Repository:
  rL LLVM

http://reviews.llvm.org/D12903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12547: Add support for function attribute "disable_tail_calls"

[Aaron Ballman via cfe-commits]

On Wed, Sep 16, 2015 at 12:46 AM, Akira Hatanaka  wrote:
> ahatanak updated this revision to Diff 34872.
> ahatanak added a comment.
>
> Sorry for the delay in my response.
>
> I had discussions with the users who requested this feature and it turns out 
> they were asking for two different kinds of attributes. They are both needed 
> to disable tail call optimization to preserve the stack frame but they differ 
> in which stack frame they are trying to preserve. The attribute in this patch 
> disables optimization on the call sites inside a function to preserve the 
> stack frame of the function. The other attribute disables tail call *to* the 
> function and therefore preserves the stack frame of the calling function.

Ah, thank you for that clarification.

> I'll send a patch for the other attribute shortly.

Do you have a guess as to what the name of that other attribute will
be? I just want to make sure we pick clear names, and I could see the
current proposed attribute name being used for either situation.

~Aaron

>
>
> http://reviews.llvm.org/D12547
>
> Files:
>   include/clang/Basic/Attr.td
>   include/clang/Basic/AttrDocs.td
>   lib/CodeGen/CGCall.cpp
>   lib/Sema/SemaDeclAttr.cpp
>   test/CodeGen/attr-disable-tail-calls.c
>   test/Sema/disable-tail-calls-attr.c
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r247803 - [clang-tidy] Make google-runtime-int configurable.

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Wed Sep 16 09:36:22 2015
New Revision: 247803

URL: http://llvm.org/viewvc/llvm-project?rev=247803&view=rev
Log:
[clang-tidy] Make google-runtime-int configurable.

Added:
clang-tools-extra/trunk/test/clang-tidy/google-runtime-int-std.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp
clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.h

Modified: clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp?rev=247803&r1=247802&r2=247803&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp Wed Sep 16 
09:36:22 2015
@@ -21,6 +21,18 @@ namespace runtime {
 
 using namespace ast_matchers;
 
+IntegerTypesCheck::IntegerTypesCheck(StringRef Name, ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+  UnsignedTypePrefix(Options.get("UnsignedTypePrefix", "uint")),
+  SignedTypePrefix(Options.get("SignedTypePrefix", "int")),
+  TypeSuffix(Options.get("TypeSuffix", "")) {}
+
+void IntegerTypesCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "UnsignedTypePrefix", UnsignedTypePrefix);
+  Options.store(Opts, "SignedTypePrefix", SignedTypePrefix);
+  Options.store(Opts, "TypeSuffix", TypeSuffix);
+}
+
 void IntegerTypesCheck::registerMatchers(MatchFinder *Finder) {
   // Find all TypeLocs. The relevant Style Guide rule only applies to C++.
   if (getLangOpts().CPlusPlus)
@@ -86,7 +98,8 @@ void IntegerTypesCheck::check(const Matc
 
   std::string Replacement =
   ((IsSigned ? SignedTypePrefix : UnsignedTypePrefix) + Twine(Width) +
-   (AddUnderscoreT ? "_t" : "")).str();
+   TypeSuffix)
+  .str();
 
   // We don't add a fix-it as changing the type can easily break code,
   // e.g. when a function requires a 'long' argument on all platforms.

Modified: clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.h?rev=247803&r1=247802&r2=247803&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.h Wed Sep 16 
09:36:22 2015
@@ -23,16 +23,15 @@ namespace runtime {
 /// Correspondig cpplint.py check: 'runtime/int'.
 class IntegerTypesCheck : public ClangTidyCheck {
 public:
-  IntegerTypesCheck(StringRef Name, ClangTidyContext *Context)
-  : ClangTidyCheck(Name, Context), UnsignedTypePrefix("uint"),
-SignedTypePrefix("int"), AddUnderscoreT(false) {}
+  IntegerTypesCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
 
 private:
-  const StringRef UnsignedTypePrefix;
-  const StringRef SignedTypePrefix;
-  const bool AddUnderscoreT;
+  const std::string UnsignedTypePrefix;
+  const std::string SignedTypePrefix;
+  const std::string TypeSuffix;
 };
 
 } // namespace runtime

Added: clang-tools-extra/trunk/test/clang-tidy/google-runtime-int-std.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/google-runtime-int-std.cpp?rev=247803&view=auto
==
--- clang-tools-extra/trunk/test/clang-tidy/google-runtime-int-std.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/google-runtime-int-std.cpp Wed Sep 
16 09:36:22 2015
@@ -0,0 +1,57 @@
+// RUN: %python %S/check_clang_tidy.py %s google-runtime-int %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: google-runtime-int.UnsignedTypePrefix, value: "std::uint"}, \
+// RUN: {key: google-runtime-int.SignedTypePrefix, value: "std::int"}, \
+// RUN: {key: google-runtime-int.TypeSuffix, value: "_t"}, \
+// RUN:   ]}' -- -std=c++11
+
+long a();
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: consider replacing 'long' with 
'std::int{{..}}_t'
+
+typedef unsigned long long uint64; // NOLINT
+
+long b(long = 1);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: consider replacing 'long' with 
'std::int{{..}}_t'
+// CHECK-MESSAGES: [[@LINE-2]]:8: warning: consider replacing 'long' with 
'std::int{{..}}_t'
+
+template 
+void tmpl() {
+  T i;
+}
+
+short bar(const short, unsigned short) {
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: consider replacing 'short' with 
'std::int16_t'
+// CHECK-MESSAGES: [[@LINE-2]]:17: warning: consider replacing 'short' with 
'std::int16_t'
+// CHECK-MESSAGES: [[@LINE-3]]:24: warning: consider replacing 'unsigned 
short' wit

[clang-tools-extra] r247798 - [clang-tidy] Added a style guide link.

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Wed Sep 16 08:54:16 2015
New Revision: 247798

URL: http://llvm.org/viewvc/llvm-project?rev=247798&view=rev
Log:
[clang-tidy] Added a style guide link.

Modified:
clang-tools-extra/trunk/docs/clang-tidy/checks/google-runtime-int.rst

Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/google-runtime-int.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/google-runtime-int.rst?rev=247798&r1=247797&r2=247798&view=diff
==
--- clang-tools-extra/trunk/docs/clang-tidy/checks/google-runtime-int.rst 
(original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/google-runtime-int.rst Wed 
Sep 16 08:54:16 2015
@@ -5,4 +5,7 @@ google-runtime-int
 Finds uses of ``short``, ``long`` and ``long long`` and suggest replacing them
 with ``u?intXX(_t)?``.
 
+The corresponding style guide rule:
+https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types.
+
 Correspondig cpplint.py check: 'runtime/int'.


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12903: Allow the -load option in the driver and pass it through to -cc1

[Renato Golin via cfe-commits]

rengolin added a subscriber: rengolin.
rengolin added a comment.

Hi John,

Can you expand a bit more on why you need this, what's the use case, and 
hopefully attach a test for them?

cheers,
--renato


Repository:
  rL LLVM

http://reviews.llvm.org/D12903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12904: fix for assertion fail for pragma weak on typedef

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a comment.

Thanks for this!



Comment at: test/CodeGen/pragma-weak.c:63
@@ -62,1 +62,3 @@
+typedef int __td3;
+#pragma weak td3 = __td3 // expected-warning {{weak identifier '__td3' never 
declared}}
 

This diagnostic isn't useful to the user; __td3 was declared as a typedef 
declaration. I would prefer something a bit more user-friendly; ideally using 
diag::warn_attribute_wrong_decl_type.


http://reviews.llvm.org/D12904



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r247792 - [clang-tidy] Improve the help text for -dump-config.

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Wed Sep 16 08:21:57 2015
New Revision: 247792

URL: http://llvm.org/viewvc/llvm-project?rev=247792&view=rev
Log:
[clang-tidy] Improve the help text for -dump-config.

Modified:
clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp

Modified: clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp?rev=247792&r1=247791&r2=247792&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp Wed Sep 16 
08:21:57 2015
@@ -121,9 +121,11 @@ static cl::opt Config(
 static cl::opt DumpConfig(
 "dump-config",
 cl::desc("Dumps configuration in the YAML format to stdout. This option\n"
- "should be used along with a file name (and '--' if the file is\n"
+ "can be used along with a file name (and '--' if the file is\n"
  "outside of a project with configured compilation database). 
The\n"
- "configuration used for this file will be printed."),
+ "configuration used for this file will be printed.\n"
+ "Use along with -checks=* to include configuration of all\n"
+ "checks.\n"),
 cl::init(false), cl::cat(ClangTidyCategory));
 
 static cl::opt EnableCheckProfile(


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12904: fix for assertion fail for pragma weak on typedef

[Alexander Musman via cfe-commits]

amusman created this revision.
amusman added reviewers: rsmith, ABataev.
amusman added a subscriber: cfe-commits.

Can you please review a fix for assertion fail on the following small test:

typedef int __td3;
#pragma weak td3 = __td3

Assertion failed: (isa(ND) || isa(ND)), function 
DeclClonePragmaWeak, file 
/export/bpart/users/amusman/workspaces/blib16/llvm/tools/clang/lib/Sema/SemaDeclAttr.cpp,
 line 5337.

Call stack:

frame #4: 0x000103e2eb61 clang`__assert_rtn(func=0x000103f0dec0, 
file=0x000103f0dd75, line=5337, expr=0x000103f0ded4) + 129 at 
Signals.inc:537
frame #5: 0x000100d6b5da 
clang`clang::Sema::DeclClonePragmaWeak(this=0x00010a051a00, 
ND=0x00010a030810, II=0x00010a0720d0, Loc=SourceLocation at 
0x7fff5fbf93d0) + 170 at SemaDeclAttr.cpp:5337
frame #6: 0x000100d6bbd5 
clang`clang::Sema::DeclApplyPragmaWeak(this=0x00010a051a00, 
S=0x000109b112f0, ND=0x00010a030810, W=0x7fff5fbf95e8) + 181 at 
SemaDeclAttr.cpp:5388
frame #7: 0x000100d43397 
clang`clang::Sema::ActOnPragmaWeakAlias(this=0x00010a051a00, 
Name=0x00010a0720d0, AliasName=0x00010a072068, PragmaLoc=SourceLocation 
at 0x7fff5fbf9638, NameLoc=SourceLocation at 0x7fff5fbf9630, 
AliasNameLoc=SourceLocation at 0x7fff5fbf9628) + 295 at SemaDecl.cpp:14221
frame #8: 0x00010098e6d5 
clang`clang::Parser::HandlePragmaWeakAlias(this=0x00010a053a00) + 261 at 
ParsePragma.cpp:433
frame #9: 0x0001009c152e 
clang`clang::Parser::ParseExternalDeclaration(this=0x00010a053a00, 
attrs=0x7fff5fbf99d8, DS=0x) + 1790 at Parser.cpp:702
frame #10: 0x0001009c0de7 
clang`clang::Parser::ParseTopLevelDecl(this=0x00010a053a00, 
Result=0x7fff5fbf9b00) + 743 at Parser.cpp:572
frame #11: 0x00010090d45d clang`clang::ParseAST(S=0x00010a051a00, 
PrintStats=false, SkipFunctionBodies=false) + 989 at ParseAST.cpp:144
frame #12: 0x00010017262f 
clang`clang::ASTFrontendAction::ExecuteAction(this=0x000109b090a0) + 511 at 
FrontendAction.cpp:537
frame #13: 0x000100171bc0 
clang`clang::FrontendAction::Execute(this=0x000109b090a0) + 112 at 
FrontendAction.cpp:439
frame #14: 0x000100101865 
clang`clang::CompilerInstance::ExecuteAction(this=0x000109b084e0, 
Act=0x000109b090a0) + 997 at CompilerInstance.cpp:806
frame #15: 0x000100020cbe 
clang`clang::ExecuteCompilerInvocation(Clang=0x000109b084e0) + 3246 at 
ExecuteCompilerInvocation.cpp:318
frame #16: 0x00011449 clang`cc1_main(Argv=ArrayRef at 
0x7fff5fbfa6a8, Argv0=0x7fff5fbfd1b8, MainAddr=0x0001000149f0) + 
2473 at cc1_main.cpp:110
frame #17: 0x000100016133 clang`ExecuteCC1Tool(argv=ArrayRef at 0x7fff5fbfb168, Tool=StringRef at 0x7fff5fbfb158) + 163 at 
driver.cpp:369
frame #18: 0x000100014fb5 clang`main(argc_=3, argv_=0x7fff5fbfc9c8) 
+ 1269 at driver.cpp:415


http://reviews.llvm.org/D12904

Files:
  lib/Sema/SemaDecl.cpp
  test/CodeGen/pragma-weak.c

Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -14530,7 +14530,7 @@
 LookupOrdinaryName);
   WeakInfo W = WeakInfo(Name, NameLoc);
 
-  if (PrevDecl) {
+  if (PrevDecl && (isa(PrevDecl) || isa(PrevDecl))) {
 if (!PrevDecl->hasAttr())
   if (NamedDecl *ND = dyn_cast(PrevDecl))
 DeclApplyPragmaWeak(TUScope, ND, W);
Index: test/CodeGen/pragma-weak.c
===
--- test/CodeGen/pragma-weak.c
+++ test/CodeGen/pragma-weak.c
@@ -59,6 +59,8 @@
 #pragma weak td2 = __td2 // expected-warning {{weak identifier '__td2' never 
declared}}
 typedef int __td2;
 
+typedef int __td3;
+#pragma weak td3 = __td3 // expected-warning {{weak identifier '__td3' never 
declared}}
 
 / test weird cases
 


Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -14530,7 +14530,7 @@
 LookupOrdinaryName);
   WeakInfo W = WeakInfo(Name, NameLoc);
 
-  if (PrevDecl) {
+  if (PrevDecl && (isa(PrevDecl) || isa(PrevDecl))) {
 if (!PrevDecl->hasAttr())
   if (NamedDecl *ND = dyn_cast(PrevDecl))
 DeclApplyPragmaWeak(TUScope, ND, W);
Index: test/CodeGen/pragma-weak.c
===
--- test/CodeGen/pragma-weak.c
+++ test/CodeGen/pragma-weak.c
@@ -59,6 +59,8 @@
 #pragma weak td2 = __td2 // expected-warning {{weak identifier '__td2' never declared}}
 typedef int __td2;
 
+typedef int __td3;
+#pragma weak td3 = __td3 // expected-warning {{weak identifier '__td3' never declared}}
 
 / test weird cases
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mai

Re: [PATCH] D12600: [mips] Added support for using the command line options -Wa, -msoft-float and -Wa, -mhard-float.

[Daniel Sanders via cfe-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM

It's not for this patch but I think we need a general solution to the -Wa,... 
options. It seems wrong to implement each assembler option twice (both with and 
without the '-Wa,').


http://reviews.llvm.org/D12600



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12700: [clang-tidy] install helper scripts in CMake build

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/D12700#246948, @sylvestre.ledru wrote:

> share/clang is fine, thanks.


Then we need to make a similar change to the configure build. Eugene, can you 
do this?


http://reviews.llvm.org/D12700



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12903: Allow the -load option in the driver and pass it through to -cc1

[John Brawn via cfe-commits]

john.brawn created this revision.
john.brawn added a subscriber: cfe-commits.
john.brawn set the repository for this revision to rL LLVM.

Currently -load is only allowed in the -cc1 command, which makes it rather 
awkward to use. Allowing it in the driver and passing it through to -cc1 is 
much more convenient.

Repository:
  rL LLVM

http://reviews.llvm.org/D12903

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  lib/Driver/Tools.cpp

Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -5035,6 +5035,8 @@
   Args.AddAllArgs(CmdArgs, options::OPT_fcomment_block_commands);
   // Forward -fparse-all-comments to -cc1.
   Args.AddAllArgs(CmdArgs, options::OPT_fparse_all_comments);
+  // Forward -load to -cc1
+  Args.AddAllArgs(CmdArgs, options::OPT_load);
 
   // Forward -Xclang arguments to -cc1, and -mllvm arguments to the LLVM option
   // parser.
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1192,6 +1192,8 @@
 def l : JoinedOrSeparate<["-"], "l">, Flags<[LinkerInput, RenderJoined]>;
 def lazy__framework : Separate<["-"], "lazy_framework">, Flags<[LinkerInput]>;
 def lazy__library : Separate<["-"], "lazy_library">, Flags<[LinkerInput]>;
+def load : Separate<["-"], "load">, Flags<[DriverOption,CC1Option]>, 
MetaVarName<"">,
+  HelpText<"Load the named plugin (dynamic shared object)">;
 def mlittle_endian : Flag<["-"], "mlittle-endian">, Flags<[DriverOption]>;
 def EL : Flag<["-"], "EL">, Alias;
 def mbig_endian : Flag<["-"], "mbig-endian">, Flags<[DriverOption]>;
Index: include/clang/Driver/CC1Options.td
===
--- include/clang/Driver/CC1Options.td
+++ include/clang/Driver/CC1Options.td
@@ -343,8 +343,6 @@
   HelpText<"Include brief documentation comments in code-completion results.">;
 def disable_free : Flag<["-"], "disable-free">,
   HelpText<"Disable freeing of memory on exit">;
-def load : Separate<["-"], "load">, MetaVarName<"">,
-  HelpText<"Load the named plugin (dynamic shared object)">;
 def plugin : Separate<["-"], "plugin">, MetaVarName<"">,
   HelpText<"Use the named plugin action instead of the default action (use 
\"help\" to list available options)">;
 def plugin_arg : JoinedAndSeparate<["-"], "plugin-arg-">,


Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -5035,6 +5035,8 @@
   Args.AddAllArgs(CmdArgs, options::OPT_fcomment_block_commands);
   // Forward -fparse-all-comments to -cc1.
   Args.AddAllArgs(CmdArgs, options::OPT_fparse_all_comments);
+  // Forward -load to -cc1
+  Args.AddAllArgs(CmdArgs, options::OPT_load);
 
   // Forward -Xclang arguments to -cc1, and -mllvm arguments to the LLVM option
   // parser.
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1192,6 +1192,8 @@
 def l : JoinedOrSeparate<["-"], "l">, Flags<[LinkerInput, RenderJoined]>;
 def lazy__framework : Separate<["-"], "lazy_framework">, Flags<[LinkerInput]>;
 def lazy__library : Separate<["-"], "lazy_library">, Flags<[LinkerInput]>;
+def load : Separate<["-"], "load">, Flags<[DriverOption,CC1Option]>, MetaVarName<"">,
+  HelpText<"Load the named plugin (dynamic shared object)">;
 def mlittle_endian : Flag<["-"], "mlittle-endian">, Flags<[DriverOption]>;
 def EL : Flag<["-"], "EL">, Alias;
 def mbig_endian : Flag<["-"], "mbig-endian">, Flags<[DriverOption]>;
Index: include/clang/Driver/CC1Options.td
===
--- include/clang/Driver/CC1Options.td
+++ include/clang/Driver/CC1Options.td
@@ -343,8 +343,6 @@
   HelpText<"Include brief documentation comments in code-completion results.">;
 def disable_free : Flag<["-"], "disable-free">,
   HelpText<"Disable freeing of memory on exit">;
-def load : Separate<["-"], "load">, MetaVarName<"">,
-  HelpText<"Load the named plugin (dynamic shared object)">;
 def plugin : Separate<["-"], "plugin">, MetaVarName<"">,
   HelpText<"Use the named plugin action instead of the default action (use \"help\" to list available options)">;
 def plugin_arg : JoinedAndSeparate<["-"], "plugin-arg-">,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12700: [clang-tidy] install helper scripts in CMake build

[Sylvestre Ledru via cfe-commits]

sylvestre.ledru added a comment.

share/clang is fine, thanks.


http://reviews.llvm.org/D12700



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D6551: Improvements to scan-build.

[Sylvestre Ledru via cfe-commits]

sylvestre.ledru closed this revision.
sylvestre.ledru added a comment.

To close this review.


http://reviews.llvm.org/D6551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12871: [OpenMP] Target directive host codegen - rebased

[Alexey Bataev via cfe-commits]

ABataev added inline comments.


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3044-3054
@@ +3043,13 @@
+
+  if (auto *VAT = dyn_cast(ElementType.getTypePtr())) {
+auto VATInfo = CGF.getVLASize(VAT);
+Size = llvm::ConstantInt::get(
+CGM.SizeTy,
+CGM.getContext().getTypeSizeInChars(VATInfo.second).getQuantity());
+Size = CGF.Builder.CreateNUWMul(Size, VATInfo.first);
+  } else {
+uint64_t ElementTypeSize =
+CGM.getContext().getTypeSizeInChars(ElementType).getQuantity();
+Size = llvm::ConstantInt::get(CGM.SizeTy, ElementTypeSize);
+  }
+

Use `getTypeSize(CGF, ElementType)` instead


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3082-3128
@@ +3081,49 @@
+
+  // Generate the code to launch the target region. The pattern is the
+  // following:
+  //
+  //   ...
+  //   br IfCond (if any), omp_offload, omp_offload_fail
+  //
+  // omp_offload.try:
+  //   ; create arrays for offloading
+  //   error = __tgt_target(...)
+  //   br error, omp_offload_fail, omp_offload_end
+  //
+  // omp_offload.fail:
+  //   host_version(...)
+  //
+  // omp_offload.end:
+  //   ...
+  //
+
+  auto OffloadTryBlock = CGF.createBasicBlock("omp_offload.try");
+  auto OffloadFailBlock = CGF.createBasicBlock("omp_offload.fail");
+  auto ContBlock = CGF.createBasicBlock("omp_offload.end");
+
+  if (IfCond)
+CGF.EmitBranchOnBoolExpr(IfCond, OffloadTryBlock, OffloadFailBlock,
+ /*TrueCount=*/0);
+
+  CGF.EmitBlock(OffloadTryBlock);
+
+  unsigned PointerNumVal = BasePointers.size();
+  llvm::Value *PointerNum = CGF.Builder.getInt32(PointerNumVal);
+  llvm::Value *BasePointersArray;
+  llvm::Value *PointersArray;
+  llvm::Value *SizesArray;
+  llvm::Value *MapTypesArray;
+
+  if (PointerNumVal) {
+llvm::APInt PointerNumAP(32, PointerNumVal, /*isSigned=*/true);
+QualType PointerArrayType = CGF.getContext().getConstantArrayType(
+CGF.getContext().VoidPtrTy, PointerNumAP, ArrayType::Normal,
+/*IndexTypeQuals=*/0);
+
+BasePointersArray =
+CGF.CreateMemTemp(PointerArrayType, ".offload_baseptrs").getPointer();
+PointersArray =
+CGF.CreateMemTemp(PointerArrayType, ".offload_ptrs").getPointer();
+
+// If we don't have any VLA types, we can use a constant array for the map
+// sizes, otherwise we need to fill up the arrays as we do for the 
pointers.

Could you use `emitOMPIfClause()` function instead of this long block?


Comment at: lib/CodeGen/CGStmtOpenMP.cpp:38
@@ +37,3 @@
+  if (UseOnlyReferences) {
+LValue LV = MakeNaturalAlignAddrLValue(
+CreateMemTemp(CurField->getType(), "__vla_size_ref").getPointer(),

Use `MakeAddrLValue(CreateMemTemp(CurField->getType(), "__vla_size_ref"), 
CurField->getType())` instead.


Comment at: lib/CodeGen/CGStmtOpenMP.cpp:80-81
@@ -67,2 +79,4 @@
   II = &getContext().Idents.get("vla");
+  if (UseOnlyReferences)
+ArgType = getContext().getPointerType(ArgType);
 }

Use `getContext().getLValueReferenceType(ArgType, /*SpelledAsLValue=*/false);`


Comment at: lib/CodeGen/CGStmtOpenMP.cpp:117-122
@@ -102,4 +116,8 @@
 if (FD->hasCapturedVLAType()) {
   auto *ExprArg =
   EmitLoadOfLValue(ArgLVal, SourceLocation()).getScalarVal();
+  if (UseOnlyReferences) {
+auto ExprArgRef = MakeNaturalAlignAddrLValue(ExprArg, FD->getType());
+ExprArg = EmitLoadOfLValue(ExprArgRef, 
SourceLocation()).getScalarVal();
+  }
   auto VAT = FD->getCapturedVLAType();

```
if (UseOnlyReferences)
  ArgLVal = CGF.EmitLoadOfReferenceLValue(ArgLVal.getAddress(), 
FD->getType()->casAs());
auto *ExprArg = EmitLoadOfLValue(ArgLVal, SourceLocation()).getScalarVal();
```


http://reviews.llvm.org/D12871



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D3967: [Proposal] [Analyzer] Individual options for checkers

[Aleksei Sidorin via cfe-commits]

a.sidorin abandoned this revision.
a.sidorin added a comment.

Abandon this change since the patch based on this is committed in 
http://reviews.llvm.org/D7905


http://reviews.llvm.org/D3967



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12839: Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[Alexander Kornienko via cfe-commits]

I also think this suits better for a clang-tidy check, but for a different
reason: adding `std::move` calls can require adding `#include `,
which is fine for a clang-tidy check (and we have facilities for that), but
it's a questionable functionality for a compiler diagnostic.
On 16 Sep 2015 09:20, "Richard Smith"  wrote:

> On Tue, Sep 15, 2015 at 12:50 PM, Aaron Ballman 
> wrote:
>
>> aaron.ballman added reviewers: rtrieu, rsmith.
>> aaron.ballman added a comment.
>>
>> There is a -Wpessimizing-move frontend warning that Richard added not too
>> long ago, which tells the user to remove calls to std::move() because they
>> pessimize the code. The new functionality you are looking to add is
>> basically the opposite: it tells the user to add std::move() because the
>> code is currently pessimized due to copies. Given how general that concept
>> is (it doesn't just appertain to constructor initializations), I wonder if
>> this makes more sense as a frontend warning like -Wpessimizing-copy.
>>
>> Richard (both of you), what do you think?
>>
>
> I think this is in the grey area between what's appropriate for clang-tidy
> and what's appropriate as a warning, where both options have merit; the
> same is true with -Wpessimizing-move. I think the difference between the
> two cases is that -Wpessimizing-move detects a case where you wrote
> something that doesn't do what (we think) you meant, whereas this check
> detects a case where you /didn't/ write something that (we think) would
> make your code better (even though both changes have similar effects, of
> safely turning a copy into a move or a move into an elided move). It's a
> fine line, but for me that nudges -Wpessimizing-move into a warning, and
> this check into clang-tidy territory.
>
> ~Aaron
>>
>>
>> 
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:32
>> @@ +31,3 @@
>> +  // excessive copying, we'll warn on them.
>> +  if (Node->isDependentType()) {
>> +return false;
>> 
>> Elide braces, here and elsewhere.
>>
>> 
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:36
>> @@ +35,3 @@
>> +  // Ignore trivially copyable types.
>> +  if (Node->isScalarType() ||
>> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
>> 
>> Can turn this into a return statement directly instead of an if.
>>
>> 
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38
>> @@ +37,3 @@
>> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
>> +  classHasTrivialCopyAndDestroy(Node)) {
>> +return false;
>> 
>> Why do you need classHasTrivialCopyAndDestroy() when you're already
>> checking if it's a trivially copyable type?
>>
>> 
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44
>> @@ +43,3 @@
>> +
>> +int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
>> + const CXXConstructorDecl
>> &ConstructorDecl,
>> 
>> Should return unsigned, please.
>>
>> 
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:50
>> @@ +49,3 @@
>> +  findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam);
>> +  auto Matches = match(AllDeclRefs, *ConstructorDecl.getBody(), Context);
>> +  Occurrences += Matches.size();
>> 
>> You don't actually need Matches, you can call match().size() instead.
>>
>> 
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:52
>> @@ +51,3 @@
>> +  Occurrences += Matches.size();
>> +  for (const auto* Initializer : ConstructorDecl.inits()) {
>> +Matches = match(AllDeclRefs, *Initializer->getInit(), Context);
>> 
>> Should be *Initializer instead of * Initializer.
>>
>> 
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120
>> @@ +119,3 @@
>> +  }
>> +  diag(InitArg->getLocStart(), "value parameter can be moved to avoid
>> copy.");
>> +}
>> 
>> Perhaps: "argument can be moved to avoid a copy" instead?
>>
>> 
>> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:84
>> @@ +83,3 @@
>> +  Movable(const Movable&) = default;
>> +  Movable& operator =(const Movable&) = default;
>> +  ~Movable() {}
>> 
>> Formatting
>>
>> 
>> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:85
>> @@ +84,3 @@
>> +  Movable& operator =(const Movable&) = default;
>> +  ~Movable() {}
>> +};
>> 
>> Why not = default?
>>
>> 
>> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:113
>> @@ +112,3 @@
>> +
>> +struct NegativeParamTriviallyCopyable {
>> +  NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}
>> 
>> Should also have a test for scalars
>>
>>
>> http://reviews.llvm.org/D12839
>>
>>
>>
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http:

Re: [PATCH] D12839: Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[Richard Smith via cfe-commits]

On Tue, Sep 15, 2015 at 12:50 PM, Aaron Ballman 
wrote:

> aaron.ballman added reviewers: rtrieu, rsmith.
> aaron.ballman added a comment.
>
> There is a -Wpessimizing-move frontend warning that Richard added not too
> long ago, which tells the user to remove calls to std::move() because they
> pessimize the code. The new functionality you are looking to add is
> basically the opposite: it tells the user to add std::move() because the
> code is currently pessimized due to copies. Given how general that concept
> is (it doesn't just appertain to constructor initializations), I wonder if
> this makes more sense as a frontend warning like -Wpessimizing-copy.
>
> Richard (both of you), what do you think?
>

I think this is in the grey area between what's appropriate for clang-tidy
and what's appropriate as a warning, where both options have merit; the
same is true with -Wpessimizing-move. I think the difference between the
two cases is that -Wpessimizing-move detects a case where you wrote
something that doesn't do what (we think) you meant, whereas this check
detects a case where you /didn't/ write something that (we think) would
make your code better (even though both changes have similar effects, of
safely turning a copy into a move or a move into an elided move). It's a
fine line, but for me that nudges -Wpessimizing-move into a warning, and
this check into clang-tidy territory.

~Aaron
>
>
> 
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:32
> @@ +31,3 @@
> +  // excessive copying, we'll warn on them.
> +  if (Node->isDependentType()) {
> +return false;
> 
> Elide braces, here and elsewhere.
>
> 
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:36
> @@ +35,3 @@
> +  // Ignore trivially copyable types.
> +  if (Node->isScalarType() ||
> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
> 
> Can turn this into a return statement directly instead of an if.
>
> 
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38
> @@ +37,3 @@
> +  Node.isTriviallyCopyableType(Finder->getASTContext()) ||
> +  classHasTrivialCopyAndDestroy(Node)) {
> +return false;
> 
> Why do you need classHasTrivialCopyAndDestroy() when you're already
> checking if it's a trivially copyable type?
>
> 
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44
> @@ +43,3 @@
> +
> +int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
> + const CXXConstructorDecl
> &ConstructorDecl,
> 
> Should return unsigned, please.
>
> 
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:50
> @@ +49,3 @@
> +  findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam);
> +  auto Matches = match(AllDeclRefs, *ConstructorDecl.getBody(), Context);
> +  Occurrences += Matches.size();
> 
> You don't actually need Matches, you can call match().size() instead.
>
> 
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:52
> @@ +51,3 @@
> +  Occurrences += Matches.size();
> +  for (const auto* Initializer : ConstructorDecl.inits()) {
> +Matches = match(AllDeclRefs, *Initializer->getInit(), Context);
> 
> Should be *Initializer instead of * Initializer.
>
> 
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120
> @@ +119,3 @@
> +  }
> +  diag(InitArg->getLocStart(), "value parameter can be moved to avoid
> copy.");
> +}
> 
> Perhaps: "argument can be moved to avoid a copy" instead?
>
> 
> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:84
> @@ +83,3 @@
> +  Movable(const Movable&) = default;
> +  Movable& operator =(const Movable&) = default;
> +  ~Movable() {}
> 
> Formatting
>
> 
> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:85
> @@ +84,3 @@
> +  Movable& operator =(const Movable&) = default;
> +  ~Movable() {}
> +};
> 
> Why not = default?
>
> 
> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:113
> @@ +112,3 @@
> +
> +struct NegativeParamTriviallyCopyable {
> +  NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}
> 
> Should also have a test for scalars
>
>
> http://reviews.llvm.org/D12839
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits