[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

2017-04-23 Thread Leslie Zhai via Phabricator via cfe-commits
xiangzhai updated this revision to Diff 96344.
xiangzhai added a comment.

Hi Artem,

Because `memcpy` checked NULL pointer dereference for `src`:

  state = checkNonNull(C, state, Source, srcVal);
  ...

so such testcase can not point out my fault:

  void f15 () { 
 
int *x = malloc(sizeof(int));   
 
memcpy(x, 0, sizeof(int)); // expected-warning {{Null pointer argument in 
call to memory copy function}}
int n = 1 / *x; 
 
free(x);
 
  }

And I have no idea how to copy `RetVal` to Mem `s`:

  if (StateSameSize) {  
  
SVal ConstVal = State->getSVal(Const, LCtx);
  
State = State->BindExpr(CE, LCtx, RetVal);  
  
// Actually bind the second argument value to the buffer.   
  
State = State->bindDefault(RetVal, ConstVal, LCtx); 
  
// FIXME: Copy to Mem   
  
const MemRegion *MR = RetVal.getAsRegion(); 
  
if (!MR)
  
  return;   
  
MR = MR->StripCasts();  
  
if (const TypedValueRegion *TVR = MR->getAs()) {  
  
  MemVal = SB.makeLazyCompoundVal(StoreRef(State->getStore(),   
  
  State->getStateManager().getStoreManager()), TVR);
  
  State = State->BindExpr(CE, LCtx, MemVal);
  
}   
  
C.addTransition(State); 
  
  }

Please give me some advice, thanks a lot!

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/null-deref-ps-region.c

Index: test/Analysis/null-deref-ps-region.c
===
--- test/Analysis/null-deref-ps-region.c
+++ test/Analysis/null-deref-ps-region.c
@@ -1,6 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -std=gnu99 -analyzer-store=region -verify %s
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,unix,alpha.unix -std=gnu99 -analyzer-store=region -verify %s
 
+#include "Inputs/system-header-simulator.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *memset(void *__s, int __c, size_t __n);
+void *malloc(size_t __size);
+void free(void *__ptr);
 
 // The store for 'a[1]' should not be removed mistakenly. SymbolicRegions may
 // also be live roots.
@@ -13,3 +18,69 @@
 i = *p; // no-warning
   }
 }
