[PATCH] D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

[S. B. Tam via Phabricator via cfe-commits]

cpplearner created this revision.
cpplearner added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.

Fixes PR39858


Repository:
  rC Clang

https://reviews.llvm.org/D55413

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx1y.cpp


Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -338,11 +338,11 @@
 int a = 3;
 a += 6;
 if (a != 9) return false;
-a -= 2;
+a -= 1.8;
 if (a != 7) return false;
 a *= 3;
 if (a != 21) return false;
-if (&(a /= 10) != ) return false;
+if (&(a /= 7.9) != ) return false;
 if (a != 2) return false;
 a <<= 3;
 if (a != 16) return false;
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -3424,13 +3424,21 @@
 if (!checkConst(SubobjType))
   return false;

-if (!SubobjType->isIntegerType() || !RHS.isInt()) {
+if (!SubobjType->isIntegerType() || !RHS.isInt() && !RHS.isFloat()) {
   // We don't support compound assignment on integer-cast-to-pointer
   // values.
   Info.FFDiag(E);
   return false;
 }

+if (RHS.isFloat()) {
+  APFloat FValue(0.0);
+  return HandleIntToFloatCast(Info, E, SubobjType, Value, PromotedLHSType,
+  FValue) &&
+ handleFloatFloatBinOp(Info, E, FValue, Opcode, RHS.getFloat()) &&
+ HandleFloatToIntCast(Info, E, PromotedLHSType, FValue, SubobjType,
+  Value);
+}
 APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType,
 SubobjType, Value);
 if (!handleIntIntBinOp(Info, E, LHS, Opcode, RHS.getInt(), LHS))


Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -338,11 +338,11 @@
 int a = 3;
 a += 6;
 if (a != 9) return false;
-a -= 2;
+a -= 1.8;
 if (a != 7) return false;
 a *= 3;
 if (a != 21) return false;
-if (&(a /= 10) != ) return false;
+if (&(a /= 7.9) != ) return false;
 if (a != 2) return false;
 a <<= 3;
 if (a != 16) return false;
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -3424,13 +3424,21 @@
 if (!checkConst(SubobjType))
   return false;

-if (!SubobjType->isIntegerType() || !RHS.isInt()) {
+if (!SubobjType->isIntegerType() || !RHS.isInt() && !RHS.isFloat()) {
   // We don't support compound assignment on integer-cast-to-pointer
   // values.
   Info.FFDiag(E);
   return false;
 }

+if (RHS.isFloat()) {
+  APFloat FValue(0.0);
+  return HandleIntToFloatCast(Info, E, SubobjType, Value, PromotedLHSType,
+  FValue) &&
+ handleFloatFloatBinOp(Info, E, FValue, Opcode, RHS.getFloat()) &&
+ HandleFloatToIntCast(Info, E, PromotedLHSType, FValue, SubobjType,
+  Value);
+}
 APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType,
 SubobjType, Value);
 if (!handleIntIntBinOp(Info, E, LHS, Opcode, RHS.getInt(), LHS))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55388: [analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move.

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

xazax.hun added a comment.

Hm. I wonder if it would also make sense to model e.g. the get method to return 
nullptr for moved from smart ptrs. This could help null dereference checker and 
also aid false path prunning.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55388/new/

https://reviews.llvm.org/D55388



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

[PATCH] D55388: [analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move.

[Henry Wong via Phabricator via cfe-commits]

MTC accepted this revision.
MTC added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:90
   // in a valid manner.
   // TODO: We can still try to identify *unsafe* use after move, such as
   // dereference of a moved-from smart pointer (which is guaranteed to be 
null).

Outdated `TODO`?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55388/new/

https://reviews.llvm.org/D55388



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

[PATCH] D55411: [clang-tidy] check for flagging using declarations not in the inner most namespace

[Yucheng Wu via Phabricator via cfe-commits]

Ywicheng created this revision.
Ywicheng added a reviewer: JonasToth.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

This patch adds one check corresponding to the Scope of the Alias section in 
https://abseil.io/tips/119.

In particular, it is better to put aliases in the innermost namespace


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55411

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/SafelyScopedCheck.cpp
  clang-tidy/abseil/SafelyScopedCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-safely-scoped.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-safely-scoped.cpp

Index: test/clang-tidy/abseil-safely-scoped.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-safely-scoped.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s abseil-safely-scoped %t
+namespace bar {
+
+class something {
+
+};
+}
+
+namespace foo {
+
+using bar::something;
+
+namespace {
+
+}  // anonymous namespace
+}  // namespace foo
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: UsingDecl 'something' should be in the innermost scope. See: https://abseil.io/tips/119 [abseil-safely-scoped]
+
+namespace foo {
+
+namespace {
+
+using ::bar::something;
+
+}  // anonymous namespace
+}  // namespace foo
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -10,8 +10,9 @@
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
+   abseil-safely-scoped
abseil-str-cat-append
+   abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
@@ -151,6 +152,7 @@
hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) 
hicpp-static-assert (redirects to misc-static-assert) 
hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) 
+   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
hicpp-use-auto (redirects to modernize-use-auto) 
hicpp-use-emplace (redirects to modernize-use-emplace) 
hicpp-use-equals-default (redirects to modernize-use-equals-default) 
@@ -159,7 +161,6 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
-   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: docs/clang-tidy/checks/abseil-safely-scoped.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-safely-scoped.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - abseil-safely-scoped
+
+abseil-safely-scoped
+
+
+Flags using declarations that are not contained in an innermost
+namespace, and suggests these declarations be moved elsewhere.
+
+Example:
+
+.. code-block:: c++
+
+  using something; // should be inside the innermost namespace bar below
+
+  namespace foo {
+  namespace bar {
+	
+  } // bar
+
+  using something_else; // shoulw be inside the innermost namespace bar above
+
+  } // foo
+
+Placing convenience aliases in upper level namespaces can lead to ambiguity in 
+which name the compiler should use. 
+
+See https://abseil.io/tips/119 for more explanation. 
+
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,12 @@
 Improvements to clang-tidy
 --
 
+- New :doc:`abseil-safely-scoped
+  ` check.
+
+  Finds instances of using declarations not in the innermost layer
+  of a series of namespaces.
+
 - New :doc:`abseil-duration-division
   ` check.
 
Index: clang-tidy/abseil/SafelyScopedCheck.h
===
--- /dev/null
+++ clang-tidy/abseil/SafelyScopedCheck.h
@@ -0,0 +1,36 @@
+//===--- SafelyScopedCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_SAFELYSCOPEDCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_SAFELYSCOPEDCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Detecting using declarations not in a namespace declaration or not in 
+/// the innermost layer of namespace declarations.
+///
+/// For the user-facing documentation 

[PATCH] D55409: [clang-tidy] check for using declarations not in an anonymous namespace when there exists one

[Yucheng Wu via Phabricator via cfe-commits]

Ywicheng updated this revision to Diff 177121.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55409/new/

https://reviews.llvm.org/D55409

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp
  clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
  clang-tidy/abseil/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp

Index: test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s abseil-anonymous-enclosed-aliases %t
+namespace bar {
+
+class something {
+
+};
+}
+
+namespace foo {
+
+using bar::something;
+
+namespace {
+
+}  // anonymous namespace
+}  // namespace foo
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: UsingDecl 'something' should be in the anonymous namespace. See: https://abseil.io/tips/119 [abseil-anonymous-enclosed-aliases]
+
+
+namespace foo {
+
+namespace {
+
+using ::bar::something;
+
+}  // anonymous namespace
+}  // namespace foo
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,14 +4,15 @@
 =
 
 .. toctree::
+   abseil-anonymous-enclosed-aliases
abseil-duration-division
abseil-duration-factory-float
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append
+   abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
@@ -151,6 +152,7 @@
hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) 
hicpp-static-assert (redirects to misc-static-assert) 
hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) 
+   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
hicpp-use-auto (redirects to modernize-use-auto) 
hicpp-use-emplace (redirects to modernize-use-emplace) 
hicpp-use-equals-default (redirects to modernize-use-equals-default) 
@@ -159,7 +161,6 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
-   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
@@ -0,0 +1,20 @@
+.. title:: clang-tidy - abseil-anonymous-enclosed-aliases
+
+abseil-anonymous-enclosed-aliases
+=
+
+Finds using declarations outside of anonymous namespaces, and
+suggests those declarations be moved to that namespace.
+
+Example:
+.. code-block:: c++
+
+  namespace foo {
+  
+  using something; // should be inside the anonymous namespace below
+
+  namespace {
+
+  } // anonymous namespace
+
+  } // foo
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,12 @@
 Improvements to clang-tidy
 --
 
+- New :doc:`abseil-anonymous-enclosed-aliases
+  ` check.
+
+  Finds instances of using declarations not in an anonymous namespace
+  when there exists one.
+
 - New :doc:`abseil-duration-division
   ` check.
 
Index: clang-tidy/abseil/CMakeLists.txt
===
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -2,6 +2,7 @@
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  AnonymousEnclosedAliasesCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
Index: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
===
--- /dev/null
+++ clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
@@ -0,0 +1,40 @@
+//===--- AnonymousEnclosedAliasesCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_ANONYMOUSENCLOSEDALIASESCHECK_H
+#define 

[PATCH] D55410: [clang-tidy] check for flagging using declarations in headers

[Shyan Akmal via Phabricator via cfe-commits]

Naysh created this revision.
Naysh added reviewers: EricWF, JonasToth.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

This patch adds an "Alias Free Headers" check, based off the guidelines at 
http://google.github.io/styleguide/cppguide.html#Aliases, to the abseil module.

The purpose of the check is to eliminate using declarations from header files.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55410

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/AliasFreeHeadersCheck.cpp
  clang-tidy/abseil/AliasFreeHeadersCheck.h
  clang-tidy/abseil/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-alias-free-headers.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-alias-free-headers.hpp

Index: test/clang-tidy/abseil-alias-free-headers.hpp
===
--- /dev/null
+++ test/clang-tidy/abseil-alias-free-headers.hpp
@@ -0,0 +1,10 @@
+// RUN: %check_clang_tidy %s abseil-alias-free-headers %t
+
+namespace foo {
+  void f();
+}
+
+using foo::f;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: convenience aliases in header files are dangerous: see http://google.github.io/styleguide/cppguide.html#Aliases [abseil-alias-free-headers]
+
+void other_function();
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,14 +4,15 @@
 =
 
 .. toctree::
+   abseil-alias-free-headers
abseil-duration-division
abseil-duration-factory-float
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append
+   abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
@@ -151,6 +152,7 @@
hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) 
hicpp-static-assert (redirects to misc-static-assert) 
hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) 
+   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
hicpp-use-auto (redirects to modernize-use-auto) 
hicpp-use-emplace (redirects to modernize-use-emplace) 
hicpp-use-equals-default (redirects to modernize-use-equals-default) 
@@ -159,7 +161,6 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
-   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: docs/clang-tidy/checks/abseil-alias-free-headers.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-alias-free-headers.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - abseil-alias-free-headers
+
+abseil-alias-free-headers
+=
+
+Flags using declarations in header files, and suggests that these aliases be removed.
+
+A using declaration placed in a header file forces users of that header file
+accept the specified alias. Because of this, using declarations should almost never
+be used in a header file
+
+The style guide  http://google.github.io/styleguide/cppguide.html#Aliases discusses this
+issue in more detail
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,12 @@
 Improvements to clang-tidy
 --
 
+- New :doc:`abseil-alias-free-headers
+  ` check.
+
+  Flags using declarations in header files, and suggests that
+  these aliases be removed
+
 - New :doc:`abseil-duration-division
   ` check.
 
Index: clang-tidy/abseil/CMakeLists.txt
===
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -2,6 +2,7 @@
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  AliasFreeHeadersCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
Index: clang-tidy/abseil/AliasFreeHeadersCheck.h
===
--- /dev/null
+++ clang-tidy/abseil/AliasFreeHeadersCheck.h
@@ -0,0 +1,35 @@
+//===--- AliasFreeHeadersCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_ALIASFREEHEADERSCHECK_H
+#define 

[PATCH] D55346: [clang-tidy] check for using declaration qualification

[Shyan Akmal via Phabricator via cfe-commits]

Naysh updated this revision to Diff 177116.
Naysh retitled this revision from "[clang-tidy] check for using declaration 
scope and qualification" to "[clang-tidy] check for using declaration 
qualification".
Naysh edited the summary of this revision.
Naysh added a comment.

Based off the request to split this patch into four separate patches 
(corresponding to each of the introduced checks), I've updated the patch to 
only include the "Qualified Aliases" check.

This is a check motivated by the discussion in https://abseil.io/tips/119, and 
flags using declarations that aren't fully qualified.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55346/new/

https://reviews.llvm.org/D55346

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/QualifiedAliasesCheck.cpp
  clang-tidy/abseil/QualifiedAliasesCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-qualified-aliases.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-qualified-aliases.cpp

Index: test/clang-tidy/abseil-qualified-aliases.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-qualified-aliases.cpp
@@ -0,0 +1,26 @@
+// RUN: %check_clang_tidy %s abseil-qualified-aliases %t
+
+namespace foo {
+  void f();
+  void correct();
+}
+
+namespace bar {
+  using foo::f;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: using declaration is not fully qualified: see https://abseil.io/tips/119 [abseil-qualified-aliases]
+  using ::foo::correct;
+}
+
+namespace outermost {
+  namespace middle {
+namespace innermost {
+
+  enum Color {Red, Blue, Yellow};
+
+} // namespace innermost
+
+using innermost::Color;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: using declaration is not fully qualified: see https://abseil.io/tips/119 [abseil-qualified-aliases]
+
+  } // namespace middle
+} // namespace example
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -9,9 +9,10 @@
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
+   abseil-qualified-aliases
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append
+   abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
@@ -151,6 +152,7 @@
hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) 
hicpp-static-assert (redirects to misc-static-assert) 
hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) 
+   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
hicpp-use-auto (redirects to modernize-use-auto) 
hicpp-use-emplace (redirects to modernize-use-emplace) 
hicpp-use-equals-default (redirects to modernize-use-equals-default) 
@@ -159,7 +161,6 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
-   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: docs/clang-tidy/checks/abseil-qualified-aliases.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-qualified-aliases.rst
@@ -0,0 +1,23 @@
+.. title:: clang-tidy - abseil-qualified-aliases
+
+abseil-qualified-aliases
+
+
+Detects using declarations that are not fully qualified, and suggests
+that the developer fully qualify those declarations.
+
+Example:
+.. code-block:: c++
+
+  namespace foo {
+void f();
+void correct();
+  }
+
+  namespace bar {
+using foo::f; // The check produces a warning here.
+using ::foo::correct; // The check sees no issue here.
+  }
+
+See https://abseil.io/tips/119 for a more in depth justification of this
+check.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -98,6 +98,12 @@
   Ensures code does not open ``namespace absl`` as that violates Abseil's
   compatibility guidelines.
 
+- New :doc:`abseil-qualified-aliases
+  ` check.
+
+  Detects using declarations that are not fully qualified, and suggests
+  that the developer fully qualify those declarations.
+
 - New :doc:`abseil-redundant-strcat-calls
   ` check.
 
Index: clang-tidy/abseil/QualifiedAliasesCheck.h
===
--- /dev/null
+++ clang-tidy/abseil/QualifiedAliasesCheck.h
@@ -0,0 +1,35 @@
+//===--- QualifiedAliasesCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file 

[PATCH] D55409: [clang-tidy] check for using declarations not in an anonymous namespace when there exists one

[Yucheng Wu via Phabricator via cfe-commits]

Ywicheng updated this revision to Diff 177115.
Ywicheng added a comment.

Updated abseil-anonymous-enclosed-alises.rst and ReleaseNotes.rst


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55409/new/

https://reviews.llvm.org/D55409

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp
  clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
  clang-tidy/abseil/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp

Index: test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s abseil-anonymous-enclosed-aliases %t
+namespace bar {
+
+class something {
+
+};
+}
+
+namespace foo {
+
+using bar::something;
+
+namespace {
+
+}  // anonymous namespace
+}  // namespace foo
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: UsingDecl 'something' should be in the anonymous namespace. See: https://abseil.io/tips/119 [abseil-anonymous-enclosed-aliases]
+
+
+namespace foo {
+
+namespace {
+
+using ::bar::something;
+
+}  // anonymous namespace
+}  // namespace foo
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,14 +4,15 @@
 =
 
 .. toctree::
+   abseil-anonymous-enclosed-aliases
abseil-duration-division
abseil-duration-factory-float
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append
+   abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
@@ -151,6 +152,7 @@
hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) 
hicpp-static-assert (redirects to misc-static-assert) 
hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) 
+   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
hicpp-use-auto (redirects to modernize-use-auto) 
hicpp-use-emplace (redirects to modernize-use-emplace) 
hicpp-use-equals-default (redirects to modernize-use-equals-default) 
@@ -159,7 +161,6 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
-   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
@@ -0,0 +1,20 @@
+.. title:: clang-tidy - abseil-anonymous-enclosed-aliases
+
+abseil-anonymous-enclosed-aliases
+=
+
+Finds using declarations outside of anonymous namespaces, and
+suggests those declarations be moved to that namespace.
+
+Example:
+.. code-block:: c++
+
+  namespace foo {
+  
+  using something; // should be inside the anonymous namespace below
+
+  namespace {
+
+  } // anonymous namespace
+
+  } // foo
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,12 @@
 Improvements to clang-tidy
 --
 
+- New :doc:`abseil-anonymous-enclosed-aliases
+  ` check.
+
+  Finds instances of using declarations not in an anonymous namespace
+  when there exists one.
+
 - New :doc:`abseil-duration-division
   ` check.
 
Index: clang-tidy/abseil/CMakeLists.txt
===
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -2,6 +2,7 @@
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  AnonymousEnclosedAliasesCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
Index: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
===
--- /dev/null
+++ clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
@@ -0,0 +1,40 @@
+//===--- AnonymousEnclosedAliasesCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef 

[PATCH] D55409: [clang-tidy] check for using declarations not in an anonymous namespace when there exists one

[Yucheng Wu via Phabricator via cfe-commits]

Ywicheng created this revision.
Ywicheng added reviewers: alexfh, hokein, aaron.ballman, astrelni, EricWF, 
JonasToth.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

This patch adds one check corresponding to the Unnamed Namespace section in 
https://abseil.io/tips/119.

In particular, if there exists an anonymous, it is better to put all aliases 
there.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55409

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp
  clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
  clang-tidy/abseil/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp

Index: test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s abseil-anonymous-enclosed-aliases %t
+namespace bar {
+
+class something {
+
+};
+}
+
+namespace foo {
+
+using bar::something;
+
+namespace {
+
+}  // anonymous namespace
+}  // namespace foo
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: UsingDecl 'something' should be in the anonymous namespace. See: https://abseil.io/tips/119 [abseil-anonymous-enclosed-aliases]
+
+
+namespace foo {
+
+namespace {
+
+using ::bar::something;
+
+}  // anonymous namespace
+}  // namespace foo
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,14 +4,15 @@
 =
 
 .. toctree::
+   abseil-anonymous-enclosed-aliases
abseil-duration-division
abseil-duration-factory-float
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append
+   abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
@@ -151,6 +152,7 @@
hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) 
hicpp-static-assert (redirects to misc-static-assert) 
hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) 
+   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
hicpp-use-auto (redirects to modernize-use-auto) 
hicpp-use-emplace (redirects to modernize-use-emplace) 
hicpp-use-equals-default (redirects to modernize-use-equals-default) 
@@ -159,7 +161,6 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
-   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - abseil-anonymous-enclosed-aliases
+
+abseil-anonymous-enclosed-aliases
+=
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,11 @@
 Improvements to clang-tidy
 --
 
+- New :doc:`abseil-anonymous-enclosed-aliases
+  ` check.
+
+  FIXME: add release notes.
+
 - New :doc:`abseil-duration-division
   ` check.
 
Index: clang-tidy/abseil/CMakeLists.txt
===
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -2,6 +2,7 @@
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  AnonymousEnclosedAliasesCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
Index: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
===
--- /dev/null
+++ clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
@@ -0,0 +1,40 @@
+//===--- AnonymousEnclosedAliasesCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_ANONYMOUSENCLOSEDALIASESCHECK_H
+#define 

[PATCH] D54872: [clangd] Add ability to not use -resource-dir at all

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In D54872#1319958 , @ilya-biryukov 
wrote:

> I'm a bit confused now, will put up a few clarifying questions to make sure I 
> understand the probem properly.
>  Does `clang-cl` work correctly the arguments from `compile_commands.json`? 
> Does it fail if we add the `-resource-dir`, similar to how it was done for 
> clangd?


Yes it does fail the same way if I use clang-cl from the same local build as 
the clangd with the added -resource-dir. Interestingly, it does also fail if I 
don't add the -resource-dir, unlike clangd (this patch). For that clang-cl to 
work on the command-line, I have to use -resource-dir C:\Program 
Files\LLVM\lib\clang\8.0.0\include. The clang-cl from the installer works 
without the -resource-dir. Presumably clang-cl might also be adding the 
resource-dir silently like Clangd?

> The `compile-commands.json` seems to be generated for the MSVC  or 
> hand-crafted, I wonder if it may be a difference between the clang frontend 
> and msvc (unlikely, but just to be sure).

Sorry, I confused things because I changed the generator to output "cl" instead 
of "clang-cl" just as a test. I wrote the generator so you can say that I 
hand-crafted the generator? ;) Anyway, I've seen no difference between using cl 
or clang-cl in the compile_commands.json

> The `compile_commands.json` does not even mention the `-resource-dir`, I 
> wonder why would adding it break anything? The only thing that comes to mind 
> is some of the microsoft headers being named the same as the clang headers, 
> but having more stuff in them (the `uintptr_t` that's missing). Is that the 
> case?

Here is the preprocessor output when I run the "win installer" clang-cl (no 
-resource-dir):

  # 48 "C:\\Program Files (x86)\\Microsoft Visual 
Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include\\vcruntime.h" 
2 3
  # 1 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 1 3
  # 32 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 3   
#include_next  
  # 1 "C:\\Program Files (x86)\\Microsoft Visual 
Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include\\vadefs.h" 1 3
  ...
  typedef unsigned __int64 uintptr_t;

Here is the preprocessor output when I run the custom-built clang-cl (no 
-resource-dir, but with /I"C:\Program Files\LLVM\lib\clang\8.0.0\include"):

  # 48 "C:\\Program Files (x86)\\Microsoft Visual 
Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include\\vcruntime.h" 
2 3
  # 1 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 1 3
  # 32 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 3  
#include_next  
  # 1 
"D:\\git\\llvm\\build-vs2017\\RelWithDebInfo\\lib\\clang\\8.0.0\\include\\vadefs.h"
 1 3
  # 33 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 2 3
  # 49 "C:\\Program Files (x86)\\Microsoft Visual 
Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include\\vcruntime.h" 
2 3
  ...
  (MSVC's vadefs.h never included)

So the #include_next in the presence of two vadefs.h from different Clang 
messes it up. It seems wrong to have this include in the compile commands: 
/I"C:\Program Files\LLVM\lib\clang\8.0.0\include", right? Because it will 
always clash with any other Clang's internal headers. I'll check if we can 
remove that flag in the build or if there was some special reason it was there 
that could impact the solution for the problem we're trying to solve here.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54872/new/

https://reviews.llvm.org/D54872



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

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D55395#1322244 , @ABataev wrote:

> This is wrong, the original implementation is correct and should not be 
> changed.


The original implementation looks pretty clearly wrong to me, but I think this 
is wrong too. It looks like what was intended here was probably to label the 
two children as "combiner" and "initializer" respectively. As-is, the 
"combiner"  text on the `OMPDeclareReductionDecl` line means nothing at all.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55395/new/

https://reviews.llvm.org/D55395



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

[PATCH] D55405: [CMake] Use hidden visibility for static libc++ in Fuchsia

[Petr Hosek via Phabricator via cfe-commits]

phosek created this revision.
phosek added a reviewer: mcgrathr.
Herald added subscribers: cfe-commits, mgorny.
Herald added a reviewer: EricWF.

This is enables the use of libc++ in contexts such as device drivers.


Repository:
  rC Clang

https://reviews.llvm.org/D55405

Files:
  clang/cmake/caches/Fuchsia-stage2.cmake


Index: clang/cmake/caches/Fuchsia-stage2.cmake
===
--- clang/cmake/caches/Fuchsia-stage2.cmake
+++ clang/cmake/caches/Fuchsia-stage2.cmake
@@ -116,6 +116,7 @@
 set(RUNTIMES_${target}-fuchsia_LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
 set(RUNTIMES_${target}-fuchsia_LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE 
BOOL "")
 
set(RUNTIMES_${target}-fuchsia_LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY OFF 
CACHE BOOL "")
+set(RUNTIMES_${target}-fuchsia_LIBCXX_HIDDEN_VISIBILITY_IN_STATIC_LIBRARY 
ON CACHE BOOL "")
 set(RUNTIMES_${target}-fuchsia_LIBCXX_ABI_VERSION 2 CACHE STRING "")
   endforeach()
 


Index: clang/cmake/caches/Fuchsia-stage2.cmake
===
--- clang/cmake/caches/Fuchsia-stage2.cmake
+++ clang/cmake/caches/Fuchsia-stage2.cmake
@@ -116,6 +116,7 @@
 set(RUNTIMES_${target}-fuchsia_LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
 set(RUNTIMES_${target}-fuchsia_LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
 set(RUNTIMES_${target}-fuchsia_LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY OFF CACHE BOOL "")
+set(RUNTIMES_${target}-fuchsia_LIBCXX_HIDDEN_VISIBILITY_IN_STATIC_LIBRARY ON CACHE BOOL "")
 set(RUNTIMES_${target}-fuchsia_LIBCXX_ABI_VERSION 2 CACHE STRING "")
   endforeach()
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55007: [Analyzer] Constraint Manager - Calculate Effective Range for Differences

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I just generally wish for a function that would return the most specific known 
range for the given arbitrary symbol. This is something we can put, for 
example, into visitor path notes.

Eg., imagine "Assuming x is greater than 5" -> "Tainted array index constrained 
to [5, 6] U {8} U [10, 12]" -> "Potential buffer overflow: accessing array of 
11 elements with a tainted index".

`getRange()` is a great name for such function. For now i think it kinda works 
for atomic symbols with a few extra goodies on top of that. If we generally 
move towards that goal, i think it would be great.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55007/new/

https://reviews.llvm.org/D55007



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

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177080.
Szelethus added a comment.
This revision is now accepted and ready to land.

- Rebased to master.
- Adding functions calls with the `noreturn` attribute to the guards.
- Shamelessly stole all sorts of assert-like function names from 
`NoReturnFunctionChecker`.
- Added new test cases accordingly.

This should be it! Sorry for being so slow with this -- I genuinely struggled a 
lot with macros before ultimately giving up.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51866/new/

https://reviews.llvm.org/D51866

Files:
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  test/Analysis/cxx-uninitialized-object-unguarded-access.cpp

Index: test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
===
--- /dev/null
+++ test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
@@ -0,0 +1,440 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:IgnoreGuardedFields=true \
+// RUN:   -std=c++11 -verify  %s
+
+//===--===//
+// Helper functions for tests.
+//===--===//
+
+[[noreturn]] void halt();
+
+void assert(int b) {
+  if (!b)
+halt();
+}
+
+int rand();
+
+//===--===//
+// Tests for fields properly guarded by asserts.
+//===--===//
+
+class NoUnguardedFieldsTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+private:
+  int Volume, Area;
+  Kind K;
+
+public:
+  NoUnguardedFieldsTest(Kind K) : K(K) {
+switch (K) {
+case V:
+  Volume = 0;
+  break;
+case A:
+  Area = 0;
+  break;
+}
+  }
+
+  void operator-() {
+assert(K == Kind::A);
+(void)Area;
+  }
+
+  void operator+() {
+assert(K == Kind::V);
+(void)Volume;
+  }
+};
+
+void fNoUnguardedFieldsTest() {
+  NoUnguardedFieldsTest T1(NoUnguardedFieldsTest::Kind::A);
+  NoUnguardedFieldsTest T2(NoUnguardedFieldsTest::Kind::V);
+}
+
+class NoUngardedFieldsNoReturnFuncCalledTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+private:
+  int Volume, Area;
+  Kind K;
+
+public:
+  NoUngardedFieldsNoReturnFuncCalledTest(Kind K) : K(K) {
+switch (K) {
+case V:
+  Volume = 0;
+  break;
+case A:
+  Area = 0;
+  break;
+}
+  }
+
+  void operator-() {
+halt();
+(void)Area;
+  }
+
+  void operator+() {
+halt();
+(void)Volume;
+  }
+};
+
+void fNoUngardedFieldsNoReturnFuncCalledTest() {
+  NoUngardedFieldsNoReturnFuncCalledTest
+T1(NoUngardedFieldsNoReturnFuncCalledTest::Kind::A);
+  NoUngardedFieldsNoReturnFuncCalledTest
+T2(NoUngardedFieldsNoReturnFuncCalledTest::Kind::V);
+}
+
+class NoUnguardedFieldsWithUndefMethodTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+private:
+  int Volume, Area;
+  Kind K;
+
+public:
+  NoUnguardedFieldsWithUndefMethodTest(Kind K) : K(K) {
+switch (K) {
+case V:
+  Volume = 0;
+  break;
+case A:
+  Area = 0;
+  break;
+}
+  }
+
+  void operator-() {
+assert(K == Kind::A);
+(void)Area;
+  }
+
+  void operator+() {
+assert(K == Kind::V);
+(void)Volume;
+  }
+
+  // We're checking method definitions for guards, so this is a no-crash test
+  // whether we handle methods without definitions.
+  void methodWithoutDefinition();
+};
+
+void fNoUnguardedFieldsWithUndefMethodTest() {
+  NoUnguardedFieldsWithUndefMethodTest
+  T1(NoUnguardedFieldsWithUndefMethodTest::Kind::A);
+  NoUnguardedFieldsWithUndefMethodTest
+  T2(NoUnguardedFieldsWithUndefMethodTest::Kind::V);
+}
+
+class UnguardedFieldThroughMethodTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+private:
+  int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}}
+  Kind K;
+
+public:
+  UnguardedFieldThroughMethodTest(Kind K) : K(K) {
+switch (K) {
+case V:
+  Volume = 0;
+  break;
+case A:
+  Area = 0; // expected-warning {{1 uninitialized field}}
+  break;
+}
+  }
+
+  void operator-() {
+assert(K == Kind::A);
+(void)Area;
+  }
+
+  void operator+() {
+(void)Volume;
+  }
+};
+
+void fUnguardedFieldThroughMethodTest() {
+  UnguardedFieldThroughMethodTest T1(UnguardedFieldThroughMethodTest::Kind::A);
+}
+
+class UnguardedPublicFieldsTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+public:
+  // Note that fields are public.
+  int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}}
+  Kind K;
+
+public:
+  UnguardedPublicFieldsTest(Kind K) : K(K) {
+   

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D54438#1322266 , @NoQ wrote:

> In D54438#1315953 , @Szelethus wrote:
>
> > - ✔️ There are in fact a variety of checkers that contain subcheckers like 
> > `MallocChecker`, which I think is all good, but the concept that if a 
> > subchecker is enabled it only sort of registeres the main checker (in 
> > `MallocChecker`'s case it enables modeling, but not reporting) is very hard 
> > to follow. I propose that all such checkers should clearly have a base, 
> > called something `DynamicMemoryModeling`, or `IteratorModeling` etc.
>
>
> This will be the most time-consuming part, as i imagine. You'll have to split 
> each callback into two callbacks in two different checkers (`PostCall` vs. 
> `PostStmt` doesn't work!) and make sure that they are called in the 
> correct order. I expect most modeling to go into `PostStmt` 
> ("post-conditions") and most checking go to `PreStmt` ("pre-conditions"). 
> I.e., "if the pre-condition of the statement is not fulfilled, the checker 
> reports a bug. Otherwise, the model enforces the post-condition after the 
> statement"). Some code (eg., on which particular statements does the checker 
> react?) might be annoying to de-duplicate. Not sure how callbacks that don't 
> have pre/post variants will behave.


Actually, it's already implemented, and the beauty of it is that it requires 
pretty much no change to the checker files. `MallocChecker` is a mess, but 
splitting it up is avoidable in order to pull this off. I'll probably upload a 
patch in the coming days with my proposed solution. Super excited about it, 
actually!

Side note: Before realizing this, I worked out a neat plan to split up 
`MallocChecker` that addresses all of the issues you mentioned above. I'll 
share once it I get the time to put together a nice looking proposal -- since 
everything is so intertwined, I really have to go into tiny details with it, a 
high level "gonna along the lines of this and that" won't cut it. I already 
have a bunch of txt files, whiteboards and papers filled with it.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54438/new/

https://reviews.llvm.org/D54438



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

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:127
   void evalMemset(CheckerContext , const CallExpr *CE) const;
+  void evalExplicitBzero(CheckerContext , const CallExpr *CE) const;
 

I guess let's do just `evalBzero`?



Comment at: test/Analysis/string.c:1405-1406
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str + 2, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{FALSE}}
+}

Let's also add the true statement. I.e., do we know here that the actual length 
is 2?