+
+void f15 () {
+  int *x = malloc(sizeof(int));
+  memcpy(x, 0, sizeof(int)); // expected-warning {{Null pointer argument in call to memory copy function}}
+  int n = 1 / *x;
+  free(x);
+}
+
+void foo() {
+  int *x = malloc(sizeof(int));
+  int *r = memset(x, 0, sizeof(int));
+  int n = 1 / *r; // expected-warning {{Division by zero}}
+  free(x);
+}
+
+void foo2() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, sizeof(int));
+  int n = 1 / *x;
+  free(x);
+}
+
+void bar() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, 1);
+  int n = 1 / *x; // no-warning
+  free(x);
+}
+
+void f531() {
+  int *x = 0;
+  memset(x, 0, 1); // expected-warning {{Null pointer argument in call to memory set function}}
+}
+
+void f61() {
+  char buf[13];
+  memset(buf, 0, 1); // no-warning
+}
+
+void f611() {
+  char *buf = (char *)malloc(13);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
+
+void f66() {
+  char buf[1];
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+}
+
+void f666() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+  free(buf);
+}
+
+void f77() {
+  char buf[1];
+  memset(buf, 0, sizeof(buf)); // no-warning
+}
+
+void f777() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -120,6 +120,7 @@
   void evalStdCopy(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void 

[PATCH] D32411: [libcxx] Provide #include_next alternative for MSVC

2017-04-23 Thread Andrey Khalyavin via Phabricator via cfe-commits
halyavin added a comment.

Here is how we solved this problem in our libc++ fork:

  #define _LIBCPP_UCRT_INCLUDE(x) <../ucrt/x>
  #define _LIBCPP_MSVC_INCLUDE(x) <../../VC/include/x>



  #ifdef _LIBCPP_COMPILER_MSVC
  #include _LIBCPP_UCRT_INCLUDE(wchar.h)
  #else
  #include_next 
  #endif

As far as I understand neither solution resolves the problem with original 
paths not excluded. Watch out for cycling dependencies created by MSVC includes 
using other includes which point right back to libc++ instead.

Also, there is a problem with errno macro. I solved it like this:

  #ifdef _LIBCPP_COMPILER_MSVC
  // errno is defined in several files so we can't use #ifndef errno here
  #ifdef errno
  // undefine errno to avoid substitution in errno.h include file name.
  #pragma push_macro("errno")
  #undef errno
  #include _LIBCPP_UCRT_INCLUDE(errno.h)
  #pragma pop_macro("errno")
  #else
  #include _LIBCPP_UCRT_INCLUDE(errno.h)
  #endif
  #else
  #include_next 
  #endif


https://reviews.llvm.org/D32411



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


[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

2017-04-23 Thread Erik Nyquist via Phabricator via cfe-commits
enyquist added inline comments.



Comment at: lib/Format/WhitespaceManager.cpp:431
+
+  // Special case for AlignTokens: for all other alignment cases,
+  // the current sequence is ended when a comma or a scope change

djasper wrote:
> enyquist wrote:
> > djasper wrote:
> > > I am not yet sure I understand this. How is this different from:
> > > 
> > >   $ clang-format test.cc -style="{AlignConsecutiveDeclarations: true}"
> > >   map m;
> > >   map  m;
> > >   int   a;
> > I'm not sure exactly what you mean. Do you mean, why do I need this special 
> > case to ignore scope changes and commas? This was the only way I could get 
> > it to work, AlignTokens was bailing out as soon as a paren or a comma 
> > inside parens was seen.
> Yes, that's what I mean. The example I am writing above works correctly and 
> structurally, the two should be the same, right? Maybe this is easier now 
> that you can skip over the parameter list because of the correct 
> MatchingParen?
Hmm... when I just set IgnoreScopeAndCommas to False, most of the tests fail 
(all the blocks with parens in them) seem to fail. Sample:

```
/home/enyquist/clang-llvm/llvm/tools/clang/unittests/Format/FormatTest.cpp:74: 
Failure
  Expected: Code.str()
  Which is: "#define a3\n#define  4\n#define ccc  (5)"
To be equal to: format(test::messUp(Code), Style)
  Which is: "#define a 3\n#define  4\n#define ccc (5)"
With diff:
@@ -1,3 +1,3 @@
-#define a3
+#define a 3
 #define  4
-#define ccc  (5)
+#define ccc (5)
```

Not sure exactly what's going on here, I'll look into it. Perhaps I don't need 
to worry about commas.


Repository:
  rL LLVM

https://reviews.llvm.org/D28462



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


[PATCH] D32412: analyze all kinds of expressions for integer overflow

2017-04-23 Thread Nick Lewycky via Phabricator via cfe-commits
nlewycky created this revision.

Remove clang::Sema::CheckForIntOverflow(E) by calling into 
E->EvaluateForOverflow instead. CheckForIntOverflow implemented a whitelist of 
top-level expressions to check, currently BinaryOperator and InitListExpr.

Two changes are made to avoid regression with the existing test suite.
test/SemaCXX/eval-sizeof-dependent-type.cpp has an example of a value-dependent 
InitListExpr. Handle value-dependent initializers in init list exprs.
test/Sema/integer-overflow.c tests that initializers are checked for overflow, 
which was conditionally disabled for performance. Allow checking for overflow 
of init list exprs in the performance check.


https://reviews.llvm.org/D32412

Files:
  lib/AST/ExprConstant.cpp
  lib/Sema/SemaChecking.cpp
  test/OpenMP/distribute_parallel_for_simd_aligned_messages.cpp
  test/OpenMP/distribute_simd_aligned_messages.cpp
  test/OpenMP/for_simd_aligned_messages.cpp
  test/OpenMP/parallel_for_simd_aligned_messages.cpp
  test/OpenMP/simd_aligned_messages.cpp
  test/OpenMP/target_parallel_for_simd_aligned_messages.cpp
  test/OpenMP/target_simd_aligned_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_simd_aligned_messages.cpp
  test/OpenMP/target_teams_distribute_simd_aligned_messages.cpp
  test/OpenMP/taskloop_simd_aligned_messages.cpp
  test/OpenMP/teams_distribute_parallel_for_simd_aligned_messages.cpp
  test/OpenMP/teams_distribute_simd_aligned_messages.cpp

Index: test/OpenMP/teams_distribute_simd_aligned_messages.cpp
===
--- test/OpenMP/teams_distribute_simd_aligned_messages.cpp
+++ test/OpenMP/teams_distribute_simd_aligned_messages.cpp
@@ -123,9 +123,8 @@
 template int foomain(I argc, C **argv) {
   I e(argc);
   I g(argc);
-  int i; // expected-note {{declared here}} expected-note {{'i' defined here}}
-  // expected-note@+2 {{declared here}}
-  // expected-note@+1 {{reference to 'i' is not a constant expression}}
+  int i; // expected-note {{'i' defined here}}
+  // expected-note@+1 {{declared here}}
   int  = i;
 
 #pragma omp target
Index: test/OpenMP/teams_distribute_parallel_for_simd_aligned_messages.cpp
===
--- test/OpenMP/teams_distribute_parallel_for_simd_aligned_messages.cpp
+++ test/OpenMP/teams_distribute_parallel_for_simd_aligned_messages.cpp
@@ -123,9 +123,8 @@
 template int foomain(I argc, C **argv) {
   I e(argc);
   I g(argc);
-  int i; // expected-note {{declared here}} expected-note {{'i' defined here}}
-  // expected-note@+2 {{declared here}}
-  // expected-note@+1 {{reference to 'i' is not a constant expression}}
+  int i; // expected-note {{'i' defined here}}
+  // expected-note@+1 {{declared here}}
   int  = i;
 
 #pragma omp target
Index: test/OpenMP/taskloop_simd_aligned_messages.cpp
===
--- test/OpenMP/taskloop_simd_aligned_messages.cpp
+++ test/OpenMP/taskloop_simd_aligned_messages.cpp
@@ -107,9 +107,8 @@
 template int foomain(I argc, C **argv) {
   I e(argc);
   I g(argc);
-  int i; // expected-note {{declared here}} expected-note {{'i' defined here}}
-  // expected-note@+2 {{declared here}}
-  // expected-note@+1 {{reference to 'i' is not a constant expression}}
+  int i; // expected-note {{'i' defined here}}
+  // expected-note@+1 {{declared here}}
   int  = i;
   #pragma omp taskloop simd aligned // expected-error {{expected '(' after 'aligned'}}
   for (I k = 0; k < argc; ++k) ++k;
Index: test/OpenMP/target_teams_distribute_simd_aligned_messages.cpp
===
--- test/OpenMP/target_teams_distribute_simd_aligned_messages.cpp
+++ test/OpenMP/target_teams_distribute_simd_aligned_messages.cpp
@@ -110,9 +110,8 @@
 template int foomain(I argc, C **argv) {
   I e(argc);
   I g(argc);
-  int i; // expected-note {{declared here}} expected-note {{'i' defined here}}
-  // expected-note@+2 {{declared here}}
-  // expected-note@+1 {{reference to 'i' is not a constant expression}}
+  int i; // expected-note {{'i' defined here}}
+  // expected-note@+1 {{declared here}}
   int  = i;
 
 #pragma omp target teams distribute simd aligned // expected-error {{expected '(' after 'aligned'}}
Index: test/OpenMP/target_teams_distribute_parallel_for_simd_aligned_messages.cpp
===
--- test/OpenMP/target_teams_distribute_parallel_for_simd_aligned_messages.cpp
+++ test/OpenMP/target_teams_distribute_parallel_for_simd_aligned_messages.cpp
@@ -110,9 +110,8 @@
 template int foomain(I argc, C **argv) {
   I e(argc);
   I g(argc);
-  int i; // expected-note {{declared here}} expected-note {{'i' defined here}}
-  // expected-note@+2 {{declared here}}
-  // expected-note@+1 {{reference to 'i' is not a constant expression}}
+  int i; // expected-note {{'i' defined here}}
+  // expected-note@+1 {{declared here}}
   int  = i;
 
 #pragma 

[PATCH] D32411: [libcxx] Provide #include_next alternative for MSVC

2017-04-23 Thread Ben Craig via Phabricator via cfe-commits
bcraig created this revision.
Herald added a subscriber: mgorny.

Visual Studio 2015 and 2017 don't implement include_next, so we'll use a 
combination of a computed include and a CMAKE input to make it work.  Also, 
retrofit all the existing invocations of #include_next that we could hit in a 
hypothetical MSVC build.

This relies on implementation defined behavior in the MSVC preprocessor.
See C11, 6.10.2.4 ("Source file inclusion") for the statement on implementation 
defined vs. undefined.


https://reviews.llvm.org/D32411

Files:
  CMakeLists.txt
  include/__config
  include/__config_site.in
  include/complex.h
  include/cstddef
  include/ctype.h
  include/errno.h
  include/float.h
  include/inttypes.h
  include/limits.h
  include/locale.h
  include/math.h
  include/setjmp.h
  include/stdbool.h
  include/stddef.h
  include/stdint.h
  include/stdio.h
  include/stdlib.h
  include/string.h
  include/wchar.h
  include/wctype.h

Index: include/wctype.h
===
--- include/wctype.h
+++ include/wctype.h
@@ -51,7 +51,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(wctype.h)
+#else
 #include_next 
+#endif
 
 #ifdef __cplusplus
 
Index: include/wchar.h
===
--- include/wchar.h
+++ include/wchar.h
@@ -14,7 +14,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(wchar.h)
+#else
 #include_next 
+#endif
 
 #elif !defined(_LIBCPP_WCHAR_H)
 #define _LIBCPP_WCHAR_H
@@ -116,7 +120,11 @@
 #define __CORRECT_ISO_CPP_WCHAR_H_PROTO
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(wchar.h)
+#else
 #include_next 
+#endif
 
 // Determine whether we have const-correct overloads for wcschr and friends.
 #if defined(_WCHAR_H_CPLUSPLUS_98_CONFORMANCE_)
Index: include/string.h
===
--- include/string.h
+++ include/string.h
@@ -58,7 +58,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(string.h)
+#else
 #include_next 
+#endif
 
 // MSVCRT, GNU libc and its derivates may already have the correct prototype in
 // . This macro can be defined by users if their C library provides
Index: include/stdlib.h
===
--- include/stdlib.h
+++ include/stdlib.h
@@ -14,7 +14,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(stdlib.h)
+#else
 #include_next 
+#endif
 
 #elif !defined(_LIBCPP_STDLIB_H)
 #define _LIBCPP_STDLIB_H
@@ -91,7 +95,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(stdlib.h)
+#else
 #include_next 
+#endif
 
 #ifdef __cplusplus
 
Index: include/stdio.h
===
--- include/stdio.h
+++ include/stdio.h
@@ -14,7 +14,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(stdio.h)
+#else
 #include_next 
+#endif
 
 #elif !defined(_LIBCPP_STDIO_H)
 #define _LIBCPP_STDIO_H
@@ -105,7 +109,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(stdio.h)
+#else
 #include_next 
+#endif
 
 #ifdef __cplusplus
 
Index: include/stdint.h
===
--- include/stdint.h
+++ include/stdint.h
@@ -116,6 +116,10 @@
 #   define __STDC_CONSTANT_MACROS
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(stdint.h)
+#else
 #include_next 
+#endif
 
 #endif  // _LIBCPP_STDINT_H
Index: include/stddef.h
===
--- include/stddef.h
+++ include/stddef.h
@@ -15,7 +15,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(stddef.h)
+#else
 #include_next 
+#endif
 
 #elif !defined(_LIBCPP_STDDEF_H)
 #define _LIBCPP_STDDEF_H
@@ -43,7 +47,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(stddef.h)
+#else
 #include_next 
+#endif
 
 #ifdef __cplusplus
 
Index: include/stdbool.h
===
--- include/stdbool.h
+++ include/stdbool.h
@@ -26,7 +26,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)
+#include _LIBCPP_INCLUDE_NEXT(stdbool.h)
+#else
 #include_next 
+#endif
 
 #ifdef __cplusplus
 #undef bool
Index: include/setjmp.h
===
--- include/setjmp.h
+++ include/setjmp.h
@@ -32,7 +32,11 @@
 #pragma GCC system_header
 #endif
 
+#if defined(_LIBCPP_HAS_NO_INCLUDE_NEXT)

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

2017-04-23 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments.



Comment at: lib/Format/WhitespaceManager.cpp:431
+
+  // Special case for AlignTokens: for all other alignment cases,
+  // the current sequence is ended when a comma or a scope change

enyquist wrote:
> djasper wrote:
> > I am not yet sure I understand this. How is this different from:
> > 
> >   $ clang-format test.cc -style="{AlignConsecutiveDeclarations: true}"
> >   map m;
> >   map  m;
> >   int   a;
> I'm not sure exactly what you mean. Do you mean, why do I need this special 
> case to ignore scope changes and commas? This was the only way I could get it 
> to work, AlignTokens was bailing out as soon as a paren or a comma inside 
> parens was seen.
Yes, that's what I mean. The example I am writing above works correctly and 
structurally, the two should be the same, right? Maybe this is easier now that 
you can skip over the parameter list because of the correct MatchingParen?


Repository:
  rL LLVM

https://reviews.llvm.org/D28462



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


[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

2017-04-23 Thread Erik Nyquist via Phabricator via cfe-commits
enyquist updated this revision to Diff 96333.
enyquist marked an inline comment as done.
enyquist added a comment.

Addressed comments


Repository:
  rL LLVM

https://reviews.llvm.org/D28462

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7520,8 +7520,104 @@
   verifyFormat("a or_eq 8;", Spaces);
 }
 
+TEST_F(FormatTest, AlignConsecutiveMacros) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignConsecutiveAssignments = true;
+  Style.AlignConsecutiveDeclarations = true;
+  Style.AlignConsecutiveMacros = false;
+
+  verifyFormat("#define a 3\n"
+   "#define  4\n"
+   "#define ccc (5)",
+   Style);
+
+  verifyFormat("#define f(x) (x * x)\n"
+   "#define fff(x, y, z) (x * y + z)\n"
+   "#define (x, y) (x - y)",
+   Style);
+
+  verifyFormat("#define foo(x, y) (x + y)\n"
+   "#define bar (5, 6)(2 + 2)",
+   Style);
+
+  verifyFormat("#define a 3\n"
+   "#define  4\n"
+   "#define ccc (5)\n"
+   "#define f(x) (x * x)\n"
+   "#define fff(x, y, z) (x * y + z)\n"
+   "#define (x, y) (x - y)",
+   Style);
+
+  Style.AlignConsecutiveMacros = true;
+  verifyFormat("#define a3\n"
+   "#define  4\n"
+   "#define ccc  (5)",
+   Style);
+
+  verifyFormat("#define f(x) (x * x)\n"
+   "#define fff(x, y, z) (x * y + z)\n"
+   "#define (x, y)   (x - y)",
+   Style);
+
+  verifyFormat("#define foo(x, y) (x + y)\n"
+   "#define bar   (5, 6)(2 + 2)",
+   Style);
+
+  verifyFormat("#define a3\n"
+   "#define  4\n"
+   "#define ccc  (5)\n"
+   "#define f(x) (x * x)\n"
+   "#define fff(x, y, z) (x * y + z)\n"
+   "#define (x, y)   (x - y)",
+   Style);
+
+  verifyFormat("#define a 5\n"
+   "#define foo(x, y) (x + y)\n"
+   "#define CCC   (6)\n"
+   "auto lambda = []() {\n"
+   "  auto  ii = 0;\n"
+   "  float j  = 0;\n"
+   "  return 0;\n"
+   "};\n"
+   "int   i  = 0;\n"
+   "float i2 = 0;\n"
+   "auto  v  = type{\n"
+   "i = 1,   //\n"
+   "(i = 2), //\n"
+   "i = 3//\n"
+   "};",
+   Style);
+
+  Style.AlignConsecutiveMacros = false;
+  Style.ColumnLimit = 20;
+
+  verifyFormat("#define a  \\\n"
+   "  \"aa\"\n"
+   "#define D  \\\n"
+   "  \"aa\" \\\n"
+   "  \"ccdde\"\n"
+   "#define B  \\\n"
+   "  \"Q\"  \\\n"
+   "  \"F\"  \\\n"
+   "  \"\"\n",
+   Style);
+
+  Style.AlignConsecutiveMacros = true;
+  verifyFormat("#define a  \\\n"
+   "  \"aa\"\n"
+   "#define D  \\\n"
+   "  \"aa\" \\\n"
+   "  \"ccdde\"\n"
+   "#define B  \\\n"
+   "  \"Q\"  \\\n"
+   "  \"F\"  \\\n"
+   "  \"\"\n",
+   Style);
+}
+
 TEST_F(FormatTest, AlignConsecutiveAssignments) {
   FormatStyle Alignment = getLLVMStyle();
+  Alignment.AlignConsecutiveMacros = true;
   Alignment.AlignConsecutiveAssignments = false;
   verifyFormat("int a = 5;\n"
"int oneTwoThree = 123;",
@@ -7704,6 +7800,7 @@
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
   FormatStyle Alignment = getLLVMStyle();
+  Alignment.AlignConsecutiveMacros = true;
   Alignment.AlignConsecutiveDeclarations = false;
   verifyFormat("float const a = 5;\n"
"int oneTwoThree = 123;",
@@ -8668,6 +8765,7 @@
   CHECK_PARSE_BOOL(AlignEscapedNewlinesLeft);
   CHECK_PARSE_BOOL(AlignOperands);
   CHECK_PARSE_BOOL(AlignTrailingComments);
+  CHECK_PARSE_BOOL(AlignConsecutiveMacros);
   CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
   CHECK_PARSE_BOOL(AlignConsecutiveDeclarations);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -165,6 +165,9 @@
   /// \c EscapedNewlineColumn for the first tokens or token parts in a line.
   void 

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

2017-04-23 Thread Erik Nyquist via Phabricator via cfe-commits
enyquist marked 2 inline comments as done.
enyquist added inline comments.



Comment at: lib/Format/WhitespaceManager.cpp:413
+
+  while (Param && !Param->is(tok::l_paren)) {
+if (!Param->is(tok::identifier) && !Param->is(tok::comma))

djasper wrote:
> enyquist wrote:
> > djasper wrote:
> > > enyquist wrote:
> > > > djasper wrote:
> > > > > I think you should be able to use Current.MatchingParen here.
> > > > Hmm, I couldn't make this work... I just replaced this line with
> > > > 
> > > > while (Param && Param != Current->MatchingParen)
> > > > 
> > > > But it must not be doing what I think it's doing, since it made some 
> > > > tests fail.
> > > > Mind you, my C-brain might be taking over here, please let me know if 
> > > > I'm using MatchingParen incorrectly
> > > You shouldn't need a while loop. Just:
> > > 
> > >   if (!Current->MatchingParen || !Current->MatchingParen->Previous)
> > > return false;
> > >   Param = Param->MatchingParen->Previous;
> > > 
> > > And with that, I think you can simplify all these methods a bit:
> > > 
> > >   static bool endsPPIdentifier(const FormatToken *Current) {
> > > if (!Current->Next || Current->Next->SpacesRequiredBefore == 0)
> > >   return false;
> > > 
> > > // If Current is a "(", skip past the parameter list.
> > > if (Current->is(tok::r_paren)) {
> > >   if (!Current->MatchingParen || !Current->MatchingParen->Previous)
> > > return false;
> > >   Current = Current->MatchingParen->Previous;
> > > }
> > > 
> > > const FormatToken *Keyword = Current->Previous;
> > > if (!Keyword || !Keyword->is(tok::pp_define))
> > >   return false;
> > > 
> > > return Current->is(tok::identifier);
> > >   }
> > > 
> > > Does that work? If so, I'd even suggest inlining this code directly into 
> > > the lambda instead of pulling out a function.
> > It seems that MatchingParen does not get set for parens surrounding a 
> > macro-function parameter list. So for now, a loop is needed. I was able to 
> > clean it up, though, and I've inlined the whole thing in the lambda.
> Fixed that in r300661. We really should not add more ways to parse parens, 
> especially not outside of unwrapped lines (e.g. in the whitespace manager). 
> That can lead to very bad situations for error recovering when the code is 
> incomplete.
Great, thanks for fixing. Your change works.



Comment at: lib/Format/WhitespaceManager.cpp:431
+
+  // Special case for AlignTokens: for all other alignment cases,
+  // the current sequence is ended when a comma or a scope change

djasper wrote:
> I am not yet sure I understand this. How is this different from:
> 
>   $ clang-format test.cc -style="{AlignConsecutiveDeclarations: true}"
>   map m;
>   map  m;
>   int   a;
I'm not sure exactly what you mean. Do you mean, why do I need this special 
case to ignore scope changes and commas? This was the only way I could get it 
to work, AlignTokens was bailing out as soon as a paren or a comma inside 
parens was seen.


Repository:
  rL LLVM

https://reviews.llvm.org/D28462



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


[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

2017-04-23 Thread Jakub Kuderski via Phabricator via cfe-commits
kuhar updated this revision to Diff 96332.
kuhar marked an inline comment as done.
kuhar added a comment.

Update modernize-use-emplace rst docs.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 template 
-pair make_pair(T1, T2) {
-  return pair();
+pair make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template 
@@ -274,18 +274,42 @@
 
 void testMakePair() {
   std::vector> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The example below illustrates the problem.
+  struct D {
+D(...) {}
+operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2));
+
+  struct X {
+X(std::pair) {}
+  };
+  std::vector x;
+  x.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(std::make_pair(1, 2));
+  // make_pair cannot be removed here, as X is not constructible with two ints.
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===
--- docs/clang-tidy/checks/modernize-use-emplace.rst
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -36,8 +36,7 @@
 
 std::vector> w;
 w.emplace_back(21, 37);
-// This will be fixed to w.emplace_back(21L, 37L); in next version
-w.emplace_back(std::make_pair(21L, 37L);
+w.emplace_back(21L, 37L);
 
 The other situation is when we pass arguments that will be converted to a type
 inside a container.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
   Finds possible inefficient vector operations in for loops that may cause
   unnecessary memory reallocations.
 
+- Improved `modernize-use-emplace
+  `_ check
+
+  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
+  into emplace_back(a, b).
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,6 +15,12 @@
 namespace tidy {
 namespace modernize {
 
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace
+
 static const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
@@ -80,18 +86,31 @@
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto makePair = ignoringImplicit(
+  callExpr(callee(expr(ignoringParenImpCasts(
+  declRefExpr(unless(hasExplicitTemplateArgs()),
+  to(functionDecl(hasName("::std::make_pair"
+  .bind("make_pair"));
+
+  // make_pair can return type convertible to 

[PATCH] D32410: change the way objcboxedexpr is handled

2017-04-23 Thread Nick Lewycky via Phabricator via cfe-commits
nlewycky created this revision.

Make ObjCBoxedExpr less of a special case, teach the expression evaluator to 
handle it in general, sometimes descending through to its subexpr. Remove the 
code that called CheckForIntOverflow from outside BuildObjCBoxedExpr, leaving 
its only caller CheckCompletedExpr.

To make the existing tests continue to work, we also need to whitelist 
ObjCBoxedExpr as a top-level expression in CheckForIntOverflow. Ultimately that 
we shouldn't need to whitelist, but one piece at a time.


https://reviews.llvm.org/D32410

Files:
  lib/AST/ExprConstant.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExprObjC.cpp


Index: lib/Sema/SemaExprObjC.cpp
===
--- lib/Sema/SemaExprObjC.cpp
+++ lib/Sema/SemaExprObjC.cpp
@@ -595,7 +595,6 @@
 break;
   }
 }
-CheckForIntOverflow(ValueExpr);
 // FIXME:  Do I need to do anything special with BoolTy expressions?
 
 // Look for the appropriate method within NSNumber.
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -9882,6 +9882,9 @@
 
 if (auto InitList = dyn_cast(E))
   Exprs.append(InitList->inits().begin(), InitList->inits().end());
+
+if (isa(E))
+  E->IgnoreParenCasts()->EvaluateForOverflow(Context);
   } while (!Exprs.empty());
 }
 
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -4466,6 +4466,8 @@
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryPlus(const UnaryOperator *E)
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
+{ return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitChooseExpr(const ChooseExpr *E)
 { return StmtVisitorTy::Visit(E->getChosenSubExpr()); }
   bool VisitGenericSelectionExpr(const GenericSelectionExpr *E)
@@ -5471,7 +5473,26 @@
   bool VisitObjCStringLiteral(const ObjCStringLiteral *E)
   { return Success(E); }
   bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
-  { return Success(E); }
+  {
+// Should we continue evaluation after determining the result value?
+switch (Info.EvalMode) {
+case EvalInfo::EM_PotentialConstantExpression:
+case EvalInfo::EM_PotentialConstantExpressionUnevaluated:
+case EvalInfo::EM_EvaluateForOverflow:
+case EvalInfo::EM_IgnoreSideEffects:
+  if (!E->getSubExpr()->isValueDependent()) {
+APValue Discard;
+Evaluate(Discard, Info, E->getSubExpr());
+  }
+
+case EvalInfo::EM_ConstantExpression:
+case EvalInfo::EM_ConstantExpressionUnevaluated:
+case EvalInfo::EM_ConstantFold:
+case EvalInfo::EM_OffsetFold:
+  return Success(E);
+}
+  llvm_unreachable("Missed EvalMode case");
+  }
   bool VisitAddrLabelExpr(const AddrLabelExpr *E)
   { return Success(E); }
   bool VisitCallExpr(const CallExpr *E);


Index: lib/Sema/SemaExprObjC.cpp
===
--- lib/Sema/SemaExprObjC.cpp
+++ lib/Sema/SemaExprObjC.cpp
@@ -595,7 +595,6 @@
 break;
   }
 }
-CheckForIntOverflow(ValueExpr);
 // FIXME:  Do I need to do anything special with BoolTy expressions?
 
 // Look for the appropriate method within NSNumber.
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -9882,6 +9882,9 @@
 
 if (auto InitList = dyn_cast(E))
   Exprs.append(InitList->inits().begin(), InitList->inits().end());
+
+if (isa(E))
+  E->IgnoreParenCasts()->EvaluateForOverflow(Context);
   } while (!Exprs.empty());
 }
 
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -4466,6 +4466,8 @@
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryPlus(const UnaryOperator *E)
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
+{ return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitChooseExpr(const ChooseExpr *E)
 { return StmtVisitorTy::Visit(E->getChosenSubExpr()); }
   bool VisitGenericSelectionExpr(const GenericSelectionExpr *E)
@@ -5471,7 +5473,26 @@
   bool VisitObjCStringLiteral(const ObjCStringLiteral *E)
   { return Success(E); }
   bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
-  { return Success(E); }
+  {
+// Should we continue evaluation after determining the result value?
+switch (Info.EvalMode) {
+case EvalInfo::EM_PotentialConstantExpression:
+case EvalInfo::EM_PotentialConstantExpressionUnevaluated:
+   

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

2017-04-23 Thread Jakub Kuderski via Phabricator via cfe-commits
kuhar marked 2 inline comments as done.
kuhar added inline comments.



Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:93
+  to(functionDecl(hasName("::std::make_pair"
+  
+  .bind("make_pair"));

Prazek wrote:
> JonasToth wrote:
> > is the new line here necessary? i think it looks better if the `.bind` is 
> > on this line.
> Better question is "is it clang formated?"
It was clang-formatted, but I do agree that it looked a bit weird. I removed 
the extra newline.



Comment at: test/clang-tidy/modernize-use-emplace.cpp:284
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));

Prazek wrote:
> I would add here test like:
> 
>   class X {
> X(std:;pair a) {}
>   };
>   
>   std::vector v;
>   v.push_back(make_pair(42, 42));
>   
> I guess as long as X ctor is not explicit this can happen, and we can't 
> transform it to
> emplace.back(42, 42)
Nice idea for a test case, added.


https://reviews.llvm.org/D32395



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


[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

2017-04-23 Thread Jakub Kuderski via Phabricator via cfe-commits
kuhar updated this revision to Diff 96329.
kuhar added a comment.

Add test for pair constructor argument, mention changes in release notes.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 template 
-pair make_pair(T1, T2) {
-  return pair();
+pair make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template 
@@ -274,18 +274,42 @@
 
 void testMakePair() {
   std::vector> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The example below illustrates the problem.
+  struct D {
+D(...) {}
+operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2));
+
+  struct X {
+X(std::pair) {}
+  };
+  std::vector x;
+  x.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(std::make_pair(1, 2));
+  // make_pair cannot be removed here, as X is not constructible with two ints.
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
   Finds possible inefficient vector operations in for loops that may cause
   unnecessary memory reallocations.
 
+- Improved `modernize-use-emplace
+  `_ check
+
+  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
+  into emplace_back(a, b).
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,6 +15,12 @@
 namespace tidy {
 namespace modernize {
 
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace
+
 static const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
@@ -80,18 +86,31 @@
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto makePair = ignoringImplicit(
+  callExpr(callee(expr(ignoringParenImpCasts(
+  declRefExpr(unless(hasExplicitTemplateArgs()),
+  to(functionDecl(hasName("::std::make_pair"
+  .bind("make_pair"));
+
+  // make_pair can return type convertible to container's element type.
+  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(makePair;
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(ctorAsArgument),
+  auto soughtParam = materializeTemporaryExpr(
+  anyOf(has(makePair), has(makePairCtor),
+hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+
+  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(soughtParam),
unless(isInTemplateInstantiation()))
  .bind("call"),
  this);
 }
 
 void 

[PATCH] D32401: [Devirtualization] insert placement new barrier with -O0

2017-04-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment.

I continue to be really uncomfortable with the entire design of this 
optimization, which appears to miscompile code by default, but as long as 
nobody's suggesting that we actually turn it on by default, I guess it can be 
your little research-compiler playground.  It might be better to downgrade it 
to a -cc1 option, though.

This specific change is fine by me.


https://reviews.llvm.org/D32401



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


[PATCH] D32401: [Devirtualization] insert placement new barrier with -O0

2017-04-23 Thread Piotr Padlewski via Phabricator via cfe-commits
Prazek added a comment.



In https://reviews.llvm.org/D32401#734870, @rjmccall wrote:

> Won't you have the exact same problem when LTO'ing with code that wasn't 
> compiled with strict v-table pointers?


No, because we fire error when combining module compiled with and without 
StrictVtablePointers

In https://reviews.llvm.org/D32401#734879, @mehdi_amini wrote:

> > To not break LTO with different optimizations levels, we should insert the 
> > barrier regardles of optimization level.
>
> What do you mean by "break"? Is it a correctness issue?


Yes, if we would combine module with barriers and without. 
You can check out this commit: https://reviews.llvm.org/D12580


https://reviews.llvm.org/D32401



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


[PATCH] D32341: Fix a bug that warnings generated with -M or -MM flags

2017-04-23 Thread Raphael Isemann via Phabricator via cfe-commits
teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

This also disables warnings for `-MD` and `-MMD` which shouldn't happen as 
we're actually compiling code here and are probably compiling a project. E.g. 
for people that use this mode to create dependency files while initially 
building (to figure out when to rebuild each object), this will effectively 
force `-w` to the whole build process which is not good.

I would just manually check for `-M` and `-MM` where we only run the 
preprocessor and where adding `-w` is safe. Also please add another test that 
we actually get the warnings for `-MD` and another one for `-MMD`.

Otherwise this look good.


https://reviews.llvm.org/D32341



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


r301138 - Fix typo in comment.

2017-04-23 Thread Nick Lewycky via cfe-commits
Author: nicholas
Date: Sun Apr 23 15:46:58 2017
New Revision: 301138

URL: http://llvm.org/viewvc/llvm-project?rev=301138=rev
Log:
Fix typo in comment.

Modified:
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=301138=301137=301138=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Sun Apr 23 15:46:58 2017
@@ -1406,7 +1406,7 @@ const internal::VariadicDynCastAllOfMatc
 ///
 /// Example: Given
 /// \code
-///   struct T {void func()};
+///   struct T {void func();};
 ///   T f();
 ///   void g(T);
 /// \endcode


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


[PATCH] D32406: [Coverage][Windows] Null pointer dereference in CodeGenPGO::skipRegionMappingForDecl (fixes PR32761)

2017-04-23 Thread Adam Folwarczny via Phabricator via cfe-commits
adamf created this revision.

In function CodeGenPGO::skipRegionMappingForDecl there is possible NULL pointer 
dereference on line:
auto Loc = D->getBody()->getLocStart();
Value returned by getBody may be NULL. 
In corresponding test it happens during processing the virtual destructor ~A.

(minor)
The variable SkipCoverageMapping in the same function is always false. We can 
remove it.


https://reviews.llvm.org/D32406

Files:
  lib/CodeGen/CodeGenPGO.cpp
  lib/CodeGen/CodeGenPGO.h
  test/CoverageMapping/empty-destructor.cpp


Index: test/CoverageMapping/empty-destructor.cpp
===
--- test/CoverageMapping/empty-destructor.cpp
+++ test/CoverageMapping/empty-destructor.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -cc1 -triple i686-pc-windows-msvc19.0.0 -emit-obj 
-fprofile-instrument=clang -std=c++14 -fdelayed-template-parsing 
-fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name 
empty-destructor.cpp -o - %s
+
+class A
+{
+public:
+  A();
+  virtual ~A();
+};
+
+class B : public A
+{
+public:
+  B(const A& base)
+: A(base)
+  {}
+};
+
+void Test()
+{
+  A a;
+  B b(a);
+}
+
Index: lib/CodeGen/CodeGenPGO.h
===
--- lib/CodeGen/CodeGenPGO.h
+++ lib/CodeGen/CodeGenPGO.h
@@ -40,14 +40,11 @@
   std::unique_ptr ProfRecord;
   std::vector RegionCounts;
   uint64_t CurrentRegionCount;
-  /// \brief A flag that is set to true when this function doesn't need
-  /// to have coverage mapping data.
-  bool SkipCoverageMapping;
 
 public:
   CodeGenPGO(CodeGenModule )
   : CGM(CGM), NumValueSites({{0}}), NumRegionCounters(0),
-FunctionHash(0), CurrentRegionCount(0), SkipCoverageMapping(false) {}
+FunctionHash(0), CurrentRegionCount(0) {}
 
   /// Whether or not we have PGO region data for the current function. This is
   /// false both when we have no data at all and when our data has been
Index: lib/CodeGen/CodeGenPGO.cpp
===
--- lib/CodeGen/CodeGenPGO.cpp
+++ lib/CodeGen/CodeGenPGO.cpp
@@ -666,7 +666,7 @@
 }
 
 bool CodeGenPGO::skipRegionMappingForDecl(const Decl *D) {
-  if (SkipCoverageMapping)
+  if (!D->hasBody())
 return true;
 
   // Don't map the functions in system headers.


Index: test/CoverageMapping/empty-destructor.cpp
===
--- test/CoverageMapping/empty-destructor.cpp
+++ test/CoverageMapping/empty-destructor.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -cc1 -triple i686-pc-windows-msvc19.0.0 -emit-obj -fprofile-instrument=clang -std=c++14 -fdelayed-template-parsing -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name empty-destructor.cpp -o - %s
+
+class A
+{
+public:
+  A();
+  virtual ~A();
+};
+
+class B : public A
+{
+public:
+  B(const A& base)
+: A(base)
+  {}
+};
+
+void Test()
+{
+  A a;
+  B b(a);
+}
+
Index: lib/CodeGen/CodeGenPGO.h
===
--- lib/CodeGen/CodeGenPGO.h
+++ lib/CodeGen/CodeGenPGO.h
@@ -40,14 +40,11 @@
   std::unique_ptr ProfRecord;
   std::vector RegionCounts;
   uint64_t CurrentRegionCount;
-  /// \brief A flag that is set to true when this function doesn't need
-  /// to have coverage mapping data.
-  bool SkipCoverageMapping;
 
 public:
   CodeGenPGO(CodeGenModule )
   : CGM(CGM), NumValueSites({{0}}), NumRegionCounters(0),
-FunctionHash(0), CurrentRegionCount(0), SkipCoverageMapping(false) {}
+FunctionHash(0), CurrentRegionCount(0) {}
 
   /// Whether or not we have PGO region data for the current function. This is
   /// false both when we have no data at all and when our data has been
Index: lib/CodeGen/CodeGenPGO.cpp
===
--- lib/CodeGen/CodeGenPGO.cpp
+++ lib/CodeGen/CodeGenPGO.cpp
@@ -666,7 +666,7 @@
 }
 
 bool CodeGenPGO::skipRegionMappingForDecl(const Decl *D) {
-  if (SkipCoverageMapping)
+  if (!D->hasBody())
 return true;
 
   // Don't map the functions in system headers.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D32401: [Devirtualization] insert placement new barrier with -O0

2017-04-23 Thread Mehdi AMINI via Phabricator via cfe-commits
mehdi_amini added a comment.

> To not break LTO with different optimizations levels, we should insert the 
> barrier regardles of optimization level.

What do you mean by "break"? Is it a correctness issue?


https://reviews.llvm.org/D32401



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


[PATCH] D32401: [Devirtualization] insert placement new barrier with -O0

2017-04-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment.

Won't you have the exact same problem when LTO'ing with code that wasn't 
compiled with strict v-table pointers?


https://reviews.llvm.org/D32401



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


[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

2017-04-23 Thread Mads Ravn via Phabricator via cfe-commits
madsravn added a comment.

In https://reviews.llvm.org/D30158#734810, @aaron.ballman wrote:

> This continues to LGTM; do you need someone to land the patch for you?


I will remove the extra newline and land the patch tomorrow. Thanks! :)


https://reviews.llvm.org/D30158



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


[libcxx] r301132 - Add missing acquire_load to call_once overload.

2017-04-23 Thread Justin Lebar via cfe-commits
Author: jlebar
Date: Sun Apr 23 11:58:48 2017
New Revision: 301132

URL: http://llvm.org/viewvc/llvm-project?rev=301132=rev
Log:
Add missing acquire_load to call_once overload.

Summary: Seemed to have been overlooked in D24028.

This bug was found and brought to my attention by Paul Wankadia.

Reviewers: kubamracek, EricWF, dvyukov

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

Modified:
libcxx/trunk/include/mutex

Modified: libcxx/trunk/include/mutex
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/mutex?rev=301132=301131=301132=diff
==
--- libcxx/trunk/include/mutex (original)
+++ libcxx/trunk/include/mutex Sun Apr 23 11:58:48 2017
@@ -685,7 +685,7 @@ inline _LIBCPP_INLINE_VISIBILITY
 void
 call_once(once_flag& __flag, const _Callable& __func)
 {
-if (__flag.__state_ != ~0ul)
+if (__libcpp_acquire_load(&__flag.__state_) != ~0ul)
 {
 __call_once_param __p(__func);
 __call_once(__flag.__state_, &__p, &__call_once_proxy);


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


[PATCH] D32401: [Devirtualization] insert placement new barrier with -O0

2017-04-23 Thread Piotr Padlewski via Phabricator via cfe-commits
Prazek created this revision.
Herald added a subscriber: mehdi_amini.

To not break LTO with different optimizations levels, we should insert
the barrier regardles of optimization level.


https://reviews.llvm.org/D32401

Files:
  lib/CodeGen/CGExprCXX.cpp


Index: lib/CodeGen/CGExprCXX.cpp
===
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -1658,8 +1658,9 @@
 
   // Passing pointer through invariant.group.barrier to avoid propagation of
   // vptrs information which may be included in previous type.
+  // To not break LTO with different optimizations levels, we do it regardless
+  // of optimization level.
   if (CGM.getCodeGenOpts().StrictVTablePointers &&
-  CGM.getCodeGenOpts().OptimizationLevel > 0 &&
   allocator->isReservedGlobalPlacementOperator())
 result = Address(Builder.CreateInvariantGroupBarrier(result.getPointer()),
  result.getAlignment());


Index: lib/CodeGen/CGExprCXX.cpp
===
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -1658,8 +1658,9 @@
 
   // Passing pointer through invariant.group.barrier to avoid propagation of
   // vptrs information which may be included in previous type.
+  // To not break LTO with different optimizations levels, we do it regardless
+  // of optimization level.
   if (CGM.getCodeGenOpts().StrictVTablePointers &&
-  CGM.getCodeGenOpts().OptimizationLevel > 0 &&
   allocator->isReservedGlobalPlacementOperator())
 result = Address(Builder.CreateInvariantGroupBarrier(result.getPointer()),
  result.getAlignment());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

2017-04-23 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment.

This continues to LGTM; do you need someone to land the patch for you?




Comment at: test/clang-tidy/modernize-replace-random-shuffle.cpp:40
+
+
+  std::shuffle(vec.begin(), vec.end());

Can remove the extra newline.


https://reviews.llvm.org/D30158



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


[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects

2017-04-23 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment.

In https://reviews.llvm.org/D31542#734455, @joshz wrote:

> Are there any further changes I should make, or is this good to submit now?
>
> Thanks!


This still LGTM, so it's good to submit. Do you still need someone to land the 
patch for you?


Repository:
  rL LLVM

https://reviews.llvm.org/D31542



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


[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons

2017-04-23 Thread Piotr Padlewski via Phabricator via cfe-commits
Prazek added a comment.

This is actually good catch, we also need to do it when inserting barrier in 
placement new. 
I think that for the ctors and dtors we can do it only with optimizations 
enabled, because if optimizations are not enabled then ctors and dtors won't 
have the invariant.group in stores.


https://reviews.llvm.org/D32378



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


[PATCH] D32181: Remove use of coverage-file flag

2017-04-23 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment.

Ping. (If I don't hear back, I will assume this is noncontroversial and go 
ahead and commit sometime next week.)


https://reviews.llvm.org/D32181



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


[PATCH] D31830: Emit invariant.group.barrier when using union field

2017-04-23 Thread Piotr Padlewski via Phabricator via cfe-commits
Prazek updated this revision to Diff 96312.
Prazek added a comment.

- Inserting barrier with -O0


https://reviews.llvm.org/D31830

Files:
  lib/CodeGen/CGExpr.cpp
  test/CodeGenCXX/strict-vtable-pointers.cpp

Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
 // RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
 // RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
 // RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,82 @@
 // CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
 // CHECK-CTORS-LABEL: {{^}}}
 
+struct A {
+  virtual void foo();
+};
+struct B : A {
+  virtual void foo();
+};
+
+union U {
+  A a;
+  B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+  a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+  // CHECK-NEW: call void @_Z9changeToBP1U(
+  changeToB(u);
+  // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+  g2(>b);
+  // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+  changeToA(u);
+  // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+  // call void @_Z2g2P1A(%struct.A* %a)
+  g2(>a);
+  // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+  A a;
+};
+
+struct Empty {};
+struct AnotherEmpty {
+  Empty e;
+};
+union NoVptrs {
+  int a;
+  AnotherEmpty empty;
+};
+void take(AnotherEmpty &);
+
+// CHECK-NEW-LABEL: noBarriers
+void noBarriers(NoVptrs ) {
+  // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: 42
+  noVptrs.a += 42;
+  // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: call void @_Z4takeR12AnotherEmpty(
+  take(noVptrs.empty);
+}
+
+union U2 {
+  HoldingVirtuals h;
+  int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 ) {
+  // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: 42
+  u.z += 42;
+  // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+  take(u.h);
+}
 
 /** DTORS **/
 // CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3513,6 +3513,22 @@
   return CGF.Builder.CreateStructGEP(base, idx, offset, field->getName());
 }
 
+static bool hasAnyVptr(const CXXRecordDecl *RD, const ASTContext ) {
+  if (RD->isDynamicClass())
+return true;
+
+  for (const FieldDecl *Field : RD->fields()) {
+QualType FieldType = Context.getBaseElementType(Field->getType());
+const RecordType *RT = FieldType->getAs();
+if (!RT)
+  continue;
+auto *FieldClassDecl = cast(RT->getDecl());
+if (hasAnyVptr(FieldClassDecl, Context))
+  return true;
+  }
+  return false;
+}
+
 LValue CodeGenFunction::EmitLValueForField(LValue base,
const FieldDecl *field) {
   AlignmentSource fieldAlignSource =
@@ -3552,6 +3568,14 @@
 assert(!type->isReferenceType() && "union has reference member");
 // TODO: handle path-aware TBAA for union.
 TBAAPath = false;
+
+const auto *RD = field->getType().getTypePtr()->getAsCXXRecordDecl();
+if (CGM.getCodeGenOpts().StrictVTablePointers &&
+RD && hasAnyVptr(RD, getContext()))
+  // Because unions can easily skip invariant.barriers, we need to add
+  // a barrier every time CXXRecord field with vptr is referenced.
+  addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+ addr.getAlignment());
   } else {
 // For structs, we GEP to the field that the record layout suggests.
 addr = emitAddrOfFieldStorage(*this, addr, field);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons

2017-04-23 Thread Piotr Padlewski via Phabricator via cfe-commits
Prazek updated this revision to Diff 96311.
Prazek marked an inline comment as done.
Prazek added a comment.

Addressing Richard's comment


https://reviews.llvm.org/D32378

Files:
  lib/CodeGen/CGExprScalar.cpp
  test/CodeGenCXX/strict-vtable-pointers.cpp

Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -257,6 +257,62 @@
   take(u.h);
 }
 
+// CHECK-NEW-LABEL: define void @_Z7comparev()
+void compare() {
+  A *a = new A;
+  a->foo();
+  // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+  A *b = new (a) B;
+
+  // CHECK-NEW: %[[a:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+  // CHECK-NEW: %[[b:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+  // CHECK-NEW: %cmp = icmp eq %struct.A* %[[a2]], %[[b2]]
+  if (a == b)
+b->foo();
+}
+
+// CHECK-NEW-LABEL: compare2
+bool compare2(A *a, A *a2) {
+  // CHECK-NEW: %[[a:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+  // CHECK-NEW: %[[b:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+  // CHECK-NEW: %cmp = icmp ult %struct.A* %[[a2]], %[[b2]]
+  return a < a2;
+}
+// CHECK-NEW-LABEL: compareIntPointers
+bool compareIntPointers(int *a, int *b) {
+  // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier
+  return a == b;
+}
+
+struct HoldingOtherVirtuals {
+  B b;
+};
+
+// There is no need to add barriers for comparision of pointer to classes
+// that are not dynamic.
+// CHECK-NEW-LABEL: compare5
+bool compare5(HoldingOtherVirtuals *a, HoldingOtherVirtuals *b) {
+  // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier
+  return a == b;
+}
+
+struct X;
+// We have to also introduce the barriers if comparing pointers to incomplete
+// objects
+// CHECK-NEW-LABEL: define zeroext i1 @_Z8compare4P1XS0_
+bool compare4(X *x, X *x2) {
+  // CHECK-NEW: %[[x:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: %[[xp:.*]] = bitcast i8* %[[x]] to %struct.X*
+  // CHECK-NEW: %[[x2:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: %[[x2p:.*]] = bitcast i8* %[[x2]] to %struct.X*
+  // CHECK-NEW: %cmp = icmp eq %struct.X* %[[xp]], %[[x2p]]
+  return x == x2;
+}
+
 /** DTORS **/
 // CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
 // CHECK-DTORS-NOT: call i8* @llvm.invariant.group.barrier(
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1698,7 +1698,7 @@
   }
 
   case CK_IntToOCLSampler:
-return CGF.CGM.createOpenCLIntToSamplerConversion(E, CGF);
+return CGF.CGM.createOpenCLIntToSamplerConversion(E, CGF);
 
   } // end of switch
 
@@ -3062,8 +3062,19 @@
   Result = Builder.CreateFCmp(FCmpOpc, LHS, RHS, "cmp");
 } else if (LHSTy->hasSignedIntegerRepresentation()) {
   Result = Builder.CreateICmp(SICmpOpc, LHS, RHS, "cmp");
-} else {
-  // Unsigned integers and pointers.
+} else { // Unsigned integers and pointers.
+  if (CGF.CGM.getCodeGenOpts().StrictVTablePointers) {
+// Based on comparisons of pointers to dynamic objects, the optimizer
+// can replace one pointer with another. This might result in
+// replacing pointer after barrier to pointer before barrier,
+// resulting in invalid devirtualization.
+if (auto *RD = LHSTy->getPointeeCXXRecordDecl())
+  if (!RD->isCompleteDefinition() || RD->isDynamicClass())
+LHS = Builder.CreateInvariantGroupBarrier(LHS);
+if (auto *RD = RHSTy->getPointeeCXXRecordDecl())
+  if (!RD->isCompleteDefinition() || RD->isDynamicClass())
+RHS = Builder.CreateInvariantGroupBarrier(RHS);
+  }
   Result = Builder.CreateICmp(UICmpOpc, LHS, RHS, "cmp");
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons

2017-04-23 Thread Piotr Padlewski via Phabricator via cfe-commits
Prazek marked an inline comment as done.
Prazek added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:3066-3067
+} else { // Unsigned integers and pointers.
+  if (CGF.CGM.getCodeGenOpts().StrictVTablePointers &&
+  CGF.CGM.getCodeGenOpts().OptimizationLevel > 0) {
+// Based on comparisons of pointers to dynamic objects, the optimizer

rsmith wrote:
> I think we need to do this regardless of optimization level -- if we LTO 
> together a -O0 translation unit with a -O2 translation unit, we still need 
> this protection for the comparisons in the -O0 TU.
> 
> (IIRC we chose to make -fstrict-vtable-pointers an IR-level ABI break, but we 
> shouldn't do the same thing for optimization level.)
sounds good


https://reviews.llvm.org/D32378



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


[clang-tools-extra] r301130 - clang-rename: fix formatting

2017-04-23 Thread Miklos Vajna via cfe-commits
Author: vmiklos
Date: Sun Apr 23 11:07:06 2017
New Revision: 301130

URL: http://llvm.org/viewvc/llvm-project?rev=301130=rev
Log:
clang-rename: fix formatting

As detected by clang-format.

Modified:
clang-tools-extra/trunk/clang-rename/RenamingAction.cpp
clang-tools-extra/trunk/clang-rename/USRLocFinder.cpp
clang-tools-extra/trunk/clang-rename/USRLocFinder.h

Modified: clang-tools-extra/trunk/clang-rename/RenamingAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-rename/RenamingAction.cpp?rev=301130=301129=301130=diff
==
--- clang-tools-extra/trunk/clang-rename/RenamingAction.cpp (original)
+++ clang-tools-extra/trunk/clang-rename/RenamingAction.cpp Sun Apr 23 11:07:06 
2017
@@ -88,7 +88,7 @@ private:
 // new name.
 //
 // FIXME: Merge with the above RenamingASTConsumer.
-class USRSymbolRenamer: public ASTConsumer {
+class USRSymbolRenamer : public ASTConsumer {
 public:
   USRSymbolRenamer(const std::vector ,
const std::vector ,
@@ -110,7 +110,7 @@ public:
 llvm::errs() << "Renaming failed in " << Replace.getFilePath()
  << "! " << llvm::toString(std::move(Err)) << "\n";
   }
-  }
+}
   }
 }
   }
@@ -127,8 +127,7 @@ std::unique_ptr RenamingAct
 }
 
 std::unique_ptr QualifiedRenamingAction::newASTConsumer() {
-  return llvm::make_unique(
-  NewNames, USRList, FileToReplaces);
+  return llvm::make_unique(NewNames, USRList, 
FileToReplaces);
 }
 
 } // namespace rename

Modified: clang-tools-extra/trunk/clang-rename/USRLocFinder.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-rename/USRLocFinder.cpp?rev=301130=301129=301130=diff
==
--- clang-tools-extra/trunk/clang-rename/USRLocFinder.cpp (original)
+++ clang-tools-extra/trunk/clang-rename/USRLocFinder.cpp Sun Apr 23 11:07:06 
2017
@@ -195,8 +195,7 @@ NestedNameSpecifier *GetNestedNameForTyp
 //
 // This class will traverse the AST and find every AST node whose USR is in the
 // given USRs' set.
-class RenameLocFinder
-: public RecursiveASTVisitor {
+class RenameLocFinder : public RecursiveASTVisitor {
 public:
   RenameLocFinder(llvm::ArrayRef USRs, ASTContext )
   : USRSet(USRs.begin(), USRs.end()), Context(Context) {}

Modified: clang-tools-extra/trunk/clang-rename/USRLocFinder.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-rename/USRLocFinder.h?rev=301130=301129=301130=diff
==
--- clang-tools-extra/trunk/clang-rename/USRLocFinder.h (original)
+++ clang-tools-extra/trunk/clang-rename/USRLocFinder.h Sun Apr 23 11:07:06 2017
@@ -17,9 +17,9 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_RENAME_USR_LOC_FINDER_H
 
 #include "clang/AST/AST.h"
-#include "llvm/ADT/StringRef.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "llvm/ADT/StringRef.h"
 #include 
 #include 
 


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


[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

2017-04-23 Thread Piotr Padlewski via Phabricator via cfe-commits
Prazek added inline comments.



Comment at: test/clang-tidy/modernize-use-emplace.cpp:284
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));

I would add here test like:

  class X {
X(std:;pair a) {}
  };
  
  std::vector v;
  v.push_back(make_pair(42, 42));
  
I guess as long as X ctor is not explicit this can happen, and we can't 
transform it to
emplace.back(42, 42)


https://reviews.llvm.org/D32395



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


[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

2017-04-23 Thread Mads Ravn via Phabricator via cfe-commits
madsravn updated this revision to Diff 96303.
madsravn added a comment.

Small changes to code due to comments.


https://reviews.llvm.org/D30158

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
  test/clang-tidy/modernize-replace-random-shuffle.cpp

Index: test/clang-tidy/modernize-replace-random-shuffle.cpp
===
--- test/clang-tidy/modernize-replace-random-shuffle.cpp
+++ test/clang-tidy/modernize-replace-random-shuffle.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy %s modernize-replace-random-shuffle %t -- -- -std=c++11
+
+//CHECK-FIXES: #include 
+
+namespace std {
+template  struct vec_iterator {
+  T *ptr;
+  vec_iterator operator++(int);
+};
+
+template  struct vector {
+  typedef vec_iterator iterator;
+
+  iterator begin();
+  iterator end();
+};
+
+template 
+void random_shuffle(FwIt begin, FwIt end);
+
+template 
+void random_shuffle(FwIt begin, FwIt end, randomFunc& randomfunc);
+
+template 
+void shuffle(FwIt begin, FwIt end);
+} // namespace std
+
+// Random Func
+int myrandom (int i) { return i;}
+
+using namespace std;
+
+int main() {
+  std::vector vec;
+
+  std::random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+
+  std::shuffle(vec.begin(), vec.end());
+
+  random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+  
+  std::random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' and an alternative random mechanism instead
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' and an alternative random mechanism instead
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  shuffle(vec.begin(), vec.end());
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
===
--- docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
+++ docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - modernize-replace-random-shuffle
+
+modernize-replace-random-shuffle
+
+
+This check will find occurrences of ``std::random_shuffle`` and replace it with ``std::shuffle``. In C++17 ``std::random_shuffle`` will no longer be available and thus we need to replace it.
+
+Below are two examples of what kind of occurrences will be found and two examples of what it will be replaced with.
+
+.. code-block:: c++
+
+  std::vector v;
+
+  // First example
+  std::random_shuffle(vec.begin(), vec.end());
+
+  // Second example
+  std::random_shuffle(vec.begin(), vec.end(), randomFun);
+
+Both of these examples will be replaced with:
+
+.. code-block:: c++
+
+  std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+The second example will also receive a warning that ``randomFunc`` is no longer supported in the same way as before so if the user wants the same functionality, the user will need to change the implementation of the ``randomFunc``.
+
+One thing to be aware of here is that ``std::random_device`` is quite expensive to initialize. So if you are using the code in a performance critical place, you probably want to initialize it elsewhere. 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -124,6 +124,7 @@
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-replace-auto-ptr
+   modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
modernize-use-auto
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -81,6 +81,11 @@
   Adds checks that implement the `High Integrity C++ Coding Standard `_ and other safety
   standards. Many checks are aliased to other modules.
 
+- New `modernize-replace-random-shuffle
+  

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

2017-04-23 Thread Piotr Padlewski via Phabricator via cfe-commits
Prazek added inline comments.



Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:93
+  to(functionDecl(hasName("::std::make_pair"
+  
+  .bind("make_pair"));

JonasToth wrote:
> is the new line here necessary? i think it looks better if the `.bind` is on 
> this line.
Better question is "is it clang formated?"



Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:97
+  // make_pair can return type convertible to container's element type.
+  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(makePair;

JonasToth wrote:
> here, on line 100 and 89: shouldnt the matchers be upper case since they are 
> variables? Iam unsure about that.
True, all the matchers should have upper case, but it would be better to send 
it in separate patch


https://reviews.llvm.org/D32395



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


[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

2017-04-23 Thread Jonas Toth via Phabricator via cfe-commits
JonasToth added a comment.

please note this enhancement in the ReleaseNotes.




Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:93
+  to(functionDecl(hasName("::std::make_pair"
+  
+  .bind("make_pair"));

is the new line here necessary? i think it looks better if the `.bind` is on 
this line.



Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:97
+  // make_pair can return type convertible to container's element type.
+  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(makePair;

here, on line 100 and 89: shouldnt the matchers be upper case since they are 
variables? Iam unsure about that.


https://reviews.llvm.org/D32395



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


[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

2017-04-23 Thread Jakub Kuderski via Phabricator via cfe-commits
kuhar created this revision.
kuhar added a project: clang-tools-extra.

When there is a push_back with a call to make_pair, turn it into emplace_back 
and remove the unnecessary make_pair call.

Eg.

  std::vector> v;
  v.push_back(std::make_pair(1, 2)); // --> v.emplace_back(1, 2);

make_pair doesn't get removed when explicit template parameters are provided, 
because of potential problems with type conversions.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 template 
-pair make_pair(T1, T2) {
-  return pair();
+pair make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template 
@@ -274,18 +274,33 @@
 
 void testMakePair() {
   std::vector> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The example below illustrates the problem.
+  struct D {
+D(...) {}
+operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2));
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,6 +15,12 @@
 namespace tidy {
 namespace modernize {
 
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace
+
 static const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
@@ -80,18 +86,32 @@
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto makePair = ignoringImplicit(
+  callExpr(callee(expr(ignoringParenImpCasts(
+  declRefExpr(unless(hasExplicitTemplateArgs()),
+  to(functionDecl(hasName("::std::make_pair"
+  
+  .bind("make_pair"));
+
+  // make_pair can return type convertible to container's element type.
+  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(makePair;
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(ctorAsArgument),
+  auto soughtParam = materializeTemporaryExpr(
+  anyOf(has(makePair), has(makePairCtor),
+hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+
+  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(soughtParam),
unless(isInTemplateInstantiation()))
  .bind("call"),
  this);
 }
 
 void UseEmplaceCheck::check(const MatchFinder::MatchResult ) {
   const auto *Call = Result.Nodes.getNodeAs("call");
   const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor");
+  const auto *MakePairCall = Result.Nodes.getNodeAs("make_pair");
+  assert(InnerCtorCall || MakePairCall);
 
   auto FunctionNameSourceRange = CharSourceRange::getCharRange(
   Call->getExprLoc(), Call->getArg(0)->getExprLoc());
@@ -101,22 +121,34 @@
   if (FunctionNameSourceRange.getBegin().isMacroID())
 return;
 
+  const auto *EmplacePrefix = MakePairCall ? "emplace_back" : "emplace_back(";
   Diag << 

[PATCH] D21303: [clang-tidy] Adds performance-returning-type check.

2017-04-23 Thread Jakub StaroĊ„ via Phabricator via cfe-commits
staronj abandoned this revision.
staronj marked 9 inline comments as done.
staronj added a comment.

Because of https://reviews.llvm.org/D21619 the check is no longer required.


https://reviews.llvm.org/D21303



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