Comment at: test/Analysis/string.c:1424
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void explicit_bzero3_out_ofbound() {

You can hide `exected-warning`s under `#ifdef`s while leaving the rest of the 
test intact. Eg.,

```
x = 0;
y = 1 / x;
#ifdef CHECK_DIVIDE_ZERO
// expected-warning@-2{{Division by zero}}
#endif
```

It would be cool to see how this test performs without suppressing out-of-bound 
warnings.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592



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

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D54438#1315953 , @Szelethus wrote:

> - ✔️ There are in fact a variety of checkers that contain subcheckers like 
> `MallocChecker`, which I think is all good, but the concept that if a 
> subchecker is enabled it only sort of registeres the main checker (in 
> `MallocChecker`'s case it enables modeling, but not reporting) is very hard 
> to follow. I propose that all such checkers should clearly have a base, 
> called something `DynamicMemoryModeling`, or `IteratorModeling` etc.


This will be the most time-consuming part, as i imagine. You'll have to split 
each callback into two callbacks in two different checkers (`PostCall` vs. 
`PostStmt` doesn't work!) and make sure that they are called in the 
correct order. I expect most modeling to go into `PostStmt` ("post-conditions") 
and most checking go to `PreStmt` ("pre-conditions"). I.e., "if the 
pre-condition of the statement is not fulfilled, the checker reports a bug. 
Otherwise, the model enforces the post-condition after the statement"). Some 
code (eg., on which particular statements does the checker react?) might be 
annoying to de-duplicate. Not sure how callbacks that don't have pre/post 
variants will behave.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54438/new/

https://reviews.llvm.org/D54438



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

[PATCH] D53701: [Analyzer] Record and process comparison of symbols instead of iterator positions in interator checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D53701#1318566 , 
@baloghadamsoftware wrote:

> In D53701#1315242 , @NoQ wrote:
>
> >
>
>
> When using iterator adapters of the STL the bugs may also be filtered out by 
> the analyzer if this option is enabled.


Mmm, are you talking about `c++-container-inlining`? This option currently 
prevents inlining of container methods. STL iterators shouldn't be affected, 
and even if they were affected, they will simply fall back to conservative 
evaluation.

On the other hand, this is how i *want* this option to work. I.e., instead of 
suppressing inlining, it should suppress bugs via visitors when an 
//interesting// event happen within a container, where "interesting" is 
potentially checker-specific and defined in every visitor separately. I believe 
that most checkers will be unaffected. Additionally, it still doesn't affect 
iterators.

> I think that only tracking the inner iterator and treating the outer iterator 
> as a non-iterator is a nightmare from the user's perspective: all detections 
> seem to be "internal" errors of the underlying API and thus regarded as 
> "probably false positives". (...) The user must see the errors on the topmost 
> level whenever possible to understand them.

Well, i mean, whenever you are inlining a method,  you //are// exposing details 
of the "internals" of the inlined API to the user. The only difference is 
whether this API itself deals with iterators. This sounds to me as if we try 
not to inline iterator methods in general. Or try really hard to prevent 
desynchronization between the two models.

Ok, how about an eager state split for now?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53701/new/

https://reviews.llvm.org/D53701



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

[PATCH] D55398: Re-order content from InitListExpr

[Stephen Kelly via Phabricator via cfe-commits]

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

This causes no change in the output of ast-dump-stmt.cpp due to the way
child nodes are printed with a delay.


Repository:
  rC Clang

https://reviews.llvm.org/D55398

Files:
  lib/AST/ASTDumper.cpp


Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1979,16 +1979,16 @@
 }
 
 void ASTDumper::VisitInitListExpr(const InitListExpr *ILE) {
+  if (auto *Field = ILE->getInitializedFieldInUnion()) {
+OS << " field ";
+NodeDumper.dumpBareDeclRef(Field);
+  }
   if (auto *Filler = ILE->getArrayFiller()) {
 dumpChild([=] {
   OS << "array filler";
   dumpStmt(Filler);
 });
   }
-  if (auto *Field = ILE->getInitializedFieldInUnion()) {
-OS << " field ";
-NodeDumper.dumpBareDeclRef(Field);
-  }
 }
 
 void ASTDumper::VisitUnaryOperator(const UnaryOperator *Node) {


Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1979,16 +1979,16 @@
 }
 
 void ASTDumper::VisitInitListExpr(const InitListExpr *ILE) {
+  if (auto *Field = ILE->getInitializedFieldInUnion()) {
+OS << " field ";
+NodeDumper.dumpBareDeclRef(Field);
+  }
   if (auto *Filler = ILE->getArrayFiller()) {
 dumpChild([=] {
   OS << "array filler";
   dumpStmt(Filler);
 });
   }
-  if (auto *Field = ILE->getInitializedFieldInUnion()) {
-OS << " field ";
-NodeDumper.dumpBareDeclRef(Field);
-  }
 }
 
 void ASTDumper::VisitUnaryOperator(const UnaryOperator *Node) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348553 - Add test for InitListExpr

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Thu Dec  6 16:08:14 2018
New Revision: 348553

URL: http://llvm.org/viewvc/llvm-project?rev=348553=rev
Log:
Add test for InitListExpr

Modified:
cfe/trunk/test/AST/ast-dump-stmt.cpp

Modified: cfe/trunk/test/AST/ast-dump-stmt.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-stmt.cpp?rev=348553=348552=348553=diff
==
--- cfe/trunk/test/AST/ast-dump-stmt.cpp (original)
+++ cfe/trunk/test/AST/ast-dump-stmt.cpp Thu Dec  6 16:08:14 2018
@@ -81,3 +81,21 @@ void TestDependentScopeMemberExpr() {
 // CHECK: FunctionTemplateDecl {{.*}} TestDependentScopeMemberExpr
 // CHECK: CXXDependentScopeMemberExpr {{.*}} lvalue .member
 // CHECK: CXXDependentScopeMemberExpr {{.*}} lvalue ->member
+
+union U {
+  int i;
+  long l;
+};
+
+void TestUnionInitList()
+{
+  U us[3] = {1};
+// Check: VarDecl {{.+}}  col:5 us 'U [3]' cinit
+// Check-NEXT: `-InitListExpr {{.+}}  'U [3]'
+// Check-NEXT:   |-array filler
+// Check-NEXT:   | `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
+// Check-NEXT:   |-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
+// Check-NEXT:   | `-IntegerLiteral {{.+}}  'int' 1
+
+
+}


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

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Alexey Bataev via Phabricator via cfe-commits]

ABataev requested changes to this revision.
ABataev added a comment.
This revision now requires changes to proceed.

This is wrong, the original implementation is correct and should not be changed.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55395/new/

https://reviews.llvm.org/D55395



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

[PATCH] D55382: Make -Wstring-plus-int warns even if when the result is not out of bounds

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

The usual process for adding / tweaking warnings is to build some large 
open-source project with the new / tweaked warning and note true and false 
positive rate. IIRC I did this on Chromium when I added the warning, and the 
out-of-bounds check prevented false positives without removing true positives. 
(...or maybe Richard asked for it during review? Did you try and find the 
review where this got added?)

That is, can you try building Chromium with this tweak and see what it does to 
true and false positive rate?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55382/new/

https://reviews.llvm.org/D55382



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

[PATCH] D55339: NFC: Move VisitExpr code to dumpStmt

[Stephen Kelly via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348546: NFC: Move VisitExpr code to dumpStmt (authored by 
steveire, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55339?vs=176870=177074#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55339/new/

https://reviews.llvm.org/D55339

Files:
  lib/AST/ASTDumper.cpp

Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -381,7 +381,6 @@
 void VisitOMPExecutableDirective(const OMPExecutableDirective *Node);
 
 // Exprs
-void VisitExpr(const Expr *Node);
 void VisitCastExpr(const CastExpr *Node);
 void VisitImplicitCastExpr(const ImplicitCastExpr *Node);
 void VisitDeclRefExpr(const DeclRefExpr *Node);
@@ -417,7 +416,6 @@
 void VisitExprWithCleanups(const ExprWithCleanups *Node);
 void VisitUnresolvedLookupExpr(const UnresolvedLookupExpr *Node);
 void VisitLambdaExpr(const LambdaExpr *Node) {
-  VisitExpr(Node);
   dumpDecl(Node->getLambdaClass());
 }
 void VisitSizeOfPackExpr(const SizeOfPackExpr *Node);
@@ -1733,6 +1731,44 @@
 NodeDumper.dumpPointer(S);
 NodeDumper.dumpSourceRange(S->getSourceRange());
 
+if (const auto *E = dyn_cast(S)) {
+  NodeDumper.dumpType(E->getType());
+
+  {
+ColorScope Color(OS, ShowColors, ValueKindColor);
+switch (E->getValueKind()) {
+case VK_RValue:
+  break;
+case VK_LValue:
+  OS << " lvalue";
+  break;
+case VK_XValue:
+  OS << " xvalue";
+  break;
+}
+  }
+
+  {
+ColorScope Color(OS, ShowColors, ObjectKindColor);
+switch (E->getObjectKind()) {
+case OK_Ordinary:
+  break;
+case OK_BitField:
+  OS << " bitfield";
+  break;
+case OK_ObjCProperty:
+  OS << " objcproperty";
+  break;
+case OK_ObjCSubscript:
+  OS << " objcsubscript";
+  break;
+case OK_VectorComponent:
+  OS << " vectorcomponent";
+  break;
+}
+  }
+}
+
 ConstStmtVisitor::Visit(S);
 
 // Some statements have custom mechanisms for dumping their children.
@@ -1835,44 +1871,6 @@
 //  Expr dumping methods.
 //===--===//
 
-void ASTDumper::VisitExpr(const Expr *Node) {
-  NodeDumper.dumpType(Node->getType());
-
-  {
-ColorScope Color(OS, ShowColors, ValueKindColor);
-switch (Node->getValueKind()) {
-case VK_RValue:
-  break;
-case VK_LValue:
-  OS << " lvalue";
-  break;
-case VK_XValue:
-  OS << " xvalue";
-  break;
-}
-  }
-
-  {
-ColorScope Color(OS, ShowColors, ObjectKindColor);
-switch (Node->getObjectKind()) {
-case OK_Ordinary:
-  break;
-case OK_BitField:
-  OS << " bitfield";
-  break;
-case OK_ObjCProperty:
-  OS << " objcproperty";
-  break;
-case OK_ObjCSubscript:
-  OS << " objcsubscript";
-  break;
-case OK_VectorComponent:
-  OS << " vectorcomponent";
-  break;
-}
-  }
-}
-
 static void dumpBasePath(raw_ostream , const CastExpr *Node) {
   if (Node->path_empty())
 return;
@@ -1899,7 +1897,6 @@
 }
 
 void ASTDumper::VisitCastExpr(const CastExpr *Node) {
-  VisitExpr(Node);
   OS << " <";
   {
 ColorScope Color(OS, ShowColors, CastColor);
@@ -1916,8 +1913,6 @@
 }
 
 void ASTDumper::VisitDeclRefExpr(const DeclRefExpr *Node) {
-  VisitExpr(Node);
-
   OS << " ";
   NodeDumper.dumpBareDeclRef(Node->getDecl());
   if (Node->getDecl() != Node->getFoundDecl()) {
@@ -1928,7 +1923,6 @@
 }
 
 void ASTDumper::VisitUnresolvedLookupExpr(const UnresolvedLookupExpr *Node) {
-  VisitExpr(Node);
   OS << " (";
   if (!Node->requiresADL())
 OS << "no ";
@@ -1943,8 +1937,6 @@
 }
 
 void ASTDumper::VisitObjCIvarRefExpr(const ObjCIvarRefExpr *Node) {
-  VisitExpr(Node);
-
   {
 ColorScope Color(OS, ShowColors, DeclKindNameColor);
 OS << " " << Node->getDecl()->getDeclKindName() << "Decl";
@@ -1956,46 +1948,37 @@
 }
 
 void ASTDumper::VisitPredefinedExpr(const PredefinedExpr *Node) {
-  VisitExpr(Node);
   OS << " " << PredefinedExpr::getIdentKindName(Node->getIdentKind());
 }
 
 void ASTDumper::VisitCharacterLiteral(const CharacterLiteral *Node) {
-  VisitExpr(Node);
   ColorScope Color(OS, ShowColors, ValueColor);
   OS << " " << Node->getValue();
 }
 
 void ASTDumper::VisitIntegerLiteral(const IntegerLiteral *Node) {
-  VisitExpr(Node);
-
   bool isSigned = Node->getType()->isSignedIntegerType();
   ColorScope Color(OS, ShowColors, ValueColor);
   OS << " " << Node->getValue().toString(10, isSigned);
 }
 
 void ASTDumper::VisitFixedPointLiteral(const FixedPointLiteral *Node) {
-  VisitExpr(Node);
-
   ColorScope Color(OS, ShowColors, ValueColor);
   OS << " " << 

[PATCH] D55338: NFC: Move VisitStmt code to dumpStmt

[Stephen Kelly via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348545: NFC: Move VisitStmt code to dumpStmt (authored by 
steveire, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55338?vs=176869=177073#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55338/new/

https://reviews.llvm.org/D55338

Files:
  lib/AST/ASTDumper.cpp

Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -366,7 +366,6 @@
 void VisitBlockDecl(const BlockDecl *D);
 
 // Stmts.
-void VisitStmt(const Stmt *Node);
 void VisitDeclStmt(const DeclStmt *Node);
 void VisitAttributedStmt(const AttributedStmt *Node);
 void VisitIfStmt(const IfStmt *Node);
@@ -1727,6 +1726,12 @@
   OS << "<<>>";
   return;
 }
+{
+  ColorScope Color(OS, ShowColors, StmtColor);
+  OS << S->getStmtClassName();
+}
+NodeDumper.dumpPointer(S);
+NodeDumper.dumpSourceRange(S->getSourceRange());
 
 ConstStmtVisitor::Visit(S);
 
@@ -1740,17 +1745,7 @@
   });
 }
 
-void ASTDumper::VisitStmt(const Stmt *Node) {
-  {
-ColorScope Color(OS, ShowColors, StmtColor);
-OS << Node->getStmtClassName();
-  }
-  NodeDumper.dumpPointer(Node);
-  NodeDumper.dumpSourceRange(Node->getSourceRange());
-}
-
 void ASTDumper::VisitDeclStmt(const DeclStmt *Node) {
-  VisitStmt(Node);
   for (DeclStmt::const_decl_iterator I = Node->decl_begin(),
  E = Node->decl_end();
I != E; ++I)
@@ -1758,7 +1753,6 @@
 }
 
 void ASTDumper::VisitAttributedStmt(const AttributedStmt *Node) {
-  VisitStmt(Node);
   for (ArrayRef::iterator I = Node->getAttrs().begin(),
 E = Node->getAttrs().end();
I != E; ++I)
@@ -1766,7 +1760,6 @@
 }
 
 void ASTDumper::VisitIfStmt(const IfStmt *Node) {
-  VisitStmt(Node);
   if (Node->hasInitStorage())
 OS << " has_init";
   if (Node->hasVarStorage())
@@ -1776,7 +1769,6 @@
 }
 
 void ASTDumper::VisitSwitchStmt(const SwitchStmt *Node) {
-  VisitStmt(Node);
   if (Node->hasInitStorage())
 OS << " has_init";
   if (Node->hasVarStorage())
@@ -1784,35 +1776,29 @@
 }
 
 void ASTDumper::VisitWhileStmt(const WhileStmt *Node) {
-  VisitStmt(Node);
   if (Node->hasVarStorage())
 OS << " has_var";
 }
 
 void ASTDumper::VisitLabelStmt(const LabelStmt *Node) {
-  VisitStmt(Node);
   OS << " '" << Node->getName() << "'";
 }
 
 void ASTDumper::VisitGotoStmt(const GotoStmt *Node) {
-  VisitStmt(Node);
   OS << " '" << Node->getLabel()->getName() << "'";
   NodeDumper.dumpPointer(Node->getLabel());
 }
 
 void ASTDumper::VisitCXXCatchStmt(const CXXCatchStmt *Node) {
-  VisitStmt(Node);
   dumpDecl(Node->getExceptionDecl());
 }
 
 void ASTDumper::VisitCaseStmt(const CaseStmt *Node) {
-  VisitStmt(Node);
   if (Node->caseStmtIsGNURange())
 OS << " gnu_range";
 }
 
 void ASTDumper::VisitCapturedStmt(const CapturedStmt *Node) {
-  VisitStmt(Node);
   dumpDecl(Node->getCapturedDecl());
 }
 
@@ -1822,7 +1808,6 @@
 
 void ASTDumper::VisitOMPExecutableDirective(
 const OMPExecutableDirective *Node) {
-  VisitStmt(Node);
   for (auto *C : Node->clauses()) {
 dumpChild([=] {
   if (!C) {
@@ -1851,7 +1836,6 @@
 //===--===//
 
 void ASTDumper::VisitExpr(const Expr *Node) {
-  VisitStmt(Node);
   NodeDumper.dumpType(Node->getType());
 
   {
@@ -2277,7 +2261,6 @@
 }
 
 void ASTDumper::VisitObjCAtCatchStmt(const ObjCAtCatchStmt *Node) {
-  VisitStmt(Node);
   if (const VarDecl *CatchParam = Node->getCatchParamDecl())
 dumpDecl(CatchParam);
   else
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348546 - NFC: Move VisitExpr code to dumpStmt

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Thu Dec  6 15:33:33 2018
New Revision: 348546

URL: http://llvm.org/viewvc/llvm-project?rev=348546=rev
Log:
NFC: Move VisitExpr code to dumpStmt

Summary:
The call is duplicated in the handlers of all Expr subclasses.

This change makes it easy to split statement handling out to
TextNodeDumper.

Reviewers: aaron.ballman

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D55339

Modified:
cfe/trunk/lib/AST/ASTDumper.cpp

Modified: cfe/trunk/lib/AST/ASTDumper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348546=348545=348546=diff
==
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)
+++ cfe/trunk/lib/AST/ASTDumper.cpp Thu Dec  6 15:33:33 2018
@@ -381,7 +381,6 @@ namespace  {
 void VisitOMPExecutableDirective(const OMPExecutableDirective *Node);
 
 // Exprs
-void VisitExpr(const Expr *Node);
 void VisitCastExpr(const CastExpr *Node);
 void VisitImplicitCastExpr(const ImplicitCastExpr *Node);
 void VisitDeclRefExpr(const DeclRefExpr *Node);
@@ -417,7 +416,6 @@ namespace  {
 void VisitExprWithCleanups(const ExprWithCleanups *Node);
 void VisitUnresolvedLookupExpr(const UnresolvedLookupExpr *Node);
 void VisitLambdaExpr(const LambdaExpr *Node) {
-  VisitExpr(Node);
   dumpDecl(Node->getLambdaClass());
 }
 void VisitSizeOfPackExpr(const SizeOfPackExpr *Node);
@@ -1733,6 +1731,44 @@ void ASTDumper::dumpStmt(const Stmt *S)
 NodeDumper.dumpPointer(S);
 NodeDumper.dumpSourceRange(S->getSourceRange());
 
+if (const auto *E = dyn_cast(S)) {
+  NodeDumper.dumpType(E->getType());
+
+  {
+ColorScope Color(OS, ShowColors, ValueKindColor);
+switch (E->getValueKind()) {
+case VK_RValue:
+  break;
+case VK_LValue:
+  OS << " lvalue";
+  break;
+case VK_XValue:
+  OS << " xvalue";
+  break;
+}
+  }
+
+  {
+ColorScope Color(OS, ShowColors, ObjectKindColor);
+switch (E->getObjectKind()) {
+case OK_Ordinary:
+  break;
+case OK_BitField:
+  OS << " bitfield";
+  break;
+case OK_ObjCProperty:
+  OS << " objcproperty";
+  break;
+case OK_ObjCSubscript:
+  OS << " objcsubscript";
+  break;
+case OK_VectorComponent:
+  OS << " vectorcomponent";
+  break;
+}
+  }
+}
+
 ConstStmtVisitor::Visit(S);
 
 // Some statements have custom mechanisms for dumping their children.
@@ -1835,44 +1871,6 @@ void ASTDumper::VisitOMPExecutableDirect
 //  Expr dumping methods.
 
//===--===//
 
-void ASTDumper::VisitExpr(const Expr *Node) {
-  NodeDumper.dumpType(Node->getType());
-
-  {
-ColorScope Color(OS, ShowColors, ValueKindColor);
-switch (Node->getValueKind()) {
-case VK_RValue:
-  break;
-case VK_LValue:
-  OS << " lvalue";
-  break;
-case VK_XValue:
-  OS << " xvalue";
-  break;
-}
-  }
-
-  {
-ColorScope Color(OS, ShowColors, ObjectKindColor);
-switch (Node->getObjectKind()) {
-case OK_Ordinary:
-  break;
-case OK_BitField:
-  OS << " bitfield";
-  break;
-case OK_ObjCProperty:
-  OS << " objcproperty";
-  break;
-case OK_ObjCSubscript:
-  OS << " objcsubscript";
-  break;
-case OK_VectorComponent:
-  OS << " vectorcomponent";
-  break;
-}
-  }
-}
-
 static void dumpBasePath(raw_ostream , const CastExpr *Node) {
   if (Node->path_empty())
 return;
@@ -1899,7 +1897,6 @@ static void dumpBasePath(raw_ostream 
 }
 
 void ASTDumper::VisitCastExpr(const CastExpr *Node) {
-  VisitExpr(Node);
   OS << " <";
   {
 ColorScope Color(OS, ShowColors, CastColor);
@@ -1916,8 +1913,6 @@ void ASTDumper::VisitImplicitCastExpr(co
 }
 
 void ASTDumper::VisitDeclRefExpr(const DeclRefExpr *Node) {
-  VisitExpr(Node);
-
   OS << " ";
   NodeDumper.dumpBareDeclRef(Node->getDecl());
   if (Node->getDecl() != Node->getFoundDecl()) {
@@ -1928,7 +1923,6 @@ void ASTDumper::VisitDeclRefExpr(const D
 }
 
 void ASTDumper::VisitUnresolvedLookupExpr(const UnresolvedLookupExpr *Node) {
-  VisitExpr(Node);
   OS << " (";
   if (!Node->requiresADL())
 OS << "no ";
@@ -1943,8 +1937,6 @@ void ASTDumper::VisitUnresolvedLookupExp
 }
 
 void ASTDumper::VisitObjCIvarRefExpr(const ObjCIvarRefExpr *Node) {
-  VisitExpr(Node);
-
   {
 ColorScope Color(OS, ShowColors, DeclKindNameColor);
 OS << " " << Node->getDecl()->getDeclKindName() << "Decl";
@@ -1956,46 +1948,37 @@ void ASTDumper::VisitObjCIvarRefExpr(con
 }
 
 void ASTDumper::VisitPredefinedExpr(const PredefinedExpr *Node) {
-  VisitExpr(Node);
   OS << " " << PredefinedExpr::getIdentKindName(Node->getIdentKind());
 }
 
 void ASTDumper::VisitCharacterLiteral(const 

r348545 - NFC: Move VisitStmt code to dumpStmt

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Thu Dec  6 15:33:27 2018
New Revision: 348545

URL: http://llvm.org/viewvc/llvm-project?rev=348545=rev
Log:
NFC: Move VisitStmt code to dumpStmt

Summary: This call is duplicated in Visits of all direct subclasses of Stmt.

Reviewers: aaron.ballman

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D55338

Modified:
cfe/trunk/lib/AST/ASTDumper.cpp

Modified: cfe/trunk/lib/AST/ASTDumper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348545=348544=348545=diff
==
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)
+++ cfe/trunk/lib/AST/ASTDumper.cpp Thu Dec  6 15:33:27 2018
@@ -366,7 +366,6 @@ namespace  {
 void VisitBlockDecl(const BlockDecl *D);
 
 // Stmts.
-void VisitStmt(const Stmt *Node);
 void VisitDeclStmt(const DeclStmt *Node);
 void VisitAttributedStmt(const AttributedStmt *Node);
 void VisitIfStmt(const IfStmt *Node);
@@ -1727,6 +1726,12 @@ void ASTDumper::dumpStmt(const Stmt *S)
   OS << "<<>>";
   return;
 }
+{
+  ColorScope Color(OS, ShowColors, StmtColor);
+  OS << S->getStmtClassName();
+}
+NodeDumper.dumpPointer(S);
+NodeDumper.dumpSourceRange(S->getSourceRange());
 
 ConstStmtVisitor::Visit(S);
 
@@ -1740,17 +1745,7 @@ void ASTDumper::dumpStmt(const Stmt *S)
   });
 }
 
-void ASTDumper::VisitStmt(const Stmt *Node) {
-  {
-ColorScope Color(OS, ShowColors, StmtColor);
-OS << Node->getStmtClassName();
-  }
-  NodeDumper.dumpPointer(Node);
-  NodeDumper.dumpSourceRange(Node->getSourceRange());
-}
-
 void ASTDumper::VisitDeclStmt(const DeclStmt *Node) {
-  VisitStmt(Node);
   for (DeclStmt::const_decl_iterator I = Node->decl_begin(),
  E = Node->decl_end();
I != E; ++I)
@@ -1758,7 +1753,6 @@ void ASTDumper::VisitDeclStmt(const Decl
 }
 
 void ASTDumper::VisitAttributedStmt(const AttributedStmt *Node) {
-  VisitStmt(Node);
   for (ArrayRef::iterator I = Node->getAttrs().begin(),
 E = Node->getAttrs().end();
I != E; ++I)
@@ -1766,7 +1760,6 @@ void ASTDumper::VisitAttributedStmt(cons
 }
 
 void ASTDumper::VisitIfStmt(const IfStmt *Node) {
-  VisitStmt(Node);
   if (Node->hasInitStorage())
 OS << " has_init";
   if (Node->hasVarStorage())
@@ -1776,7 +1769,6 @@ void ASTDumper::VisitIfStmt(const IfStmt
 }
 
 void ASTDumper::VisitSwitchStmt(const SwitchStmt *Node) {
-  VisitStmt(Node);
   if (Node->hasInitStorage())
 OS << " has_init";
   if (Node->hasVarStorage())
@@ -1784,35 +1776,29 @@ void ASTDumper::VisitSwitchStmt(const Sw
 }
 
 void ASTDumper::VisitWhileStmt(const WhileStmt *Node) {
-  VisitStmt(Node);
   if (Node->hasVarStorage())
 OS << " has_var";
 }
 
 void ASTDumper::VisitLabelStmt(const LabelStmt *Node) {
-  VisitStmt(Node);
   OS << " '" << Node->getName() << "'";
 }
 
 void ASTDumper::VisitGotoStmt(const GotoStmt *Node) {
-  VisitStmt(Node);
   OS << " '" << Node->getLabel()->getName() << "'";
   NodeDumper.dumpPointer(Node->getLabel());
 }
 
 void ASTDumper::VisitCXXCatchStmt(const CXXCatchStmt *Node) {
-  VisitStmt(Node);
   dumpDecl(Node->getExceptionDecl());
 }
 
 void ASTDumper::VisitCaseStmt(const CaseStmt *Node) {
-  VisitStmt(Node);
   if (Node->caseStmtIsGNURange())
 OS << " gnu_range";
 }
 
 void ASTDumper::VisitCapturedStmt(const CapturedStmt *Node) {
-  VisitStmt(Node);
   dumpDecl(Node->getCapturedDecl());
 }
 
@@ -1822,7 +1808,6 @@ void ASTDumper::VisitCapturedStmt(const
 
 void ASTDumper::VisitOMPExecutableDirective(
 const OMPExecutableDirective *Node) {
-  VisitStmt(Node);
   for (auto *C : Node->clauses()) {
 dumpChild([=] {
   if (!C) {
@@ -1851,7 +1836,6 @@ void ASTDumper::VisitOMPExecutableDirect
 
//===--===//
 
 void ASTDumper::VisitExpr(const Expr *Node) {
-  VisitStmt(Node);
   NodeDumper.dumpType(Node->getType());
 
   {
@@ -2277,7 +2261,6 @@ void ASTDumper::VisitObjCBoxedExpr(const
 }
 
 void ASTDumper::VisitObjCAtCatchStmt(const ObjCAtCatchStmt *Node) {
-  VisitStmt(Node);
   if (const VarDecl *CatchParam = Node->getCatchParamDecl())
 dumpDecl(CatchParam);
   else


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

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Stephen Kelly via Phabricator via cfe-commits]

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

Repository:
  rC Clang

https://reviews.llvm.org/D55395

Files:
  lib/AST/ASTDumper.cpp
  test/AST/dump.cpp


Index: test/AST/dump.cpp
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -29,10 +29,6 @@
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
 // CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner initializer
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
-// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_in' 'float'
 // CHECK-NEXT: | |-BinaryOperator {{.+}}  'float' lvalue '='
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_priv' 'float'
 // CHECK-NEXT: | | `-BinaryOperator {{.+}}  'float' '+'
@@ -40,6 +36,10 @@
 // CHECK-NEXT: | |   | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_orig' 'float'
 // CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'float' 

 // CHECK-NEXT: | | `-IntegerLiteral {{.+}}  'int' 15
+// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
+// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
+// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
+// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_in' 'float'
 
 struct S {
   int a, b;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1074,7 +1074,6 @@
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
   OS << " combiner";
-  dumpStmt(D->getCombiner());
   if (auto *Initializer = D->getInitializer()) {
 OS << " initializer";
 switch (D->getInitializerKind()) {
@@ -1089,6 +1088,7 @@
 }
 dumpStmt(Initializer);
   }
+  dumpStmt(D->getCombiner());
 }
 
 void ASTDumper::VisitOMPRequiresDecl(const OMPRequiresDecl *D) {


Index: test/AST/dump.cpp
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -29,10 +29,6 @@
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
 // CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 fun 'float' combiner initializer
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_out' 'float'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
-// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_in' 'float'
 // CHECK-NEXT: | |-BinaryOperator {{.+}}  'float' lvalue '='
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_priv' 'float'
 // CHECK-NEXT: | | `-BinaryOperator {{.+}}  'float' '+'
@@ -40,6 +36,10 @@
 // CHECK-NEXT: | |   | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_orig' 'float'
 // CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'float' 
 // CHECK-NEXT: | | `-IntegerLiteral {{.+}}  'int' 15
+// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
+// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_out' 'float'
+// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
+// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_in' 'float'
 
 struct S {
   int a, b;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1074,7 +1074,6 @@
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
   OS << " combiner";
-  dumpStmt(D->getCombiner());
   if (auto *Initializer = D->getInitializer()) {
 OS << " initializer";
 switch (D->getInitializerKind()) {
@@ -1089,6 +1088,7 @@
 }
 dumpStmt(Initializer);
   }
+  dumpStmt(D->getCombiner());
 }
 
 void ASTDumper::VisitOMPRequiresDecl(const OMPRequiresDecl *D) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55394: Re-order type param children of ObjC nodes

[Stephen Kelly via Phabricator via cfe-commits]

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

Repository:
  rC Clang

https://reviews.llvm.org/D55394

Files:
  lib/AST/ASTDumper.cpp
  test/AST/ast-dump-decl.m


Index: test/AST/ast-dump-decl.m
===
--- test/AST/ast-dump-decl.m
+++ test/AST/ast-dump-decl.m
@@ -85,9 +85,9 @@
 }
 @end
 // CHECK:  ObjCInterfaceDecl{{.*}} TestGenericInterface
-// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}}  col:33 T 'id':'id'
 // CHECK-NEXT:   -super ObjCInterface {{.+}} 'A'
 // CHECK-NEXT:   -ObjCProtocol {{.+}} 'P'
+// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}}  col:33 T 'id':'id'
 
 @implementation TestObjCClass (TestObjCCategoryDecl)
 - (void) bar {
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1592,12 +1592,12 @@
 void ASTDumper::VisitObjCCategoryDecl(const ObjCCategoryDecl *D) {
   NodeDumper.dumpName(D);
   dumpDeclRef(D->getClassInterface());
-  dumpObjCTypeParamList(D->getTypeParamList());
   dumpDeclRef(D->getImplementation());
   for (ObjCCategoryDecl::protocol_iterator I = D->protocol_begin(),
E = D->protocol_end();
I != E; ++I)
 dumpDeclRef(*I);
+  dumpObjCTypeParamList(D->getTypeParamList());
 }
 
 void ASTDumper::VisitObjCCategoryImplDecl(const ObjCCategoryImplDecl *D) {
@@ -1615,12 +1615,12 @@
 
 void ASTDumper::VisitObjCInterfaceDecl(const ObjCInterfaceDecl *D) {
   NodeDumper.dumpName(D);
-  dumpObjCTypeParamList(D->getTypeParamListAsWritten());
   dumpDeclRef(D->getSuperClass(), "super");
 
   dumpDeclRef(D->getImplementation());
   for (auto *Child : D->protocols())
 dumpDeclRef(Child);
+  dumpObjCTypeParamList(D->getTypeParamListAsWritten());
 }
 
 void ASTDumper::VisitObjCImplementationDecl(const ObjCImplementationDecl *D) {


Index: test/AST/ast-dump-decl.m
===
--- test/AST/ast-dump-decl.m
+++ test/AST/ast-dump-decl.m
@@ -85,9 +85,9 @@
 }
 @end
 // CHECK:  ObjCInterfaceDecl{{.*}} TestGenericInterface
-// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}}  col:33 T 'id':'id'
 // CHECK-NEXT:   -super ObjCInterface {{.+}} 'A'
 // CHECK-NEXT:   -ObjCProtocol {{.+}} 'P'
+// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}}  col:33 T 'id':'id'
 
 @implementation TestObjCClass (TestObjCCategoryDecl)
 - (void) bar {
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1592,12 +1592,12 @@
 void ASTDumper::VisitObjCCategoryDecl(const ObjCCategoryDecl *D) {
   NodeDumper.dumpName(D);
   dumpDeclRef(D->getClassInterface());
-  dumpObjCTypeParamList(D->getTypeParamList());
   dumpDeclRef(D->getImplementation());
   for (ObjCCategoryDecl::protocol_iterator I = D->protocol_begin(),
E = D->protocol_end();
I != E; ++I)
 dumpDeclRef(*I);
+  dumpObjCTypeParamList(D->getTypeParamList());
 }
 
 void ASTDumper::VisitObjCCategoryImplDecl(const ObjCCategoryImplDecl *D) {
@@ -1615,12 +1615,12 @@
 
 void ASTDumper::VisitObjCInterfaceDecl(const ObjCInterfaceDecl *D) {
   NodeDumper.dumpName(D);
-  dumpObjCTypeParamList(D->getTypeParamListAsWritten());
   dumpDeclRef(D->getSuperClass(), "super");
 
   dumpDeclRef(D->getImplementation());
   for (auto *Child : D->protocols())
 dumpDeclRef(Child);
+  dumpObjCTypeParamList(D->getTypeParamListAsWritten());
 }
 
 void ASTDumper::VisitObjCImplementationDecl(const ObjCImplementationDecl *D) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55393: Re-order content of template parameter dumps

[Stephen Kelly via Phabricator via cfe-commits]

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

Repository:
  rC Clang

https://reviews.llvm.org/D55393

Files:
  lib/AST/ASTDumper.cpp
  test/AST/ast-dump-decl.cpp


Index: test/AST/ast-dump-decl.cpp
===
--- test/AST/ast-dump-decl.cpp
+++ test/AST/ast-dump-decl.cpp
@@ -327,21 +327,21 @@
 
 // CHECK:  ClassTemplateDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} 
TestTemplateDefaultType
 // CHECK-NEXT:   TemplateTypeParmDecl
-// CHECK-NEXT: TemplateArgument type 'int'
 // CHECK-NEXT: inherited from TemplateTypeParm 0x{{[^ ]*}} 'T'
+// CHECK-NEXT: TemplateArgument type 'int'
 
 // CHECK:  ClassTemplateDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} 
TestTemplateDefaultNonType
 // CHECK-NEXT:   NonTypeTemplateParmDecl
+// CHECK-NEXT: inherited from NonTypeTemplateParm 0x{{[^ ]*}} 'I' 'int'
 // CHECK-NEXT: TemplateArgument expr
 // CHECK-NEXT:   ConstantExpr
 // CHECK-NEXT: IntegerLiteral
-// CHECK-NEXT: inherited from NonTypeTemplateParm 0x{{[^ ]*}} 'I' 'int'
 
 // CHECK:  ClassTemplateDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} 
TestTemplateTemplateDefaultType
 // CHECK-NEXT:   TemplateTemplateParmDecl
 // CHECK-NEXT: TemplateTypeParmDecl
-// CHECK-NEXT: TemplateArgument
 // CHECK-NEXT: inherited from TemplateTemplateParm 0x{{[^ ]*}} 'TT'
+// CHECK-NEXT: TemplateArgument
 
 // PR15220 dump instantiation only once
 namespace testCanonicalTemplate {
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1410,11 +1410,11 @@
   if (D->isParameterPack())
 OS << " ...";
   NodeDumper.dumpName(D);
-  if (D->hasDefaultArgument())
-dumpTemplateArgument(D->getDefaultArgument());
   if (auto *From = D->getDefaultArgStorage().getInheritedFrom())
 dumpDeclRef(From, D->defaultArgumentWasInherited() ? "inherited from"
: "previous");
+  if (D->hasDefaultArgument())
+dumpTemplateArgument(D->getDefaultArgument());
 }
 
 void ASTDumper::VisitNonTypeTemplateParmDecl(const NonTypeTemplateParmDecl *D) 
{
@@ -1423,11 +1423,11 @@
   if (D->isParameterPack())
 OS << " ...";
   NodeDumper.dumpName(D);
-  if (D->hasDefaultArgument())
-dumpTemplateArgument(D->getDefaultArgument());
   if (auto *From = D->getDefaultArgStorage().getInheritedFrom())
 dumpDeclRef(From, D->defaultArgumentWasInherited() ? "inherited from"
: "previous");
+  if (D->hasDefaultArgument())
+dumpTemplateArgument(D->getDefaultArgument());
 }
 
 void ASTDumper::VisitTemplateTemplateParmDecl(
@@ -1437,11 +1437,11 @@
 OS << " ...";
   NodeDumper.dumpName(D);
   dumpTemplateParameters(D->getTemplateParameters());
-  if (D->hasDefaultArgument())
-dumpTemplateArgumentLoc(D->getDefaultArgument());
   if (auto *From = D->getDefaultArgStorage().getInheritedFrom())
 dumpDeclRef(From, D->defaultArgumentWasInherited() ? "inherited from"
: "previous");
+  if (D->hasDefaultArgument())
+dumpTemplateArgumentLoc(D->getDefaultArgument());
 }
 
 void ASTDumper::VisitUsingDecl(const UsingDecl *D) {


Index: test/AST/ast-dump-decl.cpp
===
--- test/AST/ast-dump-decl.cpp
+++ test/AST/ast-dump-decl.cpp
@@ -327,21 +327,21 @@
 
 // CHECK:  ClassTemplateDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} TestTemplateDefaultType
 // CHECK-NEXT:   TemplateTypeParmDecl
-// CHECK-NEXT: TemplateArgument type 'int'
 // CHECK-NEXT: inherited from TemplateTypeParm 0x{{[^ ]*}} 'T'
+// CHECK-NEXT: TemplateArgument type 'int'
 
 // CHECK:  ClassTemplateDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} TestTemplateDefaultNonType
 // CHECK-NEXT:   NonTypeTemplateParmDecl
+// CHECK-NEXT: inherited from NonTypeTemplateParm 0x{{[^ ]*}} 'I' 'int'
 // CHECK-NEXT: TemplateArgument expr
 // CHECK-NEXT:   ConstantExpr
 // CHECK-NEXT: IntegerLiteral
-// CHECK-NEXT: inherited from NonTypeTemplateParm 0x{{[^ ]*}} 'I' 'int'
 
 // CHECK:  ClassTemplateDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} TestTemplateTemplateDefaultType
 // CHECK-NEXT:   TemplateTemplateParmDecl
 // CHECK-NEXT: TemplateTypeParmDecl
-// CHECK-NEXT: TemplateArgument
 // CHECK-NEXT: inherited from TemplateTemplateParm 0x{{[^ ]*}} 'TT'
+// CHECK-NEXT: TemplateArgument
 
 // PR15220 dump instantiation only once
 namespace testCanonicalTemplate {
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1410,11 +1410,11 @@
   if (D->isParameterPack())
 OS << " ...";
   NodeDumper.dumpName(D);
-  if (D->hasDefaultArgument())
-

[PATCH] D55083: Re-arrange content in FunctionDecl dump

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 177069.
steveire added a comment.

Adjust tests


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55083/new/

https://reviews.llvm.org/D55083

Files:
  lib/AST/ASTDumper.cpp
  test/AST/ast-dump-decl.cpp
  test/AST/ast-dump-funcs.cpp
  test/AST/float16.cpp

Index: test/AST/float16.cpp
===
--- test/AST/float16.cpp
+++ test/AST/float16.cpp
@@ -162,8 +162,8 @@
 //CHECK-NEXT: | |   |-DeclRefExpr {{.*}} 'C' lvalue ParmVar {{.*}} 'arg' 'C'
 //CHECK-NEXT: | |   `-FloatingLiteral {{.*}} '_Float16' 2.00e+00
 //CHECK-NEXT: | `-FunctionDecl {{.*}} used func1t '_Float16 (_Float16)'
-//CHECK-NEXT: |   |-TemplateArgument type '_Float16'
 //CHECK-NEXT: |   |-ParmVarDecl {{.*}} used arg '_Float16':'_Float16'
+//CHECK-NEXT: |   |-TemplateArgument type '_Float16'
 //CHECK-NEXT: |   `-CompoundStmt
 //CHECK-NEXT: | `-ReturnStmt
 //CHECK-NEXT: |   `-BinaryOperator {{.*}} '_Float16' '*'
Index: test/AST/ast-dump-funcs.cpp
===
--- test/AST/ast-dump-funcs.cpp
+++ test/AST/ast-dump-funcs.cpp
@@ -56,10 +56,10 @@
 struct T : S { // T is not referenced, but S is
   void f(float, int = 100) override;
   // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 f 'void (float, int)'
+  // CHECK-NEXT: Overrides: [ 0x{{[^ ]*}} S::f 'void (float, int)' ]
   // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:15 'float'
   // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:21 'int' cinit
   // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 100
-  // CHECK-NEXT: Overrides: [ 0x{{[^ ]*}} S::f 'void (float, int)' ]
   // CHECK-NEXT: OverrideAttr
 
   // CHECK: CXXConstructorDecl 0x{{[^ ]*}}  col:8 implicit T 'void (const T &)' inline default_delete noexcept-unevaluated
Index: test/AST/ast-dump-decl.cpp
===
--- test/AST/ast-dump-decl.cpp
+++ test/AST/ast-dump-decl.cpp
@@ -213,20 +213,20 @@
 // CHECK-NEXT: ParmVarDecl{{.*}} 'T'
 // CHECK-NEXT: CompoundStmt
 // CHECK-NEXT:   FunctionDecl{{.*}} TestFunctionTemplate {{.*}}A
-// CHECK-NEXT: TemplateArgument
 // CHECK-NEXT: ParmVarDecl
+// CHECK-NEXT: TemplateArgument
 // CHECK-NEXT: CompoundStmt
 // CHECK-NEXT:   Function{{.*}} 'TestFunctionTemplate' {{.*}}B
 // CHECK-NEXT:   FunctionDecl{{.*}} TestFunctionTemplate {{.*}}C
-// CHECK-NEXT: TemplateArgument
 // CHECK-NEXT: ParmVarDecl
-// CHECK-NEXT:   FunctionDecl{{.*}} TestFunctionTemplate {{.*}}D
 // CHECK-NEXT: TemplateArgument
+// CHECK-NEXT:   FunctionDecl{{.*}} TestFunctionTemplate {{.*}}D
 // CHECK-NEXT: ParmVarDecl
+// CHECK-NEXT: TemplateArgument
 // CHECK-NEXT: CompoundStmt
 // CHECK:  FunctionDecl{{.*}} TestFunctionTemplate {{.*}}B
-// CHECK-NEXT:   TemplateArgument
 // CHECK-NEXT:   ParmVarDecl
+// CHECK-NEXT:   TemplateArgument
 
 namespace testClassTemplateDecl {
   class A { };
@@ -355,8 +355,8 @@
   // CHECK-NEXT:   FunctionDecl{{.*}} TestFunctionTemplate 'void (T)'
   // CHECK-NEXT: ParmVarDecl{{.*}} 'T'
   // CHECK-NEXT:   FunctionDecl{{.*}} TestFunctionTemplate {{.*}}A
-  // CHECK-NEXT: TemplateArgument
   // CHECK-NEXT: ParmVarDecl
+  // CHECK-NEXT: TemplateArgument
   // CHECK:  FunctionTemplateDecl{{.*}} TestFunctionTemplate
   // CHECK-NEXT:   TemplateTypeParmDecl
   // CHECK-NEXT:   FunctionDecl{{.*}} TestFunctionTemplate 'void (T)'
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -930,22 +930,6 @@
 }
   }
 
-  if (const FunctionTemplateSpecializationInfo *FTSI =
-  D->getTemplateSpecializationInfo())
-dumpTemplateArgumentList(*FTSI->TemplateArguments);
-
-  if (!D->param_begin() && D->getNumParams())
-dumpChild([=] { OS << ">"; });
-  else
-for (const ParmVarDecl *Parameter : D->parameters())
-  dumpDecl(Parameter);
-
-  if (const CXXConstructorDecl *C = dyn_cast(D))
-for (CXXConstructorDecl::init_const_iterator I = C->init_begin(),
- E = C->init_end();
- I != E; ++I)
-  dumpCXXCtorInitializer(*I);
-
   if (const CXXMethodDecl *MD = dyn_cast(D)) {
 if (MD->size_overridden_methods() != 0) {
   auto dumpOverride = [=](const CXXMethodDecl *D) {
@@ -969,6 +953,22 @@
 }
   }
 
+  if (!D->param_begin() && D->getNumParams())
+dumpChild([=] { OS << ">"; });
+  else
+for (const ParmVarDecl *Parameter : D->parameters())
+  dumpDecl(Parameter);
+
+  if (const FunctionTemplateSpecializationInfo *FTSI =
+  D->getTemplateSpecializationInfo())
+dumpTemplateArgumentList(*FTSI->TemplateArguments);
+
+  if (const CXXConstructorDecl *C = dyn_cast(D))
+for (CXXConstructorDecl::init_const_iterator I = C->init_begin(),
+ 

Re: r348471 - Add test for ObjC generics

[Stephen Kelly via cfe-commits]

On 06/12/2018 12:48, Aaron Ballman wrote:

On Thu, Dec 6, 2018 at 4:26 AM Stephen Kelly via cfe-commits
 wrote:

Author: steveire
Date: Thu Dec  6 01:23:59 2018
New Revision: 348471

URL: http://llvm.org/viewvc/llvm-project?rev=348471=rev
Log:
Add test for ObjC generics

Modified:
 cfe/trunk/test/AST/ast-dump-decl.m

Modified: cfe/trunk/test/AST/ast-dump-decl.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-decl.m?rev=348471=348470=348471=diff
==
--- cfe/trunk/test/AST/ast-dump-decl.m (original)
+++ cfe/trunk/test/AST/ast-dump-decl.m Thu Dec  6 01:23:59 2018
@@ -81,6 +81,14 @@
  // CHECK-NEXT:   ObjCProtocol{{.*}} 'P'
  // CHECK-NEXT:   ObjCMethodDecl{{.*}} bar

+@interface TestGenericInterface : A {
+}
+@end
+// CHECK:  ObjCInterfaceDecl{{.*}} TestGenericInterface
+// CHECK-NEXT:   -ObjCTypeParamDecl
+// CHECK-NEXT:   -super ObjCInterface
+// CHECK-NEXT:   -ObjCProtocol

It would be useful to test the line and column, identifier, and type
information as well as the basic AST nodes.


Done in r348543.

Thanks!

Stephen.



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

r348543 - Add more expected content to match in test

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Thu Dec  6 15:23:10 2018
New Revision: 348543

URL: http://llvm.org/viewvc/llvm-project?rev=348543=rev
Log:
Add more expected content to match in test

Modified:
cfe/trunk/test/AST/ast-dump-decl.m

Modified: cfe/trunk/test/AST/ast-dump-decl.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-decl.m?rev=348543=348542=348543=diff
==
--- cfe/trunk/test/AST/ast-dump-decl.m (original)
+++ cfe/trunk/test/AST/ast-dump-decl.m Thu Dec  6 15:23:10 2018
@@ -85,9 +85,9 @@
 }
 @end
 // CHECK:  ObjCInterfaceDecl{{.*}} TestGenericInterface
-// CHECK-NEXT:   -ObjCTypeParamDecl
-// CHECK-NEXT:   -super ObjCInterface
-// CHECK-NEXT:   -ObjCProtocol
+// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}}  col:33 T 'id':'id'
+// CHECK-NEXT:   -super ObjCInterface {{.+}} 'A'
+// CHECK-NEXT:   -ObjCProtocol {{.+}} 'P'
 
 @implementation TestObjCClass (TestObjCCategoryDecl)
 - (void) bar {


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

[PATCH] D55388: [analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, szepet, 
rnkovacs, Szelethus.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, 
a.sidorin, baloghadamsoftware.

Calling `operator*()` or `operator->()` on a null STL smart pointer is 
undefined behavior.

Smart pointers are specified to become null after being moved from. So we can't 
warn on arbitrary method calls, but these two operators definitely make no 
sense.

The new bug is fatal, because it's UB unlike other use-after-move bugs.

Generally, we should also make a checker for dereferencing smart pointers that 
are known to be null, regardless of why they are null. Probably these checkers 
could interact somehow. But this case is kinda handy and is available to us 
almost for free.


Repository:
  rC Clang

https://reviews.llvm.org/D55388

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -1,20 +1,26 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
-// RUN:  -analyzer-config exploration_strategy=unexplored_first_queue
+// RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
+// RUN:  -analyzer-checker debug.ExprInspection
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
-// RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1
+// RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
+// RUN:  -analyzer-checker debug.ExprInspection
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
-// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE\
+// RUN:  -analyzer-checker debug.ExprInspection
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
-// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE\
+// RUN:  -analyzer-checker debug.ExprInspection
 
 #include "Inputs/system-header-simulator-cxx.h"
 
+void clang_analyzer_warnIfReached();
+
 class B {
 public:
   B() = default;
@@ -810,7 +816,19 @@
 // expected-note@-4{{Object 'P' is moved}}
 // expected-note@-4{{Method called on moved-from object 'P'}}
 #endif
-*P += 1; // FIXME: Should warn that the pointer is null.
+
+// Because that well-defined state is null, dereference is still UB.
+// Note that in aggressive mode we already warned about 'P',
+// so no extra warning is generated.
+*P += 1;
+#ifndef AGGRESSIVE
+// expected-warning@-2{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
+// expected-note@-14{{Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from}}
+// expected-note@-4{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
+#endif
+
+// The program should have crashed by now.
+clang_analyzer_warnIfReached(); // no-warning
   }
 };
 
@@ -827,3 +845,9 @@
   P.get(); // expected-warning{{Method called on moved-from object 'P'}}
// expected-note@-1{{Method called on moved-from object 'P'}}
 }
+
+void localUniquePtrWithArrow(std::unique_ptr P) {
+  std::unique_ptr Q = std::move(P); // expected-note{{Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from}}
+  P->foo(); // expected-warning{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
+// expected-note@-1{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
+}
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -61,11 +61,27 @@
   const char *NL, const char *Sep) const override;
 
 private:
-  enum MisuseKind { MK_FunCall, MK_Copy, MK_Move };
+  enum MisuseKind { MK_FunCall, MK_Copy, MK_Move, MK_Dereference };
+  enum StdObjectKind { SK_NonStd, SK_Unsafe, SK_Safe, SK_SmartPtr };
+
+  static bool misuseCausesCrash(MisuseKind MK) {
+return MK == MK_Dereference;
+  }
 
   struct ObjectKind {
-bool Local : 1; // Is this a local variable or a local rvalue reference?
-bool STL : 1; // Is this an object of a 

[PATCH] D55387: [analyzer] MoveChecker Pt.7: NFC: Misc refactoring.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, szepet, 
rnkovacs, Szelethus.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, 
a.sidorin, baloghadamsoftware.
NoQ added a parent revision: D55307: [analyzer] MoveChecker Pt.6: Suppress the 
warning for the few move-safe STL classes..

This is a no-functional-change part of the next patch.

The checker supports 5 different sorts of "use" after move: copying, moving, 
copy-assigning from, move-assigning from, calling any other method. 
De-duplicate the common checks that repeat 5 times into a single method, 
`checkUse()`.

Re-structure the code a little bit.


Repository:
  rC Clang

https://reviews.llvm.org/D55387

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp

Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -88,6 +88,20 @@
   "weak_ptr",
   };
 
+  // Should we bother tracking the state of the object?
+  bool shouldBeTracked(ObjectKind OK) const {
+// In non-aggressive mode, only warn on use-after-move of local variables
+// (or local rvalue references) and of STL objects. The former is possible
+// because local variables (or local rvalue references) are not tempting
+// their user to re-use the storage. The latter is possible because STL
+// objects are known to end up in a valid but unspecified state after the
+// move and their state-reset methods are also known, which allows us to
+// predict precisely when use-after-move is invalid. In aggressive mode,
+// warn on any use-after-move because the user has intentionally asked us
+// to completely eliminate use-after-move in his code.
+return IsAggressive || OK.Local || OK.STL;
+  }
+
   // Obtains ObjectKind of an object. Because class declaration cannot always
   // be easily obtained from the memory region, it is supplied separately.
   ObjectKind classifyObject(const MemRegion *MR, const CXXRecordDecl *RD) const;
@@ -135,8 +149,20 @@
 
 private:
   mutable std::unique_ptr BT;
+
+  // Check if the given form of potential misuse of a given object
+  // should be reported. If so, get it reported. The callback from which
+  // this function was called should immediately return after the call
+  // because this function adds one or two transitions.
+  void checkUse(ProgramStateRef State, const MemRegion *Region,
+const CXXRecordDecl *RD, MisuseKind MK,
+CheckerContext ) const;
+
+  // Returns the exploded node against which the report was emitted.
+  // The caller *must* add any further transitions against this node.
   ExplodedNode *reportBug(const MemRegion *Region, const CXXRecordDecl *RD,
   CheckerContext , MisuseKind MK) const;
+
   bool isInMoveSafeContext(const LocationContext *LC) const;
   bool isStateResetMethod(const CXXMethodDecl *MethodDec) const;
   bool isMoveSafeMethod(const CXXMethodDecl *MethodDec) const;
@@ -235,6 +261,25 @@
   return MoveNode;
 }
 
+void MoveChecker::checkUse(ProgramStateRef State, const MemRegion *Region,
+   const CXXRecordDecl *RD, MisuseKind MK,
+   CheckerContext ) const {
+  assert(!C.isDifferent() && "No transitions should have been made by now");
+  const RegionState *RS = State->get(Region);
+
+  if (!RS || isAnyBaseRegionReported(State, Region)
+  || isInMoveSafeContext(C.getLocationContext())) {
+// Finalize changes made by the caller.
+C.addTransition(State);
+return;
+  }
+
+  ExplodedNode *N = reportBug(Region, RD, C, MK);
+
+  State = State->set(Region, RegionState::getReported());
+  C.addTransition(State, N);
+}
+
 ExplodedNode *MoveChecker::reportBug(const MemRegion *Region,
  const CXXRecordDecl *RD,
  CheckerContext ,
@@ -293,6 +338,8 @@
   if (!MethodDecl)
 return;
 
+  const CXXRecordDecl *RD = MethodDecl->getParent();
+
   const auto *ConstructorDecl = dyn_cast(MethodDecl);
 
   const auto *CC = dyn_cast_or_null();
@@ -309,20 +356,6 @@
   if (!ArgRegion)
 return;
 
-  // In non-aggressive mode, only warn on use-after-move of local variables (or
-  // local rvalue references) and of STL objects. The former is possible because
-  // local variables (or local rvalue references) are not tempting their user to
-  // re-use the storage. The latter is possible because STL objects are known
-  // to end up in a valid but unspecified state after the move and their
-  // state-reset methods are also known, which allows us to predict
-  // precisely when use-after-move is invalid.
-  // In aggressive mode, warn on any use-after-move because the user
-  // has intentionally asked us to completely eliminate use-after-move
-  // in his code.
-  ObjectKind OK = 

Re: r335081 - Recommit r335063: [Darwin] Add a warning for missing include path for libstdc++

[Alex L via cfe-commits]

On Fri, 7 Sep 2018 at 12:02, Alex L  wrote:

>
> On Fri, 7 Sep 2018 at 05:41, Nico Weber  wrote:
>
>> On Wed, Sep 5, 2018 at 9:25 PM Alex L  wrote:
>>
>>> Sorry for the late response,
>>>
>>> On Sat, 18 Aug 2018 at 20:10, Nico Weber  wrote:
>>>
 Also, the diag text should probably say "pass '-stdlib=libc++' " not
 "pass '-std=libc++' "?

>>>
>>> Good catch, thanks! I'll commit a fixup for it.
>>>
>>
> Fixed in r341697.
>
>
>>
>>>

 On Sat, Aug 18, 2018 at 11:06 PM Nico Weber 
 wrote:

> Should this maybe not be emitted when compiling e.g. .ii files
> (preprocessed output)? I just got this when I built a standalone .ii file
> with some bug repro after I deleted all -I and -isysroot flags and 
> whatnot,
> since they aren't needed for preprocessed files.
>

>>> That would make sense, but I'm not sure how easy it is to determine if a
>>> file is already preprocessed. I'll try to create a patch for this fixup.
>>>
>>
>> Probably you'd want something like
>> http://llvm-cs.pcc.me.uk/tools/clang/lib/Driver/Types.cpp#123 that
>> returns true for all (or most) of the TY_PP_ types, and then just rely
>> on lookupTypeForExtension.
>>
>
> Good idea, I'll try it out.
>

Apologies for the delay, I fixed the issue with preprocessed inputs in
r348540.

Cheers,
Alex


>
>
>>
>>
>>>
>>>
>>>

> On Tue, Jun 19, 2018 at 6:52 PM Alex Lorenz via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: arphaman
>> Date: Tue Jun 19 15:47:53 2018
>> New Revision: 335081
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=335081=rev
>> Log:
>> Recommit r335063: [Darwin] Add a warning for missing include path for
>> libstdc++
>>
>> The recommit ensures that the tests that failed on bots don't trigger
>> the warning.
>>
>> Xcode 10 removes support for libstdc++, but the users just get a
>> confusing
>> include not file warning when including an STL header (when building
>> for iOS6
>> which uses libstdc++ by default for example).
>> This patch adds a new warning that lets the user know that the
>> libstdc++ include
>> path was not found to ensure that the user is more aware of why the
>> error occurs.
>>
>> rdar://40830462
>>
>> Differential Revision: https://reviews.llvm.org/D48297
>>
>> Added:
>> cfe/trunk/test/Frontend/warning-stdlibcxx-darwin.cpp
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
>> cfe/trunk/include/clang/Lex/HeaderSearch.h
>> cfe/trunk/lib/Frontend/InitHeaderSearch.cpp
>> cfe/trunk/test/CodeGenCXX/apple-kext-guard-variable.cpp
>> cfe/trunk/test/Misc/backend-stack-frame-diagnostics.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td?rev=335081=335080=335081=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
>> (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td Tue Jun
>> 19 15:47:53 2018
>> @@ -236,4 +236,9 @@ def err_invalid_vfs_overlay : Error<
>>
>>  def warn_option_invalid_ocl_version : Warning<
>>"OpenCL version %0 does not support the option '%1'">,
>> InGroup;
>> +
>> +def warn_stdlibcxx_not_found : Warning<
>> +  "include path for stdlibc++ headers not found; pass '-std=libc++'
>> on the "
>> +  "command line to use the libc++ standard library instead">,
>> +  InGroup>;
>>  }
>>
>> Modified: cfe/trunk/include/clang/Lex/HeaderSearch.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/HeaderSearch.h?rev=335081=335080=335081=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Lex/HeaderSearch.h (original)
>> +++ cfe/trunk/include/clang/Lex/HeaderSearch.h Tue Jun 19 15:47:53
>> 2018
>> @@ -272,6 +272,8 @@ public:
>>
>>FileManager () const { return FileMgr; }
>>
>> +  DiagnosticsEngine () const { return Diags; }
>> +
>>/// Interface for setting the file search paths.
>>void SetSearchPaths(const std::vector ,
>>unsigned angledDirIdx, unsigned systemDirIdx,
>>
>> Modified: cfe/trunk/lib/Frontend/InitHeaderSearch.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/InitHeaderSearch.cpp?rev=335081=335080=335081=diff
>>
>> ==
>> --- cfe/trunk/lib/Frontend/InitHeaderSearch.cpp (original)
>> +++ cfe/trunk/lib/Frontend/InitHeaderSearch.cpp Tue Jun 19 15:47:53
>> 2018
>> @@ -14,6 +14,7 @@

Re: r348469 - Make test resistant to line numbers changing

[Stephen Kelly via cfe-commits]

On 06/12/2018 12:45, Aaron Ballman wrote:

On Thu, Dec 6, 2018 at 4:25 AM Stephen Kelly via cfe-commits
 wrote:

Author: steveire
Date: Thu Dec  6 01:22:12 2018
New Revision: 348469

URL: http://llvm.org/viewvc/llvm-project?rev=348469=rev
Log:
Make test resistant to line numbers changing

I would prefer to see the line numbers be tested rather than entirely
ignored (it's still important information we want to make sure is
correct). Can you use [[@LINE + N]] to instead make the line numbers
relative?



Done with r348541.

Thanks!

Stephen.


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

r348541 - Use relative line offsets in test

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Thu Dec  6 14:51:51 2018
New Revision: 348541

URL: http://llvm.org/viewvc/llvm-project?rev=348541=rev
Log:
Use relative line offsets in test

Modified:
cfe/trunk/test/AST/dump.cpp

Modified: cfe/trunk/test/AST/dump.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/dump.cpp?rev=348541=348540=348541=diff
==
--- cfe/trunk/test/AST/dump.cpp (original)
+++ cfe/trunk/test/AST/dump.cpp Thu Dec  6 14:51:51 2018
@@ -13,7 +13,7 @@ int ga, gb;
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) 
initializer(omp_priv = omp_orig + 15)
 
-// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 operator+ 
'int' combiner
+// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int' combiner
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'int' lvalue 
'*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_out' 'int'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
@@ -28,7 +28,7 @@ int ga, gb;
 // CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_in' 'char'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 fun 
'float' combiner initializer
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner initializer
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
@@ -60,19 +60,19 @@ struct S {
 // CHECK-NEXT: |   |-OMPScheduleClause {{.+}} 
 // CHECK-NEXT: |   | `-ImplicitCastExpr {{.+}}  'int' 

 // CHECK-NEXT: |   |   `-DeclRefExpr {{.+}}  'int' lvalue 
OMPCapturedExpr {{.+}} '.capture_expr.' 'int'
-// CHECK-NEXT: |   `-CapturedStmt {{.+}} 
+// CHECK-NEXT: |   `-CapturedStmt {{.+}} 
 // CHECK-NEXT: | |-CapturedDecl {{.+}} <> 
-// CHECK-NEXT: | | |-ForStmt {{.+}} 
-// CHECK:  | | | `-UnaryOperator {{.+}}  
'int' lvalue prefix '++'
+// CHECK-NEXT: | | |-ForStmt {{.+}} 
+// CHECK:  | | | `-UnaryOperator {{.+}}  'int' lvalue prefix '++'
 // CHECK-NEXT: | | |   `-DeclRefExpr {{.+}}  'int' lvalue 
OMPCapturedExpr {{.+}} 'a' 'int &'
 
 #pragma omp declare simd
 #pragma omp declare simd inbranch
 void foo();
 
-// CHECK:|-FunctionDecl {{.+}}  col:6 foo 'void 
()'
-// CHECK-NEXT:   |-OMPDeclareSimdDeclAttr {{.+}}  
Implicit BS_Inbranch
-// CHECK:`-OMPDeclareSimdDeclAttr {{.+}}  
Implicit BS_Undefined
+// CHECK:|-FunctionDecl {{.+}}  col:6 foo 
'void ()'
+// CHECK-NEXT:   |-OMPDeclareSimdDeclAttr {{.+}}  
Implicit BS_Inbranch
+// CHECK:`-OMPDeclareSimdDeclAttr {{.+}}  
Implicit BS_Undefined
 
 #pragma omp declare target
 int bar() {
@@ -81,11 +81,11 @@ int bar() {
 }
 #pragma omp end declare target
 
-// CHECK:   `-FunctionDecl {{.+}}  
line:{{.+}}:5 bar 'int ()'
-// CHECK-NEXT:  |-CompoundStmt {{.+}} 
-// CHECK-NEXT:  | |-DeclStmt {{.+}} 
+// CHECK:   `-FunctionDecl {{.+}}  
line:[[@LINE-6]]:5 bar 'int ()'
+// CHECK-NEXT:  |-CompoundStmt {{.+}} 
+// CHECK-NEXT:  | |-DeclStmt {{.+}} 
 // CHECK-NEXT:  | | `-VarDecl {{.+}}  col:7 used f 'int'
-// CHECK-NEXT:  | `-ReturnStmt {{.+}} 
+// CHECK-NEXT:  | `-ReturnStmt {{.+}} 
 // CHECK-NEXT:  |   `-ImplicitCastExpr {{.+}}  'int' 
 // CHECK-NEXT:  | `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'f' 'int'
 // CHECK-NEXT:  `-OMPDeclareTargetDeclAttr {{.+}} <> Implicit 
MT_To


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

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177057.
Szelethus added a comment.

Woops, forgot to fix the doc of deallocation functions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54823/new/

https://reviews.llvm.org/D54823

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/malloc-annotations.c
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
 
Index: test/Analysis/malloc-annotations.c
===
--- test/Analysis/malloc-annotations.c
+++ test/Analysis/malloc-annotations.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc -analyzer-store=region -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize,unix.Malloc \
+// RUN:   -analyzer-config unix.Malloc:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -7,8 +7,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,28 +87,59 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily getAllocationFamily(
+const MemFunctionInfoTy , CheckerContext , const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream , CheckerContext ,
+  const Expr 

r348540 - [frontend][darwin] warn_stdlibcxx_not_found: supress warning for preprocessed input

[Alex Lorenz via cfe-commits]

Author: arphaman
Date: Thu Dec  6 14:45:58 2018
New Revision: 348540

URL: http://llvm.org/viewvc/llvm-project?rev=348540=rev
Log:
[frontend][darwin] warn_stdlibcxx_not_found: supress warning for preprocessed 
input

Addresses second post-commit feedback for r335081 from Nico

Modified:
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/Frontend/warning-stdlibcxx-darwin.cpp

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=348540=348539=348540=diff
==
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Thu Dec  6 14:45:58 2018
@@ -3240,6 +3240,7 @@ bool CompilerInvocation::CreateFromArgs(
   Res.getTargetOpts(), Res.getFrontendOpts());
   ParseHeaderSearchArgs(Res.getHeaderSearchOpts(), Args,
 Res.getFileSystemOpts().WorkingDir);
+  llvm::Triple T(Res.getTargetOpts().Triple);
   if (DashX.getFormat() == InputKind::Precompiled ||
   DashX.getLanguage() == InputKind::LLVM_IR) {
 // ObjCAAutoRefCount and Sanitize LangOpts are used to setup the
@@ -3260,6 +3261,12 @@ bool CompilerInvocation::CreateFromArgs(
   Res.getPreprocessorOpts(), Diags);
 if (Res.getFrontendOpts().ProgramAction == frontend::RewriteObjC)
   LangOpts.ObjCExceptions = 1;
+if (T.isOSDarwin() && DashX.isPreprocessed()) {
+  // Supress the darwin-specific 'stdlibcxx-not-found' diagnostic for
+  // preprocessed input as we don't expect it to be used with -std=libc++
+  // anyway.
+  Res.getDiagnosticOpts().Warnings.push_back("no-stdlibcxx-not-found");
+}
   }
 
   LangOpts.FunctionAlignment =
@@ -3291,7 +3298,6 @@ bool CompilerInvocation::CreateFromArgs(
   Res.getFrontendOpts().ProgramAction);
 
   // Turn on -Wspir-compat for SPIR target.
-  llvm::Triple T(Res.getTargetOpts().Triple);
   auto Arch = T.getArch();
   if (Arch == llvm::Triple::spir || Arch == llvm::Triple::spir64) {
 Res.getDiagnosticOpts().Warnings.push_back("spir-compat");

Modified: cfe/trunk/test/Frontend/warning-stdlibcxx-darwin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Frontend/warning-stdlibcxx-darwin.cpp?rev=348540=348539=348540=diff
==
--- cfe/trunk/test/Frontend/warning-stdlibcxx-darwin.cpp (original)
+++ cfe/trunk/test/Frontend/warning-stdlibcxx-darwin.cpp Thu Dec  6 14:45:58 
2018
@@ -1,5 +1,6 @@
 // RUN: %clang -cc1 -triple arm64-apple-ios6.0.0 -isysroot %S/doesnotexist %s 
2>&1 | FileCheck %s
 // RUN: %clang -cc1 -triple arm64-apple-ios6.0.0 -isysroot %S/doesnotexist 
-stdlib=libc++ %s -verify
+// RUN: %clang -cc1 -x c++-cpp-output -triple arm64-apple-ios6.0.0 -isysroot 
%S/doesnotexist %s -verify
 // CHECK: include path for stdlibc++ headers not found; pass '-stdlib=libc++' 
on the command line to use the libc++ standard library instead
 
 // expected-no-diagnostics


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

[PATCH] D55289: [analyzer] MoveChecker Pt.5: Improve invalidation policies.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LG!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55289/new/

https://reviews.llvm.org/D55289



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

[PATCH] D55307: [analyzer] MoveChecker Pt.6: Suppress the warning for the few move-safe STL classes.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 177048.
NoQ marked an inline comment as done.
NoQ added a comment.

Add one more important case: `std::optional`. When moved, it moves the object 
within it into the new optional, if any. So the object is left in unspecified 
state, but the state of the optional itself is well-specified.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/Inputs/system-header-simulator-cxx.h
  test/Analysis/diagnostics/explicit-suppression.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -13,47 +13,7 @@
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
 // RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
-namespace std {
-
-typedef __typeof(sizeof(int)) size_t;
-
-template 
-struct remove_reference;
-
-template 
-struct remove_reference { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &> { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &&> { typedef _Tp type; };
-
-template 
-typename remove_reference<_Tp>::type &(_Tp &&__t) {
-  return static_cast::type &&>(__t);
-}
-
-template 
-_Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
-  return static_cast<_Tp &&>(__t);
-}
-
-template 
-void swap(T , T ) {
-  T c(std::move(a));
-  a = std::move(b);
-  b = std::move(c);
-}
-
-template 
-class vector {
-public:
-  vector();
-  void push_back(const T );
-};
-
-} // namespace std
+#include "Inputs/system-header-simulator-cxx.h"
 
 class B {
 public:
@@ -832,13 +792,26 @@
 
 class HasSTLField {
   std::vector V;
-  void foo() {
+  void testVector() {
 // Warn even in non-aggressive mode when it comes to STL, because
 // in STL the object is left in "valid but unspecified state" after move.
 std::vector W = std::move(V); // expected-note{{Object 'V' of type 'std::vector' is left in a valid but unspecified state after move}}
 V.push_back(123); // expected-warning{{Method called on moved-from object 'V'}}
   // expected-note@-1{{Method called on moved-from object 'V'}}
   }
+
+  std::unique_ptr P;
+  void testUniquePtr() {
+// unique_ptr remains in a well-defined state after move.
+std::unique_ptr Q = std::move(P);
+P.get();
+#ifdef AGGRESSIVE
+// expected-warning@-2{{Method called on moved-from object 'P'}}
+// expected-note@-4{{Object 'P' is moved}}
+// expected-note@-4{{Method called on moved-from object 'P'}}
+#endif
+*P += 1; // FIXME: Should warn that the pointer is null.
+  }
 };
 
 void localRValueMove(A &) {
@@ -846,3 +819,11 @@
   a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
// expected-note@-1{{Method called on moved-from object 'a'}}
 }
+
+void localUniquePtr(std::unique_ptr P) {
+  // Even though unique_ptr is safe to use after move,
+  // reusing a local variable this way usually indicates a bug.
+  std::unique_ptr Q = std::move(P); // expected-note{{Object 'P' is moved}}
+  P.get(); // expected-warning{{Method called on moved-from object 'P'}}
+   // expected-note@-1{{Method called on moved-from object 'P'}}
+}
Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -19,6 +19,6 @@
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ object pointer is null}}
 #endif
 }
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -237,6 +237,13 @@
 return static_cast(a);
   }
 
+  template 
+  void swap(T , T ) {
+T c(std::move(a));
+a = std::move(b);
+b = std::move(c);
+  }
+
   template
   class vector {
 typedef T value_type;
@@ -770,6 +777,22 @@
 
 }
 
+#if __cplusplus >= 201103L
+namespace std {
+  template // TODO: Implement the stub for deleter.
+  class unique_ptr {
+  public:
+unique_ptr(const unique_ptr &) = delete;
+unique_ptr(unique_ptr &&);
+
+T *get() const;
+
+typename std::add_lvalue_reference::type operator*() const;
+T *operator->() const;
+  };
+}
+#endif
+
 #ifdef TEST_INLINABLE_ALLOCATORS
 namespace std {
   void *malloc(size_t);
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- 

[PATCH] D55307: [analyzer] MoveChecker Pt.6: Suppress the warning for the few move-safe STL classes.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: test/Analysis/diagnostics/explicit-suppression.cpp:22
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ 
object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ 
object pointer is null}}
 #endif

Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > Can't we just change this to `// expected-warning{{Called C++ object 
> > > > pointer is null}}`? This file is so tiny, I think it wouldn't cause 
> > > > much confusion, and  reduces unnecessary maintenance work.
> > > I don't think it'll work. The warning is not on this line, it is in 
> > > `system-header-simulator-cxx.h`, so we need to specify it somehow, and 
> > > it'll appear only in this test, not in other tests that include that 
> > > header, so we can't put it directly into the header.
> > Ah, okay.
> But since this is the only warning in the file, we could get away with it 
> of we use `FileCheck`! But I leave it up to you.
> 
> I had a lot of bots breaking on me because I forgot `git add` on this file 
> once, so I might end up fixing it myself.
Mmm, maybe. This test is tiny enough.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307



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

[PATCH] D55289: [analyzer] MoveChecker Pt.5: Improve invalidation policies.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 177047.
NoQ added a comment.

Fxd :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55289/new/

https://reviews.llvm.org/D55289

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -116,6 +116,19 @@
   bool empty() const;
   bool isEmpty() const;
   operator bool() const;
+
+  void testUpdateField() {
+A a;
+A b = std::move(a);
+a.i = 1;
+a.foo(); // no-warning
+  }
+  void testUpdateFieldDouble() {
+A a;
+A b = std::move(a);
+a.d = 1.0;
+a.foo(); // no-warning
+  }
 };
 
 int bignum();
@@ -594,24 +607,50 @@
   foobar(a.getI(), std::move(a)); //no-warning
 }
 
-void not_known(A );
-void not_known(A *a);
+void not_known_pass_by_ref(A );
+void not_known_pass_by_const_ref(const A );
+void not_known_pass_by_rvalue_ref(A &);
+void not_known_pass_by_ptr(A *a);
+void not_known_pass_by_const_ptr(const A *a);
 
 void regionAndPointerEscapeTest() {
   {
 A a;
 A b;
 b = std::move(a);
-not_known(a);
-a.foo(); //no-warning
+not_known_pass_by_ref(a);
+a.foo(); // no-warning
+  }
+  {
+A a;
+A b;
+b = std::move(a); // expected-note{{Object 'a' is moved}}
+not_known_pass_by_const_ref(a);
+a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
+ // expected-note@-1{{Method called on moved-from object 'a'}}
+  }
+  {
+A a;
+A b;
+b = std::move(a);
+not_known_pass_by_rvalue_ref(std::move(a));
+a.foo(); // no-warning
   }
   {
 A a;
 A b;
 b = std::move(a);
-not_known();
+not_known_pass_by_ptr();
 a.foo(); // no-warning
   }
+  {
+A a;
+A b;
+b = std::move(a); // expected-note{{Object 'a' is moved}}
+not_known_pass_by_const_ptr();
+a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
+ // expected-note@-1{{Method called on moved-from object 'a'}}
+  }
 }
 
 // A declaration statement containing multiple declarations sequences the
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -52,8 +52,8 @@
   ProgramStateRef
   checkRegionChanges(ProgramStateRef State,
  const InvalidatedSymbols *Invalidated,
- ArrayRef ExplicitRegions,
- ArrayRef Regions,
+ ArrayRef RequestedRegions,
+ ArrayRef InvalidatedRegions,
  const LocationContext *LCtx, const CallEvent *Call) const;
   void printState(raw_ostream , ProgramStateRef State,
   const char *NL, const char *Sep) const override;
@@ -524,19 +524,35 @@
 
 ProgramStateRef MoveChecker::checkRegionChanges(
 ProgramStateRef State, const InvalidatedSymbols *Invalidated,
-ArrayRef ExplicitRegions,
-ArrayRef Regions, const LocationContext *LCtx,
-const CallEvent *Call) const {
-  // In case of an InstanceCall don't remove the ThisRegion from the GDM since
-  // it is handled in checkPreCall and checkPostCall.
-  const MemRegion *ThisRegion = nullptr;
-  if (const auto *IC = dyn_cast_or_null(Call)) {
-ThisRegion = IC->getCXXThisVal().getAsRegion();
-  }
-
-  for (const auto *Region : ExplicitRegions) {
-if (ThisRegion != Region)
-  State = removeFromState(State, Region);
+ArrayRef RequestedRegions,
+ArrayRef InvalidatedRegions,
+const LocationContext *LCtx, const CallEvent *Call) const {
+  if (Call) {
+// Relax invalidation upon function calls: only invalidate parameters
+// that are passed directly via non-const pointers or non-const references
+// or rvalue references.
+// In case of an InstanceCall don't invalidate the this-region since
+// it is fully handled in checkPreCall and checkPostCall.
+const MemRegion *ThisRegion = nullptr;
+if (const auto *IC = dyn_cast(Call))
+  ThisRegion = IC->getCXXThisVal().getAsRegion();
+
+// Requested ("explicit") regions are the regions passed into the call
+// directly, but not all of them end up being invalidated.
+// But when they do, they appear in the InvalidatedRegions array as well.
+for (const auto *Region : RequestedRegions) {
+  if (ThisRegion != Region) {
+if (llvm::find(InvalidatedRegions, Region) !=
+std::end(InvalidatedRegions)) {
+  State = removeFromState(State, Region);
+}
+  }
+}
+  } else {
+// For invalidations that aren't caused by calls, assume nothing. In
+// particular, direct write into an object's field invalidates the status.
+for (const auto *Region : InvalidatedRegions)
+  State = removeFromState(State, 

[PATCH] D55158: [analyzer] Rely on os_consumes_this attribute to signify that the method call consumes a reference for "this"

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348533: [analyzer] Rely on os_consumes_this attribute to 
signify that the method call… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55158?vs=176234=177044#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55158/new/

https://reviews.llvm.org/D55158

Files:
  include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
  test/Analysis/osobject-retain-release.cpp


Index: include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
===
--- include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
+++ include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
@@ -369,8 +369,12 @@
   ///  This is only meaningful if the summary applies to an ObjCMessageExpr*.
   ArgEffect getReceiverEffect() const { return Receiver; }
 
+  /// \return the effect on the "this" receiver of the method call.
   ArgEffect getThisEffect() const { return This; }
 
+  /// Set the effect of the method on "this".
+  void setThisEffect(ArgEffect e) { This = e; }
+
   bool isNoop() const {
 return Ret == RetEffect::MakeNoRet() && Receiver == DoNothing
   && DefaultArgEffect == MayEscape && This == DoNothing
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -5,6 +5,7 @@
 #define OS_CONSUME __attribute__((os_consumed))
 #define OS_RETURNS_RETAINED __attribute__((os_returns_retained))
 #define OS_RETURNS_NOT_RETAINED __attribute__((os_returns_not_retained))
+#define OS_CONSUMES_THIS __attribute__((os_consumes_this))
 
 #define OSTypeID(type)   (type::metaClass)
 
@@ -49,6 +50,11 @@
 
   virtual void consumeReference(OS_CONSUME OSArray *other);
 
+  void putIntoArray(OSArray *array) OS_CONSUMES_THIS;
+
+  template 
+  void putIntoT(T *owner) OS_CONSUMES_THIS;
+
   static OSArray *generateArrayHasCode() {
 return new OSArray;
   }
@@ -112,6 +118,16 @@
   return 0;
 }
 
+void check_consumes_this(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoArray(owner);
+}
+
+void check_consumes_this_with_template(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoT(owner);
+}
+
 void check_free_no_error() {
   OSArray *arr = OSArray::withCapacity(10);
   arr->retain();
Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -759,6 +759,9 @@
   QualType RetTy = FD->getReturnType();
   if (Optional RetE = getRetEffectFromAnnotations(RetTy, FD))
 Template->setRetEffect(*RetE);
+
+  if (FD->hasAttr())
+Template->setThisEffect(DecRef);
 }
 
 void


Index: include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
===
--- include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
+++ include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
@@ -369,8 +369,12 @@
   ///  This is only meaningful if the summary applies to an ObjCMessageExpr*.
   ArgEffect getReceiverEffect() const { return Receiver; }
 
+  /// \return the effect on the "this" receiver of the method call.
   ArgEffect getThisEffect() const { return This; }
 
+  /// Set the effect of the method on "this".
+  void setThisEffect(ArgEffect e) { This = e; }
+
   bool isNoop() const {
 return Ret == RetEffect::MakeNoRet() && Receiver == DoNothing
   && DefaultArgEffect == MayEscape && This == DoNothing
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -5,6 +5,7 @@
 #define OS_CONSUME __attribute__((os_consumed))
 #define OS_RETURNS_RETAINED __attribute__((os_returns_retained))
 #define OS_RETURNS_NOT_RETAINED __attribute__((os_returns_not_retained))
+#define OS_CONSUMES_THIS __attribute__((os_consumes_this))
 
 #define OSTypeID(type)   (type::metaClass)
 
@@ -49,6 +50,11 @@
 
   virtual void consumeReference(OS_CONSUME OSArray *other);
 
+  void putIntoArray(OSArray *array) OS_CONSUMES_THIS;
+
+  template 
+  void putIntoT(T *owner) OS_CONSUMES_THIS;
+
   static OSArray *generateArrayHasCode() {
 return new OSArray;
   }
@@ -112,6 +118,16 @@
   return 0;
 }
 
+void check_consumes_this(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoArray(owner);
+}
+
+void check_consumes_this_with_template(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoT(owner);
+}
+
 void check_free_no_error() {
   OSArray *arr = OSArray::withCapacity(10);
   arr->retain();

r348533 - [analyzer] Rely on os_consumes_this attribute to signify that the method call consumes a reference for "this"

[George Karpenkov via cfe-commits]

Author: george.karpenkov
Date: Thu Dec  6 14:07:12 2018
New Revision: 348533

URL: http://llvm.org/viewvc/llvm-project?rev=348533=rev
Log:
[analyzer] Rely on os_consumes_this attribute to signify that the method call 
consumes a reference for "this"

Differential Revision: https://reviews.llvm.org/D55158

Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
cfe/trunk/test/Analysis/osobject-retain-release.cpp

Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h?rev=348533=348532=348533=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h 
(original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h Thu Dec  
6 14:07:12 2018
@@ -369,8 +369,12 @@ public:
   ///  This is only meaningful if the summary applies to an ObjCMessageExpr*.
   ArgEffect getReceiverEffect() const { return Receiver; }
 
+  /// \return the effect on the "this" receiver of the method call.
   ArgEffect getThisEffect() const { return This; }
 
+  /// Set the effect of the method on "this".
+  void setThisEffect(ArgEffect e) { This = e; }
+
   bool isNoop() const {
 return Ret == RetEffect::MakeNoRet() && Receiver == DoNothing
   && DefaultArgEffect == MayEscape && This == DoNothing

Modified: cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp?rev=348533=348532=348533=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp Thu Dec  6 
14:07:12 2018
@@ -759,6 +759,9 @@ RetainSummaryManager::updateSummaryFromA
   QualType RetTy = FD->getReturnType();
   if (Optional RetE = getRetEffectFromAnnotations(RetTy, FD))
 Template->setRetEffect(*RetE);
+
+  if (FD->hasAttr())
+Template->setThisEffect(DecRef);
 }
 
 void

Modified: cfe/trunk/test/Analysis/osobject-retain-release.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/osobject-retain-release.cpp?rev=348533=348532=348533=diff
==
--- cfe/trunk/test/Analysis/osobject-retain-release.cpp (original)
+++ cfe/trunk/test/Analysis/osobject-retain-release.cpp Thu Dec  6 14:07:12 2018
@@ -5,6 +5,7 @@ struct OSMetaClass;
 #define OS_CONSUME __attribute__((os_consumed))
 #define OS_RETURNS_RETAINED __attribute__((os_returns_retained))
 #define OS_RETURNS_NOT_RETAINED __attribute__((os_returns_not_retained))
+#define OS_CONSUMES_THIS __attribute__((os_consumes_this))
 
 #define OSTypeID(type)   (type::metaClass)
 
@@ -49,6 +50,11 @@ struct OSArray : public OSObject {
 
   virtual void consumeReference(OS_CONSUME OSArray *other);
 
+  void putIntoArray(OSArray *array) OS_CONSUMES_THIS;
+
+  template 
+  void putIntoT(T *owner) OS_CONSUMES_THIS;
+
   static OSArray *generateArrayHasCode() {
 return new OSArray;
   }
@@ -112,6 +118,16 @@ unsigned int check_attribute_indirect_pr
   return 0;
 }
 
+void check_consumes_this(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoArray(owner);
+}
+
+void check_consumes_this_with_template(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoT(owner);
+}
+
 void check_free_no_error() {
   OSArray *arr = OSArray::withCapacity(10);
   arr->retain();


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

[PATCH] D55155: [attributes] Add an attribute "os_consumes_this" with similar semantics to "ns_consumes_self"

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348532: [attributes] Add an attribute os_consumes_this, with 
similar semantics to… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55155?vs=177041=177043#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55155/new/

https://reviews.llvm.org/D55155

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-osobject.cpp

Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -862,6 +862,9 @@
 Similiarly, the annotation ``__attribute__((ns_returns_not_retained))``
 specifies that the object is returned at ``+0`` and the ownership remains with
 the callee.
+The annotation ``__attribute__((ns_consumes_self))`` specifies that
+the Objective-C method call consumes the reference to ``self``, e.g. by
+attaching it to a supplied parameter.
 Additionally, parameters can have an annotation
 ``__attribute__((ns_consumed))``, which specifies that passing an owned object
 as that parameter effectively transfers the ownership, and the caller is no
@@ -881,7 +884,11 @@
 ``__attribute__((os_returns_not_retained))``,
 ``__attribute__((os_returns_retained))`` and ``__attribute__((os_consumed))``,
 with the same respective semantics.
-These attributes are also used by the Clang Static Analyzer.
+Similar to ``__attribute__((ns_consumes_self))``,
+``__attribute__((os_consumes_this))`` specifies that the method call consumes
+the reference to "this" (e.g., when attaching it to a different object supplied
+as a parameter).
+These attributes are only used by the Clang Static Analyzer.
 
 The family of attributes ``X_returns_X_retained`` can be added to functions,
 C++ methods, and Objective-C methods and properties.
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -90,6 +90,10 @@
 [{!S->isBitField()}],
 "non-bit-field non-static data members">;
 
+def NonStaticCXXMethod : SubsetSubjectisStatic()}],
+   "non-static member functions">;
+
 def NonStaticNonConstCXXMethod
 : SubsetSubjectisStatic() && !S->isConst()}],
@@ -846,6 +850,12 @@
   let Documentation = [RetainBehaviorDocs];
 }
 
+def OSConsumesThis : InheritableAttr {
+  let Spellings = [Clang<"os_consumes_this">];
+  let Subjects = SubjectList<[NonStaticCXXMethod]>;
+  let Documentation = [RetainBehaviorDocs];
+}
+
 def Cleanup : InheritableAttr {
   let Spellings = [GCC<"cleanup">];
   let Args = [FunctionArgument<"FunctionDecl">];
@@ -1649,13 +1659,13 @@
 def NSConsumesSelf : InheritableAttr {
   let Spellings = [Clang<"ns_consumes_self">];
   let Subjects = SubjectList<[ObjCMethod]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def NSConsumed : InheritableParamAttr {
   let Spellings = [Clang<"ns_consumed">];
   let Subjects = SubjectList<[ParmVar]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def ObjCException : InheritableAttr {
Index: test/Sema/attr-osobject.cpp
===
--- test/Sema/attr-osobject.cpp
+++ test/Sema/attr-osobject.cpp
@@ -4,6 +4,10 @@
   __attribute__((os_returns_retained)) S* method_returns_retained() {
 return nullptr;
   }
+
+  __attribute__((os_consumes_this)) void method_consumes_this();
+
+  __attribute__((os_consumes_this)) static void rejected_on_static(); // expected-warning{{'os_consumes_this' attribute only applies to non-static member functions}}
 };
 __attribute__((os_returns_retained)) S *ret_retained() {
   return nullptr;
@@ -37,6 +41,8 @@
 
 __attribute__((os_returns_not_retained(10))) S* os_returns_no_retained_no_extra_args( S *arg) { // expected-error{{'os_returns_not_retained' attribute takes no arguments}}
   return nullptr;
-} 
+}
 
 struct __attribute__((os_returns_not_retained)) NoNotRetainedAttrOnStruct {}; // expected-warning{{'os_returns_not_retained' attribute only applies to functions, Objective-C methods, and Objective-C properties}}
+
+__attribute__((os_consumes_this)) void no_consumes_this_on_function() {} // expected-warning{{'os_consumes_this' attribute only applies to non-static member functions}}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6408,6 +6408,9 @@
   case ParsedAttr::AT_NSConsumesSelf:
 handleSimpleAttribute(S, D, AL);
 break;
+  case ParsedAttr::AT_OSConsumesThis:
+handleSimpleAttribute(S, D, AL);
+break;

r348532 - [attributes] Add an attribute os_consumes_this, with similar semantics to ns_consumes_self

[George Karpenkov via cfe-commits]

Author: george.karpenkov
Date: Thu Dec  6 14:06:59 2018
New Revision: 348532

URL: http://llvm.org/viewvc/llvm-project?rev=348532=rev
Log:
[attributes] Add an attribute os_consumes_this, with similar semantics to 
ns_consumes_self

The attribute specifies that the call of the C++ method consumes a
reference to "this".

Differential Revision: https://reviews.llvm.org/D55155

Modified:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/test/Sema/attr-osobject.cpp

Modified: cfe/trunk/include/clang/Basic/Attr.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=348532=348531=348532=diff
==
--- cfe/trunk/include/clang/Basic/Attr.td (original)
+++ cfe/trunk/include/clang/Basic/Attr.td Thu Dec  6 14:06:59 2018
@@ -90,6 +90,10 @@ def NonBitField : SubsetSubjectisBitField()}],
 "non-bit-field non-static data members">;
 
+def NonStaticCXXMethod : SubsetSubjectisStatic()}],
+   "non-static member functions">;
+
 def NonStaticNonConstCXXMethod
 : SubsetSubjectisStatic() && !S->isConst()}],
@@ -846,6 +850,12 @@ def OSReturnsNotRetained : InheritableAt
   let Documentation = [RetainBehaviorDocs];
 }
 
+def OSConsumesThis : InheritableAttr {
+  let Spellings = [Clang<"os_consumes_this">];
+  let Subjects = SubjectList<[NonStaticCXXMethod]>;
+  let Documentation = [RetainBehaviorDocs];
+}
+
 def Cleanup : InheritableAttr {
   let Spellings = [GCC<"cleanup">];
   let Args = [FunctionArgument<"FunctionDecl">];
@@ -1649,13 +1659,13 @@ def NSReturnsAutoreleased : InheritableA
 def NSConsumesSelf : InheritableAttr {
   let Spellings = [Clang<"ns_consumes_self">];
   let Subjects = SubjectList<[ObjCMethod]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def NSConsumed : InheritableParamAttr {
   let Spellings = [Clang<"ns_consumed">];
   let Subjects = SubjectList<[ParmVar]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def ObjCException : InheritableAttr {

Modified: cfe/trunk/include/clang/Basic/AttrDocs.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=348532=348531=348532=diff
==
--- cfe/trunk/include/clang/Basic/AttrDocs.td (original)
+++ cfe/trunk/include/clang/Basic/AttrDocs.td Thu Dec  6 14:06:59 2018
@@ -862,6 +862,9 @@ is responsible for freeing it.
 Similiarly, the annotation ``__attribute__((ns_returns_not_retained))``
 specifies that the object is returned at ``+0`` and the ownership remains with
 the callee.
+The annotation ``__attribute__((ns_consumes_self))`` specifies that
+the Objective-C method call consumes the reference to ``self``, e.g. by
+attaching it to a supplied parameter.
 Additionally, parameters can have an annotation
 ``__attribute__((ns_consumed))``, which specifies that passing an owned object
 as that parameter effectively transfers the ownership, and the caller is no
@@ -881,7 +884,11 @@ the same attribute family is present:
 ``__attribute__((os_returns_not_retained))``,
 ``__attribute__((os_returns_retained))`` and ``__attribute__((os_consumed))``,
 with the same respective semantics.
-These attributes are also used by the Clang Static Analyzer.
+Similar to ``__attribute__((ns_consumes_self))``,
+``__attribute__((os_consumes_this))`` specifies that the method call consumes
+the reference to "this" (e.g., when attaching it to a different object supplied
+as a parameter).
+These attributes are only used by the Clang Static Analyzer.
 
 The family of attributes ``X_returns_X_retained`` can be added to functions,
 C++ methods, and Objective-C methods and properties.

Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=348532=348531=348532=diff
==
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Thu Dec  6 14:06:59 2018
@@ -6408,6 +6408,9 @@ static void ProcessDeclAttribute(Sema 
   case ParsedAttr::AT_NSConsumesSelf:
 handleSimpleAttribute(S, D, AL);
 break;
+  case ParsedAttr::AT_OSConsumesThis:
+handleSimpleAttribute(S, D, AL);
+break;
   case ParsedAttr::AT_NSReturnsAutoreleased:
   case ParsedAttr::AT_NSReturnsNotRetained:
   case ParsedAttr::AT_NSReturnsRetained:

Modified: cfe/trunk/test/Sema/attr-osobject.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/attr-osobject.cpp?rev=348532=348531=348532=diff
==
--- cfe/trunk/test/Sema/attr-osobject.cpp (original)
+++ cfe/trunk/test/Sema/attr-osobject.cpp Thu Dec  6 14:06:59 

r348531 - [analyzer] Fix an infinite recursion bug while checking parent methods in RetainCountChecker

[George Karpenkov via cfe-commits]

Author: george.karpenkov
Date: Thu Dec  6 14:06:44 2018
New Revision: 348531

URL: http://llvm.org/viewvc/llvm-project?rev=348531=rev
Log:
[analyzer] Fix an infinite recursion bug while checking parent methods in 
RetainCountChecker

Differential Revision: https://reviews.llvm.org/D55351

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
cfe/trunk/test/Analysis/osobject-retain-release.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp?rev=348531=348530=348531=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp Thu Dec  6 
14:06:44 2018
@@ -730,7 +730,7 @@ bool applyFunctionParamAnnotationEffect(
 if (const auto *MD = dyn_cast(FD)) {
   for (const auto *OD : MD->overridden_methods()) {
 const ParmVarDecl *OP = OD->parameters()[parm_idx];
-if (applyFunctionParamAnnotationEffect(OP, parm_idx, MD, AF, Template))
+if (applyFunctionParamAnnotationEffect(OP, parm_idx, OD, AF, Template))
   return true;
   }
 }

Modified: cfe/trunk/test/Analysis/osobject-retain-release.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/osobject-retain-release.cpp?rev=348531=348530=348531=diff
==
--- cfe/trunk/test/Analysis/osobject-retain-release.cpp (original)
+++ cfe/trunk/test/Analysis/osobject-retain-release.cpp Thu Dec  6 14:06:44 2018
@@ -45,6 +45,8 @@ struct OSArray : public OSObject {
 
   OSObject *identity() override;
 
+  virtual OSObject *generateObject(OSObject *input);
+
   virtual void consumeReference(OS_CONSUME OSArray *other);
 
   static OSArray *generateArrayHasCode() {
@@ -68,6 +70,8 @@ struct MyArray : public OSArray {
   void consumeReference(OSArray *other) override;
 
   OSObject *identity() override;
+
+  OSObject *generateObject(OSObject *input) override;
 };
 
 struct OtherStruct {
@@ -79,6 +83,14 @@ struct OSMetaClassBase {
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
+void test_no_infinite_check_recursion(MyArray *arr) {
+  OSObject *input = new OSObject;
+  OSObject *o = arr->generateObject(input);
+  o->release();
+  input->release();
+}
+
+
 void check_param_attribute_propagation(MyArray *parent) {
   OSArray *arr = new OSArray;
   parent->consumeReference(arr);


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

[PATCH] D55212: Handle alloc_size attribute on function pointers

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from two small style nits with the new test file.




Comment at: test/SemaCXX/alloc-size.cpp:6
+  int alloc(int) __attribute__((alloc_size(2))); // 
expected-warning{{'alloc_size' attribute only applies to return values that are 
pointers}}
+  void* alloc2(int) __attribute__((alloc_size(1))); // 
expected-error{{'alloc_size' attribute is invalid for the implicit this 
argument}}
+  void* alloc3(int) __attribute__((alloc_size(2))); // fine

Can you run this new test file through clang-format?



Comment at: test/SemaCXX/alloc-size.cpp:21
+// We should not be warning when assigning function pointers with and without 
the alloc size attribute
+// since it doesn't change the type of the function
+typedef void * (__attribute__((alloc_size(1))) * 
my_malloc_fn_pointer_type)(int);

Add a full stop at the end of the sentence.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55212/new/

https://reviews.llvm.org/D55212



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

Re: [cfe-dev] r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

[Richard Smith via cfe-commits]

On Wed, 5 Dec 2018 at 11:14, Alex L via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> We have about 100k unique occurrences of this warning across a slice of
> our software stack. It's simply not feasible to us to adopt it, so we would
> prefer for it to be reverted to avoid downstream divergence in Apple's
> clang. Granted, these occur in projects that use -Weverything but those
> projects are typically understanding of warnings that they can turn off
> when they see that they provide useful value in terms of warnings about
> unintended behavior that's sometimes caught. This warning doesn't seem to
> catch unintended bugs and it would be a heroic battle to adopt it for us in
> a way where projects are somewhat satisfied. It seems to be a better fit
> for a clang-tidy check that could suggest automatic fixits and that the
> users could run on their codebase to "modernize" if they want to.
>

Please refer to the review thread where -Weverything was added:


http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110815/045292.html

Users using -Weverything have always been expected to turn off warnings
that they find not useful for their use cases (or to not use -Weverything
if they cannot tolerate new warnings) -- in fact, the very model for
-Weverything is that you see new warnings and turn off the ones you don't
like. That's the model that users explicitly opt into when they ask for
-Weveryhing. It would be harmful to allow only lowest-common-denominator
warnings in order to avoid breaking people who incautiously opted into
-Weverything -Werror, so the fact that a disabled-by-default warning is
problematic for such users is not a compelling reason to remove the warning.

If the warning itself does not objectively have merit, then we can remove
it, but the fact that it breaks -Weverything -Werror builds shouldn't be a
consideration.

Cheers,
> Alex
>
> On Wed, 5 Dec 2018 at 10:43, Richard Smith via cfe-dev <
> cfe-...@lists.llvm.org> wrote:
>
>> On Wed, 5 Dec 2018, 10:01 George Karpenkov via cfe-commits <
>> cfe-commits@lists.llvm.org wrote:
>>
>>> These are the cases I get:
>>>
>>> #define unexpected(a) { kprintf("unexpected %s:%d\n", __FILE__,
>>> __LINE__); a; }
>>>
>>> and
>>>
>>> #if 0
>>> #define DEBG(fmt, args...)  { IOLog(fmt, ## args); kprintf(fmt, ##
>>> args); }
>>> #else
>>> #define DEBG(fmt, args...)  {}
>>> #endif
>>>
>>> Now calls
>>>
>>> `DEBG(‘x’);` and `unexpected(‘msg’);`
>>>
>>> cause the warning to be fired.
>>> Of course, semicolon could be omitted, but the fact that the macro has
>>> braces is sort of an implementation detail.
>>>
>>> Greg Parker has pointed out it’s possible to rewrite the macros using
>>> the “do { … } while (0)”,
>>> but that is a lot of macros to change, and the resulting code would be
>>> arguably less readable.
>>>
>>
>> That suggestion is the (or at least an) idiomatic way to write a
>> "statement-like" macro that expects a semicolon. The approach taken by the
>> above macros is often considered to be a macro antipattern (try putting it
>> in an unbraced if and adding an else clause, for example).
>>
>>> On Dec 5, 2018, at 5:10 AM, Aaron Ballman 
>>> wrote:
>>>
>>> On Wed, Dec 5, 2018 at 1:40 AM Roman Lebedev 
>>> wrote:
>>>
>>>
>>> It is a problem in practice for us since we have projects which enable
>>> all available warnings and also enable “-Werror”.
>>>
>>> Hm, they have asked for -Weverything and they got it. Seems to be
>>> working as intended.
>>> When in previous discussions elsewhere i have talked about
>>> -Weverything, i was always
>>> been told that it isn't supposed to be used like that (admittedly, i
>>> *do* use it like that :)).
>>>
>>> Could you explain how is this different from any other warning that is
>>> considered pointless by the project?
>>> Why not simply disable it?
>>>
>>>
>>> You are right, it could be disabled. It’s just for this warning the
>>> noise vs. usefulness ratio seemed pretty low.
>>>
>>>
>>> Would you be okay with the warning if it was only diagnosed when the
>>> source location of the semi-colon is not immediately preceded by a
>>> macro expansion location? e.g.,
>>>
>>> EMPTY_EXPANSION(12); // Not diagnosed
>>> EMPTY_EXPANSION; // Not diagnosed
>>> ; // Diagnosed
>>>
>>> This could work provided that all empty space preceding the semicolon is
>>> ignored (as the macro could be separated by space(s) or newline(s).
>>> I’m not sure the complexity would be worth it, as I haven’t seen bugs
>>> arising from extra semicolons and empty statements,
>>> but it would take care of my use case.
>>>
>>>
>>> Or rather when the *previous* semicolon is coming from a macro.
>>>
>>> I don't like this. clang is already wa-a-ay too forgiving about
>>> macros, it almost ignores almost every warning in macros.
>>> It was an *intentional* decision to still warn on useless ';' after
>>> non-empty macros.
>>>
>>>
>>> I think I'm confused now. This is a test case:
>>>
>>> NULLMACRO(v7); // OK. This could be either 

[PATCH] D55382: Make -Wstring-plus-int warns even if when the result is not out of bounds

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner added a comment.

Nico: I've added you as reviewer since you originally implemented this warning 
(thanks for this BTW :) as said, it's really helpful), but feel free to 
delegate to someone else.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55382/new/

https://reviews.llvm.org/D55382



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

[PATCH] D55382: Make -Wstring-plus-int warns even if when the result is not out of bounds

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner added a comment.

I found this warning to be really useful but was confused that it is not always 
triggered.
I initially discovered -Wstring-plus-char (and fixed some issues in the 
projects I work on where sometimes developers didn't realized the "+" wasn't 
doing to do concatenation because it was applied on char and char*).
Then I also activated -Werror=string-plus-int to reduce the risk of developers 
doing mistakes in our codebase.

Turns out that because we had some buggy code not catch by this warning for 
this reason: the result of the concatenation was not out of bounds, so the 
warning wasn't triggered.

I understand that point of view, but IMHO this warning should be activated even 
though, like for -Wstring-plus-char: it's easy to fix the warning by having a 
nicer, more readable code, and IMO code raising this warning is likely to 
indicate an issue in the code.
Having developers accidentally concatenating string and integer can happen 
(even more if when not concatenating literals directly).
But I've found a bug in our codebase even more tricky: when concatenating enum 
and literals char* strings:
We had a code like this (using Qt framework):

  QString path = QStandardPaths::StandardLocation(QStandardPaths::DataLocation) 
+ "/filename.ext";

The developer was trying to use QStandardPaths::standardLocations [1] static 
function (mind the lowercase s and the extra s at the end) which takes an enum 
of type QStandardPaths::StandardLocation. Similar name (so easy to do a typo)  
but very different meanings.
So instead of getting a string object and concatenating it with some literal 
strings, path was set to a truncated version of "/filename.ext".
This kind of bugs is hard to catch during code review process (and wasn't in my 
case).

Long story, but I think it's interesting to illustrate the need to have this 
warning applied to more cases.

[1]: http://doc.qt.io/qt-5/qstandardpaths.html#standardLocations


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55382/new/

https://reviews.llvm.org/D55382



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

[PATCH] D55212: Handle alloc_size attribute on function pointers

[Alexander Richardson via Phabricator via cfe-commits]

arichardson updated this revision to Diff 177027.
arichardson added a comment.

Add a C++ test for alloc_size


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55212/new/

https://reviews.llvm.org/D55212

Files:
  include/clang/Basic/Attr.td
  lib/AST/ExprConstant.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/alloc-size-fnptr.c
  test/CodeGen/alloc-size.c
  test/Sema/alloc-size.c
  test/SemaCXX/alloc-size.cpp

Index: test/SemaCXX/alloc-size.cpp
===
--- /dev/null
+++ test/SemaCXX/alloc-size.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 %s -verify
+
+class Test {
+public:
+  int alloc(int) __attribute__((alloc_size(2))); // expected-warning{{'alloc_size' attribute only applies to return values that are pointers}}
+  void* alloc2(int) __attribute__((alloc_size(1))); // expected-error{{'alloc_size' attribute is invalid for the implicit this argument}}
+  void* alloc3(int) __attribute__((alloc_size(2))); // fine
+  void* alloc4(int) __attribute__((alloc_size(3))); // expected-error{{'alloc_size' attribute parameter 1 is out of bounds}}
+  void* alloc5(int, int) __attribute__((alloc_size(1, 2))); // expected-error{{'alloc_size' attribute is invalid for the implicit this argument}}
+  void* alloc6(int, int) __attribute__((alloc_size(2, 1))); // expected-error{{'alloc_size' attribute is invalid for the implicit this argument}}
+  void* alloc7(int, int) __attribute__((alloc_size(2, 3))); // fine
+  // cannot overload based on alloc_size attribute:
+  void* overload(int, int) __attribute__((alloc_size(2, 3))); // expected-note{{previous declaration is here}}
+  void* overload(int, int); // expected-error{{class member cannot be redeclared}}
+};
+// Check that we can't overload functions based on alloc_size on parameters:
+void test1(void * (__attribute__((alloc_size(1)))* fn_ptr_with_attr)(int)) { } // expected-note{{previous definition is here}}
+void test1(void * (*fn_ptr_without_attr)(int)) { } // expected-error {{redefinition of 'test1'}}
+
+// We should not be warning when assigning function pointers with and without the alloc size attribute
+// since it doesn't change the type of the function
+typedef void * (__attribute__((alloc_size(1))) * my_malloc_fn_pointer_type)(int);
+typedef void * (* my_other_malloc_fn_pointer_type)(int);
+void *fn(int i);
+__attribute__((alloc_size(1))) void *fn2(int i);
+
+int main() {
+  my_malloc_fn_pointer_type f = fn;
+  my_other_malloc_fn_pointer_type f2 = fn;
+  my_malloc_fn_pointer_type f3 = fn2;
+  my_other_malloc_fn_pointer_type f4 = fn2;
+}
Index: test/Sema/alloc-size.c
===
--- test/Sema/alloc-size.c
+++ test/Sema/alloc-size.c
@@ -14,7 +14,7 @@
 
 int fail9(int a) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to return values that are pointers}}
 
-int fail10 __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}
+int fail10 __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to non-K functions}}
 
 void *fail11(void *a) __attribute__((alloc_size(1))); //expected-error{{'alloc_size' attribute argument may only refer to a function parameter of integer type}}
 
@@ -22,4 +22,39 @@
 void *fail12(int a) __attribute__((alloc_size(1, "abc"))); //expected-error{{'alloc_size' attribute requires parameter 2 to be an integer constant}}
 void *fail13(int a) __attribute__((alloc_size(1U<<31))); //expected-error{{integer constant expression evaluates to value 2147483648 that cannot be represented in a 32-bit signed integer type}}
 
-int (*PR31453)(int) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}
+void *(*PR31453)(int)__attribute__((alloc_size(1)));
+
+void *KR() __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to non-K functions}}
+
+// Applying alloc_size to function pointers should work:
+void *(__attribute__((alloc_size(1))) * func_ptr1)(int);
+void *(__attribute__((alloc_size(1, 2))) func_ptr2)(int, int);
+
+// TODO: according to GCC documentation the following should actually be the type
+// “pointer to pointer to alloc_size attributed function returning void*” and should
+// therefore be supported
+void *(__attribute__((alloc_size(1))) **ptr_to_func_ptr)(int); // expected-warning{{'alloc_size' attribute only applies to non-K functions}}
+// The following definitions apply the attribute to the pointer to the function pointer which should not be possible
+void *(*__attribute__((alloc_size(1))) * ptr_to_func_ptr2)(int); // expected-warning{{'alloc_size' attribute only applies to non-K functions}}
+void *(**__attribute__((alloc_size(1))) ptr_to_func_ptr2)(int);  // expected-warning{{'alloc_size' attribute only applies to non-K functions}}
+
+// It should also work for typedefs:
+typedef 

[PATCH] D55382: Make -Wstring-plus-int warns even if when the result is not out of bounds

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner created this revision.
Herald added a subscriber: cfe-commits.

Repository:
  rC Clang

https://reviews.llvm.org/D55382

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/string-plus-int.cpp


Index: test/SemaCXX/string-plus-int.cpp
===
--- test/SemaCXX/string-plus-int.cpp
+++ test/SemaCXX/string-plus-int.cpp
@@ -31,37 +31,36 @@
   consume("foo" + 5);  // expected-warning {{adding 'int' to a string does not 
append to the string}} expected-note {{use array indexing to silence this 
warning}}
   consume("foo" + index);  // expected-warning {{adding 'int' to a string does 
not append to the string}} expected-note {{use array indexing to silence this 
warning}}
   consume("foo" + kMyEnum);  // expected-warning {{adding 'MyEnum' to a string 
does not append to the string}} expected-note {{use array indexing to silence 
this warning}}
+  consume("foo" + kMySmallEnum); // expected-warning {{adding 'MyEnum' to a 
string does not append to the string}} expected-note {{use array indexing to 
silence this warning}}
 
   consume(5 + "foo");  // expected-warning {{adding 'int' to a string does not 
append to the string}} expected-note {{use array indexing to silence this 
warning}}
   consume(index + "foo");  // expected-warning {{adding 'int' to a string does 
not append to the string}} expected-note {{use array indexing to silence this 
warning}}
   consume(kMyEnum + "foo");  // expected-warning {{adding 'MyEnum' to a string 
does not append to the string}} expected-note {{use array indexing to silence 
this warning}}
+  consume(kMySmallEnum + "foo"); // expected-warning {{adding 'MyEnum' to a 
string does not append to the string}} expected-note {{use array indexing to 
silence this warning}}
 
   // FIXME: suggest replacing with "foo"[5]
   consumeChar(*("foo" + 5));  // expected-warning {{adding 'int' to a string 
does not append to the string}} expected-note {{use array indexing to silence 
this warning}}
   consumeChar(*(5 + "foo"));  // expected-warning {{adding 'int' to a string 
does not append to the string}} expected-note {{use array indexing to silence 
this warning}}
 
   consume(L"foo" + 5);  // expected-warning {{adding 'int' to a string does 
not append to the string}} expected-note {{use array indexing to silence this 
warning}}
+  consume(L"foo" + 2); // expected-warning {{adding 'int' to a string does not 
append to the string}} expected-note {{use array indexing to silence this 
warning}}
+
+  consume("foo" + 3);  // expected-warning {{adding 'int' to a string does not 
append to the string}} expected-note {{use array indexing to silence this 
warning}}
+  consume("foo" + 4);  // expected-warning {{adding 'int' to a string does not 
append to the string}} expected-note {{use array indexing to silence this 
warning}}
+  consume("\pfoo" + 4);  // expected-warning {{adding 'int' to a string does 
not append to the string}} expected-note {{use array indexing to silence this 
warning}}
+
+  #define A "foo"
+  #define B "bar"
+  consume(A B + sizeof(A) - 1); // expected-warning {{to a string does not 
append to the string}} expected-note {{use array indexing to silence this 
warning}}
 
   // Should not warn.
   consume(&("foo"[3]));
   consume(&("foo"[index]));
   consume(&("foo"[kMyEnum]));
-  consume("foo" + kMySmallEnum);
-  consume(kMySmallEnum + "foo");
 
-  consume(L"foo" + 2);
-
-  consume("foo" + 3);  // Points at the \0
-  consume("foo" + 4);  // Points 1 past the \0, which is legal too.
-  consume("\pfoo" + 4);  // Pascal strings don't have a trailing \0, but they
- // have a leading length byte, so this is fine too.
 
   consume("foo" + kMyOperatorOverloadedEnum);
   consume(kMyOperatorOverloadedEnum + "foo");
-
-  #define A "foo"
-  #define B "bar"
-  consume(A B + sizeof(A) - 1);
 }
 
 template 
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -9133,16 +9133,6 @@
   if (!IsStringPlusInt || IndexExpr->isValueDependent())
 return;
 
-  Expr::EvalResult Result;
-  if (IndexExpr->EvaluateAsInt(Result, Self.getASTContext())) {
-llvm::APSInt index = Result.Val.getInt();
-unsigned StrLenWithNull = StrExpr->getLength() + 1;
-if (index.isNonNegative() &&
-index <= llvm::APSInt(llvm::APInt(index.getBitWidth(), StrLenWithNull),
-  index.isUnsigned()))
-  return;
-  }
-
   SourceRange DiagRange(LHSExpr->getBeginLoc(), RHSExpr->getEndLoc());
   Self.Diag(OpLoc, diag::warn_string_plus_int)
   << DiagRange << IndexExpr->IgnoreImpCasts()->getType();


Index: test/SemaCXX/string-plus-int.cpp
===
--- test/SemaCXX/string-plus-int.cpp
+++ test/SemaCXX/string-plus-int.cpp
@@ -31,37 +31,36 @@
   consume("foo" + 5);  // expected-warning {{adding 'int' to a string does not append to the 

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> In fact -mllvm is used extensively in a lot of lit/FileCheck tests, so that's 
> also going to cause problems.

I count 13 uses of -mllvm in driver commands in-tree, and some of those are 
actually testing the flag itself.  That doesn't seem like a lot.  (This is 
excluding uses in CHECK lines and "clang -cc1" invocations.)

> Possibly you missed that this flag is exempt from -Werror -- it _only_ prints 
> a warning, and is not an error, even when -Werror is specified.

If the warning isn't going to honor -Werror, you might as well not print it at 
all; for many projects, nobody reads the logs of a successful build.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

Thanks, LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55262/new/

https://reviews.llvm.org/D55262



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D55150#1321793 , @jyknight wrote:

> In D55150#1321759 , 
> @george.karpenkov wrote:
>
> > Using `-Xclang` is the only way to pass options to the static analyzer, I 
> > don't think we should warn on it.
>
>
> Well,, that seems unfortunate if we have the only supported interface for the 
> static analyzer be an internal interface. Perhaps it can be given a different 
> option? Even discounting this change, I that seems like it would be 
> appropriate.


It's not really "supported" as in "we encourage users to use it". However, 
there's a third layer of "supported" here: we encourage external GUIs for the 
Static Analyzer to take advantage of these options.

Static Analyzer is, by design, almost unusable as a stand-alone command line 
tool and is only intended to be used via either the `scan-build` tool (a 
command-line tool that turns Static Analyzer's output into a fancy HTML 
output), or IDE intergration.

Different GUI developers will always want to use different internal flags and 
we cannot really control it. So, as a middle-ground solution, we keep these 
flags internal because people are not supposed to specify them manually (other 
than while developing the Static Analyzer itself), but GUIs are anyway allowed 
to take advantage of arbitrary combinations of them at their own risk.

It will be a good idea for us to settle at supporting different combinations, 
but we're not there yet. It might be a good idea to duplicate options that will 
be supported forever into frontend options, but this also needs work.

If the `-Wwarn-drv-xclang-option` is introduced, GUIs that use `-Xclang` and 
also display warnings will start displaying that warning, and will not stop 
doing it until they themselves are updated. This makes it harder to use new 
clang with old GUIs. Which is not a huge deal, but may have unexpected annoying 
consequences.

So, well, i'm not super against that, the overall idea seems good long-term, 
but i'm worried that there'd be a certain time interval of increased annoyance 
while the various GUI tools adapt.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321829 , @efriedma wrote:

> I'm not sure that putting a warning that can be disabled really helps here; 
> anyone who needs the option will just disable the warning anyway, and then 
> users adding additional options somewhere else in the build system will miss 
> the warning.
>
> Instead, it would probably be better to rename Xclang and mllvm to something 
> that makes it clear the user is doing something unsupported. Maybe 
> "--unstable-llvm-option" and "--unstable-clang-option" or something like 
> that.  (This will lead to some breakage, but the breakage is roughly 
> equivalent for anyone using -Werror.)


Possibly you missed that this flag is exempt from -Werror -- it _only_ prints a 
warning, and is not an error, even when -Werror is specified.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55378: [compiler-rt] [test] Add missing cmake include for building libFuzzer alone

[Michał Górny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348524: [test] Add missing cmake include for building 
libFuzzer alone (authored by mgorny, committed by ).
Herald added a subscriber: delcypher.

Changed prior to commit:
  https://reviews.llvm.org/D55378?vs=177017=177025#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55378/new/

https://reviews.llvm.org/D55378

Files:
  compiler-rt/trunk/lib/fuzzer/tests/CMakeLists.txt


Index: compiler-rt/trunk/lib/fuzzer/tests/CMakeLists.txt
===
--- compiler-rt/trunk/lib/fuzzer/tests/CMakeLists.txt
+++ compiler-rt/trunk/lib/fuzzer/tests/CMakeLists.txt
@@ -1,3 +1,5 @@
+include(CompilerRTCompile)
+
 set(LIBFUZZER_UNITTEST_CFLAGS
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}


Index: compiler-rt/trunk/lib/fuzzer/tests/CMakeLists.txt
===
--- compiler-rt/trunk/lib/fuzzer/tests/CMakeLists.txt
+++ compiler-rt/trunk/lib/fuzzer/tests/CMakeLists.txt
@@ -1,3 +1,5 @@
+include(CompilerRTCompile)
+
 set(LIBFUZZER_UNITTEST_CFLAGS
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina requested changes to this revision.
kristina added a comment.

In D55150#1321829 , @efriedma wrote:

> I'm not sure that putting a warning that can be disabled really helps here; 
> anyone who needs the option will just disable the warning anyway, and then 
> users adding additional options somewhere else in the build system will miss 
> the warning.
>
> Instead, it would probably be better to rename Xclang and mllvm to something 
> that makes it clear the user is doing something unsupported. Maybe 
> "--unstable-llvm-option" and "--unstable-clang-option" or something like 
> that.  (This will lead to some breakage, but the breakage is roughly 
> equivalent for anyone using -Werror.)


Thinking about it more, downstream forks with custom passes may utilize those 
flags in tests, renaming them is definitely not the way to go, that is going to 
cause a lot of problem and possibly a lot of angry downstream users as well as 
contributors. Some out-of-tree test suites will treat warnings as failures so 
that behavior by default is also a possible cause for concern. I *really* think 
just changing the documentation to inform consumers about what the flags are 
intended for. In fact `-mllvm` is used extensively in a lot of lit/FileCheck 
tests, so that's also going to cause problems.

I think it's best to just document these options better, I agree, the 
documentation is extremely poor but anything beyond that will/could cause 
issues in so many places.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Or actually, if you really want to discourage people from using them, maybe we 
could use the LLVM version number in the name, like "--unstable-llvm-option-8" 
(which would change to "--unstable-llvm-option-9" in in 9.0, etc.).  This would 
allow developers to continue using the same workflows, but it would strongly 
discourage users from putting them into their build systems.

I don't think the argument that Swift or other users need Xclang options to 
hide them from users makes sense; stable workflows should use real driver 
flags.  If the flag names are clear that they aren't intended for general use, 
that should be good enough.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: lib/Index/USRGeneration.cpp:274
+if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
   Out << (char)('0' + quals);
 switch (MD->getRefQualifier()) {

rjmccall wrote:
> akyrtzi wrote:
> > rjmccall wrote:
> > > Paging @akyrtzi here.  The address-space qualifier should be part of the 
> > > USR but I don't think if there's a defined schema for that.
> > If I understand correctly from other comments you can't add special 
> > mangling and USR generation yet and intend to add FIXME for doing mangling 
> > support later ? If yes, could you also add FIXME here and re-visit ?
> While you're here, can you described the right way for us to extend USR 
> generation here to potentially add an address space?
I'm not familiar with the mechanism, is address space supposed to be identified 
via a single integer ? If so, then I think adding '-' character plus the 
integer as character for the address space, immediately after the type 
qualifiers, should be a way to go.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54862/new/

https://reviews.llvm.org/D54862



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> Well,, that seems unfortunate if we have the only supported interface for the 
> static analyzer be an internal interface. Perhaps it can be given a different 
> option? Even discounting this change, I that seems like it would be 
> appropriate.

Officially there is, it's called `-Xanalyzer`, but in practice it does the same 
as `-Xclang`, and users use the two interchangeably.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I'm not sure that putting a warning that can be disabled really helps here; 
anyone who needs the option will just disable the warning anyway, and then 
users adding additional options somewhere else in the build system will miss 
the warning.

Instead, it would probably be better to rename Xclang and mllvm to something 
that makes it clear the user is doing something unsupported. Maybe 
"--unstable-llvm-option" and "--unstable-clang-option" or something like that.  
(This will lead to some breakage, but the breakage is roughly equivalent for 
anyone using -Werror.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321759 , @george.karpenkov 
wrote:

> Using `-Xclang` is the only way to pass options to the static analyzer, I 
> don't think we should warn on it.


Well,, that seems unfortunate if we have the only supported interface for the 
static analyzer be an internal interface. Perhaps it can be given a different 
option? Even discounting this change, I that seems like it would be appropriate.

In D55150#1321771 , @arphaman wrote:

> Swift uses `-Xclang` to pass in build settings to its own build and to pass 
> in custom options through its Clang importer that we intentionally don't want 
> to expose to Clang's users. We don't want to warn for those uses for sure.


I'm not sure if I understand correctly, so I'll try to reiterate: Swift calls 
clang internally, and when it does so, intentionally uses options that are not 
generally intended for others to use. Of course we shouldn't emit warnings in 
that case.

Is that a correct understanding? If so, doesn't it just make sense for that 
constructed command-line to disable the warning? And, isn't it good that it 
will warn, otherwise, in order to discourage people from using those flags that 
are intentionally-not-exposed?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In D55150#1321734 , @thakis wrote:

> I don't have an opinion on this patch (if you force me to have one, I'm 
> weakly in favor), but I agree with the general sentiment. When I told people 
> to not use mllvm and Xclang before, they told me "but if I'm not supposed to 
> use them, why are they advertised in --help"?
>
>   thakis@thakis:~/src/llvm-build$ bin/clang --help | grep Xclang
> -XclangPass  to the clang compiler
>   thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
> -mllvm   Additional arguments to forward to LLVM's option 
> processing
>
>
> Which to me is a valid point! Maybe we should remove them from --help or say 
> "internal only, don't usually use" there?


I think the documentation for `-mllvm` could definitely emphasize the aspect 
that these flags are not a part of any public or stable interface and are used 
to change internal behavior of LLVM components a lot more. Definitely in favor 
of doing that.

In D55150#1321724 , @jyknight wrote:

> In D55150#1321705 , @kristina wrote:
>
> > > There is a cost to having people encode these flags into their build 
> > > systems -- it can then cause issues if we ever change these internal 
> > > flags. I do not think any Clang maintainer intends to support these as 
> > > stable APIs, unlike most of the driver command-line. But, neither -Xclang 
> > > nor -mllvm obviously scream out "don't use this!", and so people may then 
> > > add them to their buildsystems without causing reviewers to say 
> > > "Wait...really? Are you sure that's a good idea?".
> >
> > This is why I proposed a compromise, allow this warning to be disabled 
> > completely for people actively using those flags, which are pretty much 
> > exclusively toolchain developers (well basically what I proposed, since 
> > it's not clear what counts as a build made by someone who is working and 
> > debugging a pass, being fully aware of those flags, using the subset of 
> > them specific to that pass/feature, I would say assertion+dump enabled 
> > builds are closest, but having an explicit build flag for it would be 
> > nicer). It's more unified than everyone either adding workarounds into 
> > build systems or disabling it completely (by just removing it).
>
>
> I mean, I'm not much opposed to adding that -- just that any new build-time 
> options is always a minor shame. But you didn't respond to the other part of 
> my message -- is adding `-Wno-experimental-driver-option` to your 
> compile-flags not already a completely sufficient solution for your use-case?
>
> > Besides, I don't think this really ever surfaced as an issue, until 
> > Chandler's idea with regards to an unsupressable warning for performance 
> > tests for 7.x.x
>
> The primary impetus for me was actually the discovery that Boost's build 
> system was using "-Xclang -include-pth" a few weeks back.


It would be an inconvenience to developers and a lot of patches grow from 
downstream, given that some people may want to disable the flag for 
development, it would basically mean that some may end up with varying 
workarounds as opposed to a uniform one. I don't think that happening would be 
of benefit to anyone. And yes I could add an extra flag for that configuration 
in the build system, in my case, but I don't think I'll be the only one doing 
that so again, this would just cause more fragmentation. I don't see that as a 
good thing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55378: [compiler-rt] [test] Add missing cmake include for building libFuzzer alone

[Michał Górny via Phabricator via cfe-commits]

mgorny created this revision.
mgorny added reviewers: phosek, george.karpenkov.
Herald added subscribers: Sanitizers, llvm-commits, dberris.

Include CompilerRTCompile in fuzzer tests explicitly.  Otherwise, when
building only libFuzzer, CMake fails due to:

  CMake Error at cmake/Modules/AddCompilerRT.cmake:395 (sanitizer_test_compile):
Unknown CMake command "sanitizer_test_compile".
  Call Stack (most recent call first):
lib/fuzzer/tests/CMakeLists.txt:53 (generate_compiler_rt_tests)


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D55378

Files:
  lib/fuzzer/tests/CMakeLists.txt


Index: lib/fuzzer/tests/CMakeLists.txt
===
--- lib/fuzzer/tests/CMakeLists.txt
+++ lib/fuzzer/tests/CMakeLists.txt
@@ -1,3 +1,5 @@
+include(CompilerRTCompile)
+
 set(LIBFUZZER_UNITTEST_CFLAGS
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}


Index: lib/fuzzer/tests/CMakeLists.txt
===
--- lib/fuzzer/tests/CMakeLists.txt
+++ lib/fuzzer/tests/CMakeLists.txt
@@ -1,3 +1,5 @@
+include(CompilerRTCompile)
+
 set(LIBFUZZER_UNITTEST_CFLAGS
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Alex Lorenz via Phabricator via cfe-commits]

arphaman requested changes to this revision.
arphaman added a comment.

Swift uses `-Xclang` to pass in build settings to its own build and to pass in 
custom options through its Clang importer that we intentionally don't want to 
expose to Clang's users. We don't want to warn for those uses for sure.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D52956: Support `-fno-visibility-inlines-hidden`

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Is this change blocked on something?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52956/new/

https://reviews.llvm.org/D52956



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

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Thanks! I'll commit in a couple days to give some breathing room for anyone 
who'd object.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54401/new/

https://reviews.llvm.org/D54401



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added reviewers: dcoughlin, NoQ.
george.karpenkov requested changes to this revision.
george.karpenkov added a comment.

Using `-Xclang` is the only way to pass options to the static analyzer, I don't 
think we should warn on it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'm in favor of at least the documentation changes, though I'd like to see them 
use stronger language.  I'm fairly in favor of the warning itself since its 
easy enough to disable (with the -Wno flag), so I don't terribly understand 
those against it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55377: Allow forwarding -fdebug-compilation-dir to cc1as

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348515: Allow forwarding -fdebug-compilation-dir to cc1as 
(authored by nico, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55377?vs=177004=177015#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55377/new/

https://reviews.llvm.org/D55377

Files:
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/test/Driver/integrated-as.s


Index: cfe/trunk/test/Driver/integrated-as.s
===
--- cfe/trunk/test/Driver/integrated-as.s
+++ cfe/trunk/test/Driver/integrated-as.s
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 
2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s 
-Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler 
-fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck 
--check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -2152,6 +2152,9 @@
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;


Index: cfe/trunk/test/Driver/integrated-as.s
===
--- cfe/trunk/test/Driver/integrated-as.s
+++ cfe/trunk/test/Driver/integrated-as.s
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler -fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck --check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -2152,6 +2152,9 @@
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Oh hey, the patch does that already! Ignore me, then :-) That part is probably 
less controversial, maybe you want to land that while the warning part is being 
discussed?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

I don't really see the point of this and think it will just be an inconvenience 
to llvm developers.

Another use case we have for using these in a build system is for the builtin 
library shipped with the compiler


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[Romanov Vlad via Phabricator via cfe-commits]

romanovvlad updated this revision to Diff 177012.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55262/new/

https://reviews.llvm.org/D55262

Files:
  lib/CodeGen/CGExpr.cpp
  test/CodeGenOpenCLCXX/address-space-deduction2.cl


Index: test/CodeGenOpenCLCXX/address-space-deduction2.cl
===
--- test/CodeGenOpenCLCXX/address-space-deduction2.cl
+++ test/CodeGenOpenCLCXX/address-space-deduction2.cl
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm 
-o - | FileCheck %s
+
+class P {
+public:
+  P(const P ) = default;
+
+  long A;
+  long B;
+};
+
+void foo(__global P *GPtr) {
+// CHECK: call void @llvm.memcpy{{.*}}, {{.*}}, i32 16
+  P Val = GPtr[0];
+}
+
+struct __attribute__((packed)) A { int X; };
+int test(__global A *GPtr) {
+// CHECK: {{.*}} = load i32, {{.*}}, align 1
+  return static_cast<__generic A &>(*GPtr).X;
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -4269,8 +4269,9 @@
 QualType DestTy = getContext().getPointerType(E->getType());
 llvm::Value *V = getTargetHooks().performAddrSpaceCast(
 *this, LV.getPointer(), E->getSubExpr()->getType().getAddressSpace(),
-DestTy.getAddressSpace(), ConvertType(DestTy));
-return MakeNaturalAlignPointeeAddrLValue(V, DestTy);
+E->getType().getAddressSpace(), ConvertType(DestTy));
+return MakeAddrLValue(Address(V, LV.getAddress().getAlignment()),
+  E->getType(), LV.getBaseInfo(), LV.getTBAAInfo());
   }
   case CK_ObjCObjectLValueCast: {
 LValue LV = EmitLValue(E->getSubExpr());


Index: test/CodeGenOpenCLCXX/address-space-deduction2.cl
===
--- test/CodeGenOpenCLCXX/address-space-deduction2.cl
+++ test/CodeGenOpenCLCXX/address-space-deduction2.cl
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s
+
+class P {
+public:
+  P(const P ) = default;
+
+  long A;
+  long B;
+};
+
+void foo(__global P *GPtr) {
+// CHECK: call void @llvm.memcpy{{.*}}, {{.*}}, i32 16
+  P Val = GPtr[0];
+}
+
+struct __attribute__((packed)) A { int X; };
+int test(__global A *GPtr) {
+// CHECK: {{.*}} = load i32, {{.*}}, align 1
+  return static_cast<__generic A &>(*GPtr).X;
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -4269,8 +4269,9 @@
 QualType DestTy = getContext().getPointerType(E->getType());
 llvm::Value *V = getTargetHooks().performAddrSpaceCast(
 *this, LV.getPointer(), E->getSubExpr()->getType().getAddressSpace(),
-DestTy.getAddressSpace(), ConvertType(DestTy));
-return MakeNaturalAlignPointeeAddrLValue(V, DestTy);
+E->getType().getAddressSpace(), ConvertType(DestTy));
+return MakeAddrLValue(Address(V, LV.getAddress().getAlignment()),
+  E->getType(), LV.getBaseInfo(), LV.getTBAAInfo());
   }
   case CK_ObjCObjectLValueCast: {
 LValue LV = EmitLValue(E->getSubExpr());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348515 - Allow forwarding -fdebug-compilation-dir to cc1as

[Nico Weber via cfe-commits]

Author: nico
Date: Thu Dec  6 10:50:39 2018
New Revision: 348515

URL: http://llvm.org/viewvc/llvm-project?rev=348515=rev
Log:
Allow forwarding -fdebug-compilation-dir to cc1as

The flag -fdebug-compilation-dir is useful to make generated .o files
independent of the path of the build directory, without making the compile
command-line dependent on the path of the build directory, like
-fdebug-prefix-map requires. This change makes it so that the driver can
forward the flag to -cc1as, like it already can for -cc1. We might want to
consider making -fdebug-compilation-dir a driver flag in a follow-up.

(Since -fdebug-compilation-dir defaults to PWD, it's already possible to get
this effect by setting PWD, but explicit compiler flags are better than env
vars, because e.g. ninja tracks command lines and reruns commands that change.)

Somewhat related to PR14625.

Differential Revision: https://reviews.llvm.org/D55377

Modified:
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/test/Driver/integrated-as.s

Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=348515=348514=348515=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Thu Dec  6 10:50:39 2018
@@ -2152,6 +2152,9 @@ static void CollectArgsForIntegratedAsse
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;

Modified: cfe/trunk/test/Driver/integrated-as.s
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/integrated-as.s?rev=348515=348514=348515=diff
==
--- cfe/trunk/test/Driver/integrated-as.s (original)
+++ cfe/trunk/test/Driver/integrated-as.s Thu Dec  6 10:50:39 2018
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 
2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s 
-Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler 
-fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck 
--check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."


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

[PATCH] D55377: Allow forwarding -fdebug-compilation-dir to cc1as

[Hans Wennborg via Phabricator via cfe-commits]

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

lgtm


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55377/new/

https://reviews.llvm.org/D55377



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

I don't have an opinion on this patch (if you force me to have one, I'm weakly 
in favor), but I agree with the general sentiment. When I told people to not 
use mllvm and Xclang before, they told me "but if I'm not supposed to use them, 
why are they advertised in --help"?

  thakis@thakis:~/src/llvm-build$ bin/clang --help | grep Xclang
-XclangPass  to the clang compiler
  thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
-mllvm   Additional arguments to forward to LLVM's option 
processing

Which to me is a valid point! Maybe we should remove them from --help or say 
"internal only, don't usually use" there?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/UsersManual.rst:3137
   -W Enable the specified warning
-  -XclangPass  to the clang compiler
+  -XclangPass  to the clang cc1 frontend. For 
experimental use only.
 

One downside to this is that I believe there are still situations where -Xclang 
may be required, such as when loading plugins. This could be mildly annoying 
even without it impacting -Werror because some CI tools do warning counts, it 
chats at users, etc.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321705 , @kristina wrote:

> > There is a cost to having people encode these flags into their build 
> > systems -- it can then cause issues if we ever change these internal flags. 
> > I do not think any Clang maintainer intends to support these as stable 
> > APIs, unlike most of the driver command-line. But, neither -Xclang nor 
> > -mllvm obviously scream out "don't use this!", and so people may then add 
> > them to their buildsystems without causing reviewers to say "Wait...really? 
> > Are you sure that's a good idea?".
>
> This is why I proposed a compromise, allow this warning to be disabled 
> completely for people actively using those flags, which are pretty much 
> exclusively toolchain developers (well basically what I proposed, since it's 
> not clear what counts as a build made by someone who is working and debugging 
> a pass, being fully aware of those flags, using the subset of them specific 
> to that pass/feature, I would say assertion+dump enabled builds are closest, 
> but having an explicit build flag for it would be nicer). It's more unified 
> than everyone either adding workarounds into build systems or disabling it 
> completely (by just removing it).


I mean, I'm not much opposed to adding that -- just that any new build-time 
options is always a minor shame. But you didn't respond to the other part of my 
message -- is adding `-Wno-experimental-driver-option` to your compile-flags 
not already a completely sufficient solution for your use-case?

> Besides, I don't think this really ever surfaced as an issue, until 
> Chandler's idea with regards to an unsupressable warning for performance 
> tests for 7.x.x

The primary impetus for me was actually the discovery that Boost's build system 
was using "-Xclang -include-pth" a few weeks back.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

r348513 - Reapply "Avoid emitting redundant or unusable directories in DIFile metadata entries.""

[Adrian Prantl via cfe-commits]

Author: adrian
Date: Thu Dec  6 10:44:50 2018
New Revision: 348513

URL: http://llvm.org/viewvc/llvm-project?rev=348513=rev
Log:
Reapply "Avoid emitting redundant or unusable directories in DIFile metadata 
entries.""

This reverts commit r348280 and reapplies D55085 without modifications.

Original commit message:

Avoid emitting redundant or unusable directories in DIFile metadata entries.

As discussed on llvm-dev recently, Clang currently emits redundant
directories in DIFile entries, such as

  .file  1 "/Volumes/Data/llvm" 
"/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"

This patch looks at any common prefix between the compilation
directory and the (absolute) file path and strips the redundant
part. More importantly it leaves the compilation directory empty if
the two paths have no common prefix.

After this patch the above entry is (assuming a compilation dir of 
"/Volumes/Data/llvm/_build"):

  .file 1 "/Volumes/Data/llvm" 
"tools/clang/test/CodeGen/debug-info-abspath.c"

When building the FileCheck binary with debug info, this patch makes
the build artifacts ~1kb smaller.

Differential Revision: https://reviews.llvm.org/D55085

Added:
cfe/trunk/test/CodeGen/debug-info-abspath.c
Modified:
cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
cfe/trunk/lib/CodeGen/CodeGenAction.cpp
cfe/trunk/test/CodeGen/debug-prefix-map.c
cfe/trunk/test/Modules/module-debuginfo-prefix.m

Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348513=348512=348513=diff
==
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Thu Dec  6 10:44:50 2018
@@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
   SourceManager  = CGM.getContext().getSourceManager();
   auto *Scope = cast(LexicalBlockStack.back());
   PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
-
-  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
+  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
 return;
 
   if (auto *LBF = dyn_cast(Scope)) {
@@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
   SourceManager  = CGM.getContext().getSourceManager();
   PresumedLoc PLoc = SM.getPresumedLoc(Loc);
 
-  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
+  StringRef FileName = PLoc.getFilename();
+  if (PLoc.isInvalid() || FileName.empty())
 // If the location is not valid then use main input file.
 return getOrCreateMainFile();
 
   // Cache the results.
-  const char *fname = PLoc.getFilename();
-  auto It = DIFileCache.find(fname);
+  auto It = DIFileCache.find(FileName.data());
 
   if (It != DIFileCache.end()) {
 // Verify that the information still exists.
@@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
   if (CSKind)
 CSInfo.emplace(*CSKind, Checksum);
 
-  llvm::DIFile *F = DBuilder.createFile(
-  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()), 
CSInfo,
-  getSource(SM, SM.getFileID(Loc)));
+  StringRef Dir;
+  StringRef File;
+  std::string RemappedFile = remapDIPath(FileName);
+  std::string CurDir = remapDIPath(getCurrentDirname());
+  SmallString<128> DirBuf;
+  SmallString<128> FileBuf;
+  if (llvm::sys::path::is_absolute(RemappedFile)) {
+// Strip the common prefix (if it is more than just "/") from current
+// directory and FileName for a more space-efficient encoding.
+auto FileIt = llvm::sys::path::begin(RemappedFile);
+auto FileE = llvm::sys::path::end(RemappedFile);
+auto CurDirIt = llvm::sys::path::begin(CurDir);
+auto CurDirE = llvm::sys::path::end(CurDir);
+for (; CurDirIt != CurDirE && *CurDirIt == *FileIt; ++CurDirIt, ++FileIt)
+  llvm::sys::path::append(DirBuf, *CurDirIt);
+if (std::distance(llvm::sys::path::begin(CurDir), CurDirIt) == 1) {
+  // The common prefix only the root; stripping it would cause
+  // LLVM diagnostic locations to be more confusing.
+  Dir = {};
+  File = RemappedFile;
+} else {
+  for (; FileIt != FileE; ++FileIt)
+llvm::sys::path::append(FileBuf, *FileIt);
+  Dir = DirBuf;
+  File = FileBuf;
+}
+  } else {
+Dir = CurDir;
+File = RemappedFile;
+  }
+  llvm::DIFile *F =
+  DBuilder.createFile(File, Dir, CSInfo,
+  getSource(SM, SM.getFileID(Loc)));
 
-  DIFileCache[fname].reset(F);
+  DIFileCache[FileName.data()].reset(F);
   return F;
 }
 

Modified: cfe/trunk/lib/CodeGen/CodeGenAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenAction.cpp?rev=348513=348512=348513=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenAction.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenAction.cpp Thu Dec  6 

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/index/Background.cpp:202
 std::lock_guard Lock(QueueMu);
-Queue.push_back(std::move(T));
+if (Priority == ThreadPriority::Low) {
+  Queue.push_back(Bind(

ilya-biryukov wrote:
> Since we might be interested in scheduling higher-priority tasks first anyway 
> (not in this patch, but still), maybe store a pair of `(Task, Priority)` in 
> the queue and call `setCurrentThreadPriority` when actually running the task?
I believe we can introduce that later on whenever we have more than 2 
priorities, currently we push to front for important tasks and back for the low 
priority ones. Do you think it is not enough?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55315/new/

https://reviews.llvm.org/D55315



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

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 177007.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Delete redundant comment


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55315/new/

https://reviews.llvm.org/D55315

Files:
  clangd/Threading.cpp
  clangd/Threading.h
  clangd/index/Background.cpp
  clangd/index/Background.h

Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -13,6 +13,7 @@
 #include "Context.h"
 #include "FSProvider.h"
 #include "GlobalCompilationDatabase.h"
+#include "Threading.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
@@ -110,7 +111,7 @@
   // queue management
   using Task = std::function;
   void run(); // Main loop executed by Thread. Runs tasks from Queue.
-  void enqueueTask(Task T);
+  void enqueueTask(Task T, ThreadPriority Prioirty);
   void enqueueLocked(tooling::CompileCommand Cmd,
  BackgroundIndexStorage *IndexStorage);
   std::mutex QueueMu;
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -22,6 +22,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SHA1.h"
@@ -104,11 +105,6 @@
   assert(this->IndexStorageFactory && "Storage factory can not be null!");
   while (ThreadPoolSize--) {
 ThreadPool.emplace_back([this] { run(); });
-// Set priority to low, since background indexing is a long running task we
-// do not want to eat up cpu when there are any other high priority threads.
-// FIXME: In the future we might want a more general way of handling this to
-// support tasks with various priorities.
-setThreadPriority(ThreadPool.back(), ThreadPriority::Low);
   }
 }
 
@@ -160,44 +156,61 @@
 }
 
 void BackgroundIndex::enqueue(const std::vector ) {
-  enqueueTask([this, ChangedFiles] {
-trace::Span Tracer("BackgroundIndexEnqueue");
-// We're doing this asynchronously, because we'll read shards here too.
-// FIXME: read shards here too.
-
-log("Enqueueing {0} commands for indexing", ChangedFiles.size());
-SPAN_ATTACH(Tracer, "files", int64_t(ChangedFiles.size()));
-
-// We shuffle the files because processing them in a random order should
-// quickly give us good coverage of headers in the project.
-std::vector Permutation(ChangedFiles.size());
-std::iota(Permutation.begin(), Permutation.end(), 0);
-std::mt19937 Generator(std::random_device{}());
-std::shuffle(Permutation.begin(), Permutation.end(), Generator);
-
-for (const unsigned I : Permutation)
-  enqueue(ChangedFiles[I]);
-  });
+  enqueueTask(
+  [this, ChangedFiles] {
+trace::Span Tracer("BackgroundIndexEnqueue");
+// We're doing this asynchronously, because we'll read shards here too.
+// FIXME: read shards here too.
+
+log("Enqueueing {0} commands for indexing", ChangedFiles.size());
+SPAN_ATTACH(Tracer, "files", int64_t(ChangedFiles.size()));
+
+// We shuffle the files because processing them in a random order should
+// quickly give us good coverage of headers in the project.
+std::vector Permutation(ChangedFiles.size());
+std::iota(Permutation.begin(), Permutation.end(), 0);
+std::mt19937 Generator(std::random_device{}());
+std::shuffle(Permutation.begin(), Permutation.end(), Generator);
+
+for (const unsigned I : Permutation)
+  enqueue(ChangedFiles[I]);
+  },
+  ThreadPriority::Normal);
 }
 
 void BackgroundIndex::enqueue(const std::string ) {
   ProjectInfo Project;
   if (auto Cmd = CDB.getCompileCommand(File, )) {
 auto *Storage = IndexStorageFactory(Project.SourceRoot);
+// Set priority to low, since background indexing is a long running
+// task we do not want to eat up cpu when there are any other high
+// priority threads.
 enqueueTask(Bind(
-[this, File, Storage](tooling::CompileCommand Cmd) {
-  Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir);
-  if (auto Error = index(std::move(Cmd), Storage))
-log("Indexing {0} failed: {1}", File, std::move(Error));
-},
-std::move(*Cmd)));
+[this, File, Storage](tooling::CompileCommand Cmd) {
+  Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir);
+  if (auto Error = index(std::move(Cmd), Storage))
+log("Indexing {0} failed: {1}", File, std::move(Error));
+},
+std::move(*Cmd)),
+ThreadPriority::Low);
   }
 

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

> There is a cost to having people encode these flags into their build systems 
> -- it can then cause issues if we ever change these internal flags. I do not 
> think any Clang maintainer intends to support these as stable APIs, unlike 
> most of the driver command-line. But, neither -Xclang nor -mllvm obviously 
> scream out "don't use this!", and so people may then add them to their 
> buildsystems without causing reviewers to say "Wait...really? Are you sure 
> that's a good idea?".

This is why I proposed a compromise, allow this warning to be disabled 
completely for people actively using those flags, which are pretty much 
exclusively toolchain developers (well basically what I proposed, since it's 
not clear what counts as a build made by someone who is working and debugging a 
pass, being fully aware of those flags, using the subset of them specific to 
that pass/feature, I would say assertion+dump enabled builds are closest, but 
having an explicit build flag for it would be nicer). It's more unified than 
everyone either adding workarounds into build systems or disabling it 
completely (by just removing it).

Alternatively just let people shoot themselves in the foot, a documentation 
change regarding the dangers of the flag should suffice. Besides, I don't think 
this really ever surfaced as an issue, until Chandler's idea with regards to an 
unsupressable warning for performance tests for 7.x.x (which is a very specific 
and narrow edge case to allow people to collect performance data). Outside of 
that very specific case, have we really had many issues with consumers 
accidentally setting weird flags that they would have to discover in the first 
place. I don't have a strong opinion on `-Xclang` but `-mllvm ` is 
verbose enough to use as is. Maybe it should be made a lot more obvious in one 
way or another but a warning by default seems like it's taking rather drastic 
preemptive measures against a non-issue (do correct me if I'm wrong here).

I do agree that the documentation should definitely scream that it's not stable 
and it's intended for maintainers since it's a convinient interface (the 
`-mllvm` one) for passing flags through the driver all the way to things like 
the MC layer, where needed, and yes these flags can be removed without notice, 
but again, I don't think it's our responsibility to protect users from using 
*intentionally* hidden flags aside from documenting the reason for why they're 
intentionally hidden in a more obvious way, I think the fact that they are 
intentionally not shown in standard LLVM help and yet are available contradicts 
the idea behind this patch, the whole purpose of the flag is to directly 
interact with specific internal parts of LLVM which in itself is not really 
intended to be a stable interface.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



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

[PATCH] D55377: Allow forwarding -fdebug-compilation-dir to cc1as

[Nico Weber via Phabricator via cfe-commits]

thakis created this revision.
thakis added a reviewer: rnk.

The flag -fdebug-compilation-dir is useful to make generated .o files 
independent of the path of the build directory, without making the compile 
command-line dependent on the path of the build directory, like 
-fdebug-prefix-map requires. This change makes it so that the driver can 
forward the flag to -cc1as, like it already can for -cc1. We might want to 
consider making -fdebug-compilation-dir a driver flag in a follow-up.

(Since -fdebug-compilation-dir defaults to PWD, it's already possible to get 
this effect by setting PWD, but explicit compiler flags are better than env 
vars, because e.g. ninja tracks command lines and reruns commands that change.)

Somewhat related to PR14625.


https://reviews.llvm.org/D55377

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/integrated-as.s


Index: test/Driver/integrated-as.s
===
--- test/Driver/integrated-as.s
+++ test/Driver/integrated-as.s
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 
2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s 
-Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler 
-fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck 
--check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2152,6 +2152,9 @@
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;


Index: test/Driver/integrated-as.s
===
--- test/Driver/integrated-as.s
+++ test/Driver/integrated-as.s
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler -fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck --check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2152,6 +2152,9 @@
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D55262#1321682 , @rjmccall wrote:

> Sorry.  Can you post the result of passing `-ast-dump` instead of 
> `-emit-llvm` to the compiler?


Actually, nevermind, I don't need that.  If you can fix the address-space issue 
I pointed out, I think this is ready to go.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55262/new/

https://reviews.llvm.org/D55262



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

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 177005.
kadircet added a comment.

- Fix a few problems that come up in the field test.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55224/new/

https://reviews.llvm.org/D55224

Files:
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/index/Background.cpp
  clangd/index/Background.h
  clangd/index/BackgroundIndexStorage.cpp
  clangd/index/SymbolCollector.cpp
  test/clangd/background-index.test
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -7,6 +7,7 @@
 
 using testing::_;
 using testing::AllOf;
+using testing::Contains;
 using testing::Not;
 using testing::UnorderedElementsAre;
 
@@ -125,7 +126,7 @@
FileURI("unittest:///root/B.cc")}));
 }
 
-TEST_F(BackgroundIndexTest, ShardStorageWriteTest) {
+TEST_F(BackgroundIndexTest, ShardStorageTest) {
   MockFSProvider FS;
   FS.Files[testPath("root/A.h")] = R"cpp(
   void common();
@@ -154,6 +155,16 @@
   EXPECT_EQ(CacheHits, 0U);
   EXPECT_EQ(Storage.size(), 2U);
 
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return  });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+  EXPECT_EQ(Storage.size(), 2U);
+
   auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_THAT(
@@ -221,5 +232,73 @@
   EmptyIncludeNode());
 }
 
+TEST_F(BackgroundIndexTest, ShardStorageLoad) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return  });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+
+  // Change header.
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  class A_CCnew {};
+  )cpp";
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return  });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+
+  // Change source.
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }\nvoid f_b() {}";
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return  });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardSource->Symbols,
+  Contains(AllOf(Named("f_b"), Declared(), Defined(;
+}
+
 } // namespace clangd
 } // namespace clang
Index: test/clangd/background-index.test
===
--- test/clangd/background-index.test
+++ test/clangd/background-index.test
@@ -16,6 +16,5 @@
 # RUN: ls %t/.clangd-index/foo.cpp.*.idx
 
 # Test the index is read from disk: delete code and restart clangd.
-# FIXME: This test currently fails as we don't read the index yet.
 # RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | not FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ 

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Sorry.  Can you post the result of passing `-ast-dump` instead of `-emit-llvm` 
to the compiler?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55262/new/

https://reviews.llvm.org/D55262



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[Romanov Vlad via Phabricator via cfe-commits]

romanovvlad added a comment.

Backtrace: F7656050: stack 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55262/new/

https://reviews.llvm.org/D55262



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

[PATCH] D42682: [clang-tidy] Add io-functions-misused checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/IoFunctionsCheck.cpp:32
+has(cxxMemberCallExpr(
+on(hasType(namedDecl(hasAnyName("istream",
+callee(cxxMethodDecl(hasName("get")).bind("DeclOfGet"),

`::std::istream`



Comment at: clang-tidy/bugprone/IoFunctionsCheck.cpp:47-49
+  "consider to cast the return value of %0 from type integer to type char, 
"
+  "possible loss of precision if an error has occurred or the end "
+  "of file has been reached");

This diagnostic confuses me, so perhaps you can explain the situation you want 
to diagnose a bit further.

FIO34-C is about situations where `sizeof(char) == sizeof(int)` and the call 
returns EOF/WEOF. In that case, there's no way to distinguish between an 
EOF/error and a valid character. Suggesting to explicitly cast to a character 
type doesn't add any safety to the code -- the user needs to insert calls to 
`feof()` or `ferror()` instead (to make it portable, anyway) and should store 
the character value in a sufficiently large integer type before doing the 
comparison against EOF.

Are you trying to catch a different kind of bug?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D42682/new/

https://reviews.llvm.org/D42682



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

[PATCH] D55371: Fix thunks returning memptrs via sret by emitting also scalar return values directly in sret slot (PR39901)

[John McCall via Phabricator via cfe-commits]

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

That seems reasonable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55371/new/

https://reviews.llvm.org/D55371



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D55262#1321641 , @romanovvlad wrote:

> Yes it's 1, but after addrspacecast alignment becomes not struct A but 
> alignment of the pointer


Oh!  Yes, that's even more broken than I thought.




Comment at: lib/CodeGen/CGExpr.cpp:4272
 *this, LV.getPointer(), E->getSubExpr()->getType().getAddressSpace(),
 DestTy.getAddressSpace(), ConvertType(DestTy));
+return MakeAddrLValue(Address(V, LV.getAddress().getAlignment()),

This call to `getAddressSpace()` doesn't look right: we just constructed 
`DestTy` as an unqualified pointer type.  It needs to be 
`E->getType().getAddressSpace()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55262/new/

https://reviews.llvm.org/D55262



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

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

LGTM, but you can probably undo one of my requests. :)




Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }

mikael wrote:
> mikael wrote:
> > rjmccall wrote:
> > > The indentation here is messed up.
> > > 
> > > You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few 
> > > places.  That's currently correct, in the sense that the fast qualifiers 
> > > are currently defined to be the CVR qualifiers, but the point of the 
> > > distinction is that we might want to change that (and we have in fact 
> > > explored that in the past).  I think you should make `FunctionTypeBits` 
> > > only claim to store the fast qualifiers, which mostly just means updating 
> > > a few field / accessor names and changing things like the 
> > > `getCVRQualifiers()` call right above this to be `getFastQualifiers()`.
> > > 
> > > If there are places where it's convenient to assume that the CVR 
> > > qualifiers are exactly the fast qualifiers, it's okay to static_assert 
> > > that, but you should make sure to static_assert it in each place you 
> > > assume it.
> > I'll change it, but I do have a question related to this:
> > 
> > Can we make any assumptions with the "fast qualifiers"? I'd like it if we 
> > can assume that they fit in 3 bits. Otherwise I think I also need an 
> > assertion here.
> I solved it like this in the end:
> 
> * I defined the FastTypeQuals to get the size of Qualifiers::FastWidth to 
> make it dependent on possible changes to the fast flags.
> * Then I put a static_assertion to guard the hastConst(), hasVolatile() and 
> hasRestrict() methods.
Great, looks good.



Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
+  getOrCreateInstanceMethodType(
+  CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+  FPT, U),

mikael wrote:
> rjmccall wrote:
> > Why is this `getMostRecentCXXRecordDecl` instead of `getClass`?
> Your initial comment suggested to send in a record rather than a type. But I 
> see now that it may make more sense to use the type directly instead. Will 
> update it.
Oh, I actually didn't even think about the fact that the `getClass())` returns 
a `Type*` instead of a `CXXRecordDecl`.  Using the record decl is probably 
better, sorry.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54862/new/

https://reviews.llvm.org/D54862



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

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > Szelethus wrote:
> > > > > MTC wrote:
> > > > > > If I not wrong, `MemFunctionInfo` is mainly a class member of 
> > > > > > `MallocChecker` that hold a bunch of memory function identifiers 
> > > > > > and provide corresponding helper APIs. And we should pick a proper 
> > > > > > time to initialize it.
> > > > > > 
> > > > > > I want to known why you call `initIdentifierInfo()` in 
> > > > > > `checkPostStmt(const CallExpr *E,)`, this callback is called after 
> > > > > > `checkPreCall(const CallEvent , )` in which we need 
> > > > > > `MemFunctionInfo`.
> > > > > Well, I'd like to know too! I think lazy initializing this struct 
> > > > > creates more problems that it solves, but I wanted to draw a line 
> > > > > somewhere in terms of how invasive I'd like to make this patch.
> > > > How about put the initialization stuff into constructor? Let the 
> > > > constructor do the initialization that it should do, let `register*()` 
> > > > do its registration, like setting `setOptimism` and so on.
> > > Interestingly, `MemFunctionInfo` is not always initialized, so a 
> > > performance argument can be made here. I specifically checked whether 
> > > there's any point in doing this hackery, and the answer is... well, some. 
> > > I'll probably touch on these in a followup patch.
> > Lazy initialization of identifiers is kinda perceived as a fairly worthless 
> > optimization. Hence `CallDescription`.
> > 
> > Also it cannot be done during registration because the AST is not 
> > constructed yet at this point. Well, probably it can be done anyway because 
> > the empty identifier table is already there in the `ASTContext` and we can 
> > always eagerly add a few items into it, but it still sounds scary.
> I would personally prefer to risk initializing these during registration, but 
> if we're this many comments into this discussion, then I believe it deserves 
> a separate patch.
> 
> I'll leave a `TODO:` in the code.
Or just go with `CallDescription`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54823/new/

https://reviews.llvm.org/D54823



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

[PATCH] D55269: [CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D55269#1320382 , @tra wrote:

> >> Let's start with fixing OpenMP's cmake files. Once it no longer insists on 
> >> specifying --cuda-path=/usr, and isUbuntu is in place, what is the 
> >> remaining failure that you see?
> > 
> > I disagree here: It's not OpenMP but CMake that's detecting the wrong path 
> > here. This is the place to fix it once and for all (and possibly get the 
> > shim package right in that process).
>
> It's somewhat orthogonal, IMO. I agree that it's not OpenMP's problem. Cmake 
> will fix it at some point in future, but, presumably, we want OpenMP 
> buildable *now*. Adding a temporary workaround for something that Cmake can't 
> handle now would make building OpenMP with clang somewhat easier which was 
> the end goal of this patch. In the end it's up to OpenMP maintainers what 
> they would want to do.


I don't know when I'll explore the cmake problem further.  If someone else 
decides to, please cc/subscribe me.

Thanks for the reviews.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55269/new/

https://reviews.llvm.org/D55269



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[Romanov Vlad via Phabricator via cfe-commits]

romanovvlad added a comment.

Yes it's 1, but after addrspacecast alignment becomes not struct A but 
alignment of the pointer


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55262/new/

https://reviews.llvm.org/D55262



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

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

NoQ wrote:
> Szelethus wrote:
> > MTC wrote:
> > > Szelethus wrote:
> > > > MTC wrote:
> > > > > If I not wrong, `MemFunctionInfo` is mainly a class member of 
> > > > > `MallocChecker` that hold a bunch of memory function identifiers and 
> > > > > provide corresponding helper APIs. And we should pick a proper time 
> > > > > to initialize it.
> > > > > 
> > > > > I want to known why you call `initIdentifierInfo()` in 
> > > > > `checkPostStmt(const CallExpr *E,)`, this callback is called after 
> > > > > `checkPreCall(const CallEvent , )` in which we need 
> > > > > `MemFunctionInfo`.
> > > > Well, I'd like to know too! I think lazy initializing this struct 
> > > > creates more problems that it solves, but I wanted to draw a line 
> > > > somewhere in terms of how invasive I'd like to make this patch.
> > > How about put the initialization stuff into constructor? Let the 
> > > constructor do the initialization that it should do, let `register*()` do 
> > > its registration, like setting `setOptimism` and so on.
> > Interestingly, `MemFunctionInfo` is not always initialized, so a 
> > performance argument can be made here. I specifically checked whether 
> > there's any point in doing this hackery, and the answer is... well, some. 
> > I'll probably touch on these in a followup patch.
> Lazy initialization of identifiers is kinda perceived as a fairly worthless 
> optimization. Hence `CallDescription`.
> 
> Also it cannot be done during registration because the AST is not constructed 
> yet at this point. Well, probably it can be done anyway because the empty 
> identifier table is already there in the `ASTContext` and we can always 
> eagerly add a few items into it, but it still sounds scary.
I would personally prefer to risk initializing these during registration, but 
if we're this many comments into this discussion, then I believe it deserves a 
separate patch.

I'll leave a `TODO:` in the code.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
 

Szelethus wrote:
> MTC wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > I'm not sure `hasNonTrivialConstructorCall()` is a good name although 
> > > > you explained it in details in the comments below. And the comments for 
> > > > this function is difficult for me to understand, which is maybe my 
> > > > problem. 
> > > > 
> > > > And I also think this function doesn't do as much as described in the 
> > > > comments, it is just `true if the invoked constructor in \p NE has a 
> > > > parameter - pointer or reference to a record`, right?
> > > I admit that I copy-pasted this from the source I provided below, and I 
> > > overlooked it before posting it here. I //very// much prefer what you 
> > > proposed :)
> > The comments you provided is from the summary of the patch [[ 
> > https://reviews.llvm.org/D4025 | D4025 ]], ayartsev has not updated the 
> > summary information in time to adapt to his patch, so I think it is 
> > appropriate to extract the summary information when he submitted this 
> > patch, see [[ 
> > https://github.com/llvm-mirror/clang/commit/2e61d2893934f7b91ca9e2f889a4e4cc9939b05c#diff-ff9160d4628cb9b6186559837c2c8668
> >  | [Analyzer] fix for PR19102 ]]. What do you think?
> Wow, nice detective work there :D Sounds good to me.
Umm, yea, I really couldn't come up with a better name. Hopefully the comments 
both here and at the call site help on this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54823/new/

https://reviews.llvm.org/D54823



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D55262#1321048 , @romanovvlad wrote:

> I'm getting the same IR for both examples you provided. And for both load of 
> X value is aligned on 4 bytes wo patch and on 1 byte with.


That's odd, because the natural type alignment for `A` ought to be 1, since 
it's a packed type.  What does the AST look like?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55262/new/

https://reviews.llvm.org/D55262



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

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177002.
Szelethus marked 32 inline comments as done.
Szelethus added a reviewer: MTC.
Szelethus added a comment.

Addressing inline comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54823/new/

https://reviews.llvm.org/D54823

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/malloc-annotations.c
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
 
Index: test/Analysis/malloc-annotations.c
===
--- test/Analysis/malloc-annotations.c
+++ test/Analysis/malloc-annotations.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc -analyzer-store=region -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize,unix.Malloc \
+// RUN:   -analyzer-config unix.Malloc:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -7,8 +7,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,28 +87,59 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily getAllocationFamily(
+const MemFunctionInfoTy , CheckerContext , const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream , CheckerContext