[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Alexey Bataev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365786: [OPENMP]Initial fix PR42392: Improve -Wuninitialized 
warnings for OpenMP… (authored by ABataev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64356?vs=209042=209222#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64356

Files:
  cfe/trunk/include/clang/AST/OpenMPClause.h
  cfe/trunk/include/clang/AST/StmtOpenMP.h
  cfe/trunk/lib/AST/OpenMPClause.cpp
  cfe/trunk/lib/Analysis/CFG.cpp
  cfe/trunk/lib/Analysis/UninitializedValues.cpp
  cfe/trunk/test/Analysis/cfg-openmp.cpp
  cfe/trunk/test/OpenMP/atomic_messages.c
  cfe/trunk/test/OpenMP/critical_messages.cpp
  cfe/trunk/test/OpenMP/distribute_parallel_for_messages.cpp
  cfe/trunk/test/OpenMP/distribute_parallel_for_simd_misc_messages.c
  cfe/trunk/test/OpenMP/distribute_simd_misc_messages.c
  cfe/trunk/test/OpenMP/for_misc_messages.c
  cfe/trunk/test/OpenMP/for_simd_misc_messages.c
  cfe/trunk/test/OpenMP/master_messages.cpp
  cfe/trunk/test/OpenMP/ordered_messages.cpp
  cfe/trunk/test/OpenMP/parallel_for_messages.cpp
  cfe/trunk/test/OpenMP/parallel_for_simd_messages.cpp
  cfe/trunk/test/OpenMP/parallel_messages.cpp
  cfe/trunk/test/OpenMP/parallel_sections_messages.cpp
  cfe/trunk/test/OpenMP/sections_misc_messages.c
  cfe/trunk/test/OpenMP/simd_misc_messages.c
  cfe/trunk/test/OpenMP/single_misc_messages.c
  cfe/trunk/test/OpenMP/target_depend_messages.cpp
  cfe/trunk/test/OpenMP/target_parallel_for_messages.cpp
  cfe/trunk/test/OpenMP/target_parallel_for_simd_messages.cpp
  cfe/trunk/test/OpenMP/target_parallel_messages.cpp
  cfe/trunk/test/OpenMP/target_simd_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_distribute_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_simd_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_distribute_simd_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_messages.cpp
  cfe/trunk/test/OpenMP/target_update_messages.cpp
  cfe/trunk/test/OpenMP/task_messages.cpp
  cfe/trunk/test/OpenMP/taskgroup_messages.cpp
  cfe/trunk/test/OpenMP/taskloop_misc_messages.c
  cfe/trunk/test/OpenMP/taskloop_simd_misc_messages.c
  cfe/trunk/test/OpenMP/teams_distribute_parallel_for_messages.cpp
  cfe/trunk/test/OpenMP/teams_distribute_parallel_for_simd_messages.cpp
  cfe/trunk/test/OpenMP/teams_distribute_simd_messages.cpp
  cfe/trunk/test/OpenMP/teams_messages.cpp

Index: cfe/trunk/include/clang/AST/StmtOpenMP.h
===
--- cfe/trunk/include/clang/AST/StmtOpenMP.h
+++ cfe/trunk/include/clang/AST/StmtOpenMP.h
@@ -87,6 +87,63 @@
   }
 
 public:
+  /// Iterates over expressions/statements used in the construct.
+  class used_clauses_child_iterator
+  : public llvm::iterator_adaptor_base<
+used_clauses_child_iterator, ArrayRef::iterator,
+std::forward_iterator_tag, Stmt *, ptrdiff_t, Stmt *, Stmt *> {
+ArrayRef::iterator End;
+OMPClause::child_iterator ChildI, ChildEnd;
+
+void MoveToNext() {
+  if (ChildI != ChildEnd)
+return;
+  while (this->I != End) {
+++this->I;
+if (this->I != End) {
+  ChildI = (*this->I)->used_children().begin();
+  ChildEnd = (*this->I)->used_children().end();
+  if (ChildI != ChildEnd)
+return;
+}
+  }
+}
+
+  public:
+explicit used_clauses_child_iterator(ArrayRef Clauses)
+: used_clauses_child_iterator::iterator_adaptor_base(Clauses.begin()),
+  End(Clauses.end()) {
+  if (this->I != End) {
+ChildI = (*this->I)->used_children().begin();
+ChildEnd = (*this->I)->used_children().end();
+MoveToNext();
+  }
+}
+Stmt *operator*() const { return *ChildI; }
+Stmt *operator->() const { return **this; }
+
+used_clauses_child_iterator ++() {
+  ++ChildI;
+  if (ChildI != ChildEnd)
+return *this;
+  if (this->I != End) {
+++this->I;
+if (this->I != End) {
+  ChildI = (*this->I)->used_children().begin();
+  ChildEnd = (*this->I)->used_children().end();
+}
+  }
+  MoveToNext();
+  return *this;
+}
+  };
+
+  static llvm::iterator_range
+  used_clauses_children(ArrayRef Clauses) {
+return {used_clauses_child_iterator(Clauses),
+used_clauses_child_iterator(llvm::makeArrayRef(Clauses.end(), 0))};
+  }
+
   /// Iterates over a filtered subrange of clauses applied to a
   /// directive.
   ///
Index: cfe/trunk/include/clang/AST/OpenMPClause.h
===
--- cfe/trunk/include/clang/AST/OpenMPClause.h
+++ cfe/trunk/include/clang/AST/OpenMPClause.h
@@ -90,6 +90,15 @@
 return 

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked an inline comment as done.
ABataev added a comment.

In D64356#1579154 , @NoQ wrote:

> Ugh, i forced a lot of boilerplate on you. Hope it was worth it >.<
>
> Thank you!~


No problems, thanks for the review!




Comment at: test/Analysis/cfg-openmp.cpp:3
+
+// CHECK:void xxx(int argc)
+// CHECK:[B1]

NoQ wrote:
> Those are quite readable because of very verbose pretty-prints but generally 
> i prefer to interleave code and `CHECK:` lines so that to easily see both on 
> one screen and know what corresponds to what.
Ok, I will do this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Artem Dergachev via Phabricator via cfe-commits]

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

Ugh, i forced a lot of boilerplate on you. Hope it was worth it >.<

Thank you!~




Comment at: test/Analysis/cfg-openmp.cpp:3
+
+// CHECK:void xxx(int argc)
+// CHECK:[B1]

Those are quite readable because of very verbose pretty-prints but generally i 
prefer to interleave code and `CHECK:` lines so that to easily see both on one 
screen and know what corresponds to what.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev updated this revision to Diff 209042.
ABataev added a comment.

Improved code.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356

Files:
  include/clang/AST/OpenMPClause.h
  include/clang/AST/StmtOpenMP.h
  lib/AST/OpenMPClause.cpp
  lib/Analysis/CFG.cpp
  lib/Analysis/UninitializedValues.cpp
  test/Analysis/cfg-openmp.cpp
  test/OpenMP/atomic_messages.c
  test/OpenMP/critical_messages.cpp
  test/OpenMP/distribute_parallel_for_messages.cpp
  test/OpenMP/distribute_parallel_for_simd_misc_messages.c
  test/OpenMP/distribute_simd_misc_messages.c
  test/OpenMP/for_misc_messages.c
  test/OpenMP/for_simd_misc_messages.c
  test/OpenMP/master_messages.cpp
  test/OpenMP/ordered_messages.cpp
  test/OpenMP/parallel_for_messages.cpp
  test/OpenMP/parallel_for_simd_messages.cpp
  test/OpenMP/parallel_messages.cpp
  test/OpenMP/parallel_sections_messages.cpp
  test/OpenMP/sections_misc_messages.c
  test/OpenMP/simd_misc_messages.c
  test/OpenMP/single_misc_messages.c
  test/OpenMP/target_depend_messages.cpp
  test/OpenMP/target_parallel_for_messages.cpp
  test/OpenMP/target_parallel_for_simd_messages.cpp
  test/OpenMP/target_parallel_messages.cpp
  test/OpenMP/target_simd_messages.cpp
  test/OpenMP/target_teams_distribute_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_simd_messages.cpp
  test/OpenMP/target_teams_distribute_simd_messages.cpp
  test/OpenMP/target_teams_messages.cpp
  test/OpenMP/target_update_messages.cpp
  test/OpenMP/task_messages.cpp
  test/OpenMP/taskgroup_messages.cpp
  test/OpenMP/taskloop_misc_messages.c
  test/OpenMP/taskloop_simd_misc_messages.c
  test/OpenMP/teams_distribute_parallel_for_messages.cpp
  test/OpenMP/teams_distribute_parallel_for_simd_messages.cpp
  test/OpenMP/teams_distribute_simd_messages.cpp
  test/OpenMP/teams_messages.cpp

Index: test/OpenMP/teams_messages.cpp
===
--- test/OpenMP/teams_messages.cpp
+++ test/OpenMP/teams_messages.cpp
@@ -2,6 +2,13 @@
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -std=c++11 -o - %s -Wuninitialized
 
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this warning}}
+#pragma omp target
+#pragma omp teams
+  argc = x; // expected-warning {{variable 'x' is uninitialized when used here}}
+}
+
 void foo() {
 }
 
Index: test/OpenMP/teams_distribute_simd_messages.cpp
===
--- test/OpenMP/teams_distribute_simd_messages.cpp
+++ test/OpenMP/teams_distribute_simd_messages.cpp
@@ -2,6 +2,14 @@
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -std=c++11 %s -Wuninitialized
 
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this warning}}
+#pragma omp target
+#pragma omp teams distribute simd
+  for (int i = 0; i < 10; ++i)
+argc = x; // expected-warning {{variable 'x' is uninitialized when used here}}
+}
+
 void foo() {
 }
 
Index: test/OpenMP/teams_distribute_parallel_for_simd_messages.cpp
===
--- test/OpenMP/teams_distribute_parallel_for_simd_messages.cpp
+++ test/OpenMP/teams_distribute_parallel_for_simd_messages.cpp
@@ -2,6 +2,14 @@
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -std=c++11 %s -Wuninitialized
 
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this warning}}
+#pragma omp target
+#pragma omp teams distribute parallel for simd
+  for (int i = 0; i < 10; ++i)
+argc = x; // expected-warning {{variable 'x' is uninitialized when used here}}
+}
+
 void foo() {
 }
 
Index: test/OpenMP/teams_distribute_parallel_for_messages.cpp
===
--- test/OpenMP/teams_distribute_parallel_for_messages.cpp
+++ test/OpenMP/teams_distribute_parallel_for_messages.cpp
@@ -2,6 +2,14 @@
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -std=c++11 %s -Wuninitialized
 
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this warning}}
+#pragma omp target
+#pragma omp teams distribute parallel for
+  for (int i = 0; i < 10; ++i)
+argc = x; // expected-warning {{variable 'x' is uninitialized when used here}}
+}
+
 void foo() {
 }
 
Index: test/OpenMP/taskloop_simd_misc_messages.c
===
--- test/OpenMP/taskloop_simd_misc_messages.c
+++ test/OpenMP/taskloop_simd_misc_messages.c
@@ -2,6 +2,13 @@
 
 // RUN: %clang_cc1 -fsyntax-only -fopenmp-simd -triple x86_64-unknown-unknown -verify %s -Wuninitialized
 
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this warning}}
+#pragma omp taskloop simd
+  for (int i = 0; i < 10; ++i)
+argc = x; // expected-warning {{variable 'x' is uninitialized when used here}}
+}
+
 // 

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yay, great!


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D64356#1576813 , @NoQ wrote:

> Something like this:
>
> F9509417: photo_2019-07-09_12-21-46.jpg 
>
> Like, you can construct a CFG for an arbitrary statement. CFG₁ is the CFG for 
> xxx() and CFG₂ is the CFG for the CapturedStmt (and its children). I'm trying 
> to say that even if used expressions are duplicated in the AST, they should 
> not be duplicated in our CFGs.
>
> But that's more, like, for the future patches. I'll be happy to accept this 
> patch once you add CFG tests :)


I don't expect such duplication here. Those expressions associated with the 
directive have no relation with the CapturedStmt. They are associated only with 
the directive itself through the OpenMP clauses.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Something like this:

F9509417: photo_2019-07-09_12-21-46.jpg 

Like, you can construct a CFG for an arbitrary statement. CFG₁ is the CFG for 
xxx() and CFG₂ is the CFG for the CapturedStmt (and its children). I'm trying 
to say that even if used expressions are duplicated in the AST, they should not 
be duplicated in our CFGs.

But that's more, like, for the future patches. I'll be happy to accept this 
patch once you add CFG tests :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked an inline comment as done.
ABataev added a comment.

In D64356#1574981 , @NoQ wrote:

> Ok, so i think i more or less understand where this is going and i like it! 
> My only concern about making sure that used-expressions don't appear in both 
> CFGs; and, even then, it's likely that i'm wrong.
>
> +@rsmith just in case he has any immediate thoughts on this.


What "both" CFGs do you mean?




Comment at: include/clang/AST/StmtOpenMP.h:292
+  /// reduction, linear and firstprivate clauses, etc.
+  void for_each_used_expr(llvm::function_ref Fn) const;
 };

NoQ wrote:
> This whole `X.for_each(λ)` idiom doesn't seem to be popular in LLVM; people 
> seem to prefer to write an iterator and then use the generic `for_each(X, λ)` 
> over it.
> 
> (i don't really care)
I can try to add the iterator instead.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked 3 inline comments as done.
ABataev added inline comments.



Comment at: lib/Analysis/CFG.cpp:4746-4750
+  D->for_each_used_expr([this, ](Expr *E) {
+assert(E && "Expected expression.");
+if (CFGBlock *R = Visit(E))
+  B = R;
+  });

NoQ wrote:
> ABataev wrote:
> > NoQ wrote:
> > > Not sure, are these expressions actually evaluated in run-time when the 
> > > directive is hit? When i looked at a few AST dumps it seemed to me as if 
> > > these are just a few `DeclRefExpr`s that refer back to the captured 
> > > variables; they don't really do anything.
> > Some of them are real expressions, evaluated at the runtime. 
> > 
> > Some of them are the captured variables. But for some of those vars we need 
> > to create private copies, initialized with the values of the original 
> > variables. And we need to diagnose that the original captured variable is 
> > used without being initialized.
> > I'm going to extend this function to check all required expressions later. 
> > This is just an initial patch to setup the basic required functionality.
> Aha, ok, got it. So these are kinda like function arguments that are 
> evaluated before the call, outside the call.
> 
> My concern is that the CFG for these expressions need to appear only once: 
> either as part of the current CFG or as part of the CFG of the "outlined" 
> statement, but not in both. Otherwise any sort of inter-"procedure"-al 
> analysis over such CFG would mistakenly think that these statements are 
> evaluated twice.
> Aha, ok, got it. So these are kinda like function arguments that are 
> evaluated before the call, outside the call.

Well, a kind of.

> My concern is that the CFG for these expressions need to appear only once: 
> either as part of the current CFG or as part of the CFG of the "outlined" 
> statement, but not in both. Otherwise any sort of inter-"procedure"-al 
> analysis over such CFG would mistakenly think that these statements are 
> evaluated twice.

Agree, but currently they do not appear at all. Those expressions in most cases 
are not immdiate children of the OpenMP directive, they are children of OpenMP 
clauses, associated with the directives. And the function iterates through all 
the clauses to build the CFG for such expressions.



Comment at: lib/Analysis/CFG.cpp:4756-4757
+addLocalScopeAndDtors(S);
+  if (CFGBlock *R = addStmt(S))
+B = R;
+}

NoQ wrote:
> What you're saying here is "the statement is going to be executed at the 
> current moment of time, exactly once". Just curious, how accurate is this?
> 
> Generally it's perfectly fine drop some effects of the statement (eg., 
> atomicity). I'm not sure about things like `#pragma omp parallel` that may 
> evaluate the statement more than once, but it's not entirely inaccurate to 
> assume that the statement is executed once (on some systems it may as well be 
> true).
Do you suggest to exclude the bodies of the directives from the analysis?



Comment at: test/OpenMP/atomic_messages.c:5-9
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this 
warning}}
+#pragma omp atomic read
+  argc = x; // expected-warning {{variable 'x' is uninitialized when used 
here}}
+}

NoQ wrote:
> Currently the CFG is as follows:
> ```lang=c++
> [B1]
>1: int x;
>2: argc
>3: x
>4: argc = x; // CapturedStmt
>5: #pragma omp atomic read 'argc = x;'
> ```
> I.e.,
> 1. Declare `x`;
> 2. Compute lvalue `argc`;
> 3. Compute lvalue `x`;
> 4. Draw the rest of the ~~owl~~ assignment operator. Ignore the whole 
> complexity with lvalue-to-rvalue casts that are expected to be there.
> 5. Suddenly realize that this whole thing was an OpenMP atomic read from the 
> start.
> 
> Step 4 actually looks pretty good to me, given that we treat `CapturedStmt` 
> as some sort of function call. I.e., we simply delegate the work of 
> evaluating the actual statement into an "outlined" function which has its 
> own, separate CFG that doesn't need to be squeezed into the current CFG. And 
> this other CFG is where we're going to have our implicit cast and stuff.
> 
> I don't understand the point of having a separate step 5. I just don't see 
> what extra work can be done here that wasn't already done on step 4. I'd 
> probably mildly prefer to have only one of those: either step 4 or (better 
> because it has more information) step 5. I don't mind having both though.
> 
> So i guess generally this CFG looks good to me. In particular, i can totally 
> work with it if i end up implementing OpenMP support in the Static Analyzer. 
> At the same time i'd probably be fine with a CFG that contains only steps 1 
> and 5. I need to see more examples :)
I'll investigate this and will add some CFG specific tests.


Repository:
  rC Clang

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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a subscriber: rsmith.
NoQ added a comment.

Ok, so i think i more or less understand where this is going and i like it! My 
only concern about making sure that used-expressions don't appear in both CFGs; 
and, even then, it's likely that i'm wrong.

+@rsmith just in case he has any immediate thoughts on this.




Comment at: include/clang/AST/StmtOpenMP.h:292
+  /// reduction, linear and firstprivate clauses, etc.
+  void for_each_used_expr(llvm::function_ref Fn) const;
 };

This whole `X.for_each(λ)` idiom doesn't seem to be popular in LLVM; people 
seem to prefer to write an iterator and then use the generic `for_each(X, λ)` 
over it.

(i don't really care)



Comment at: lib/Analysis/CFG.cpp:2063
 
+  if (Context->getLangOpts().OpenMP && isa(S))
+return VisitOMPExecutableDirective(cast(S), asc);

The first check looks redundant. I don't think it wins much performance either.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/Analysis/CFG.cpp:4746-4750
+  D->for_each_used_expr([this, ](Expr *E) {
+assert(E && "Expected expression.");
+if (CFGBlock *R = Visit(E))
+  B = R;
+  });

ABataev wrote:
> NoQ wrote:
> > Not sure, are these expressions actually evaluated in run-time when the 
> > directive is hit? When i looked at a few AST dumps it seemed to me as if 
> > these are just a few `DeclRefExpr`s that refer back to the captured 
> > variables; they don't really do anything.
> Some of them are real expressions, evaluated at the runtime. 
> 
> Some of them are the captured variables. But for some of those vars we need 
> to create private copies, initialized with the values of the original 
> variables. And we need to diagnose that the original captured variable is 
> used without being initialized.
> I'm going to extend this function to check all required expressions later. 
> This is just an initial patch to setup the basic required functionality.
Aha, ok, got it. So these are kinda like function arguments that are evaluated 
before the call, outside the call.

My concern is that the CFG for these expressions need to appear only once: 
either as part of the current CFG or as part of the CFG of the "outlined" 
statement, but not in both. Otherwise any sort of inter-"procedure"-al analysis 
over such CFG would mistakenly think that these statements are evaluated twice.



Comment at: lib/Analysis/CFG.cpp:4756-4757
+addLocalScopeAndDtors(S);
+  if (CFGBlock *R = addStmt(S))
+B = R;
+}

What you're saying here is "the statement is going to be executed at the 
current moment of time, exactly once". Just curious, how accurate is this?

Generally it's perfectly fine drop some effects of the statement (eg., 
atomicity). I'm not sure about things like `#pragma omp parallel` that may 
evaluate the statement more than once, but it's not entirely inaccurate to 
assume that the statement is executed once (on some systems it may as well be 
true).



Comment at: test/OpenMP/atomic_messages.c:5-9
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this 
warning}}
+#pragma omp atomic read
+  argc = x; // expected-warning {{variable 'x' is uninitialized when used 
here}}
+}

Currently the CFG is as follows:
```lang=c++
[B1]
   1: int x;
   2: argc
   3: x
   4: argc = x; // CapturedStmt
   5: #pragma omp atomic read 'argc = x;'
```
I.e.,
1. Declare `x`;
2. Compute lvalue `argc`;
3. Compute lvalue `x`;
4. Draw the rest of the ~~owl~~ assignment operator. Ignore the whole 
complexity with lvalue-to-rvalue casts that are expected to be there.
5. Suddenly realize that this whole thing was an OpenMP atomic read from the 
start.

Step 4 actually looks pretty good to me, given that we treat `CapturedStmt` as 
some sort of function call. I.e., we simply delegate the work of evaluating the 
actual statement into an "outlined" function which has its own, separate CFG 
that doesn't need to be squeezed into the current CFG. And this other CFG is 
where we're going to have our implicit cast and stuff.

I don't understand the point of having a separate step 5. I just don't see what 
extra work can be done here that wasn't already done on step 4. I'd probably 
mildly prefer to have only one of those: either step 4 or (better because it 
has more information) step 5. I don't mind having both though.

So i guess generally this CFG looks good to me. In particular, i can totally 
work with it if i end up implementing OpenMP support in the Static Analyzer. At 
the same time i'd probably be fine with a CFG that contains only steps 1 and 5. 
I need to see more examples :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D64356#1574625 , @ABataev wrote:

> That's strange. Maybe the patch applied incorrectly?


Whoops, i needed to pull rC365334 . The 
patch has applied cleanly "with fuzz 2", which meant "oh there's no 
-Wuninitialized in this run line but i guess that's fine".


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked an inline comment as done.
ABataev added a comment.

In D64356#1574540 , @NoQ wrote:

> I don't know much about OpenMP, but i guess i can help with the CFG a bit.


Thanks a lot!

> I strongly recommend adding a few direct tests for the CFG so that to check 
> that the order of statements and the overall topology of the CFG is as you 
> intended. Currently those tests are routed through the Static Analyzer 
> because nobody else seems to care; see `test/Analysis/cfg.cpp` as an example 
> (there are a few more such tests, grep for `DumpCFG`).

Thanks for the advice, will do.

> I tried to apply the patch and accidentally noticed that none of the newly 
> added tests actually pass for me. Is this the right patch? Or is it just me?

That's strange. Maybe the patch applied incorrectly?




Comment at: lib/Analysis/CFG.cpp:4746-4750
+  D->for_each_used_expr([this, ](Expr *E) {
+assert(E && "Expected expression.");
+if (CFGBlock *R = Visit(E))
+  B = R;
+  });

NoQ wrote:
> Not sure, are these expressions actually evaluated in run-time when the 
> directive is hit? When i looked at a few AST dumps it seemed to me as if 
> these are just a few `DeclRefExpr`s that refer back to the captured 
> variables; they don't really do anything.
Some of them are real expressions, evaluated at the runtime. 

Some of them are the captured variables. But for some of those vars we need to 
create private copies, initialized with the values of the original variables. 
And we need to diagnose that the original captured variable is used without 
being initialized.
I'm going to extend this function to check all required expressions later. This 
is just an initial patch to setup the basic required functionality.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I don't know much about OpenMP, but i guess i can help with the CFG a bit.

I strongly recommend adding a few direct tests for the CFG so that to check 
that the order of statements and the overall topology of the CFG is as you 
intended. Currently those tests are routed through the Static Analyzer because 
nobody else seems to care; see `test/Analysis/cfg.cpp` as an example (there are 
a few more such tests, grep for `DumpCFG`).

I tried to apply the patch and accidentally noticed that none of the newly 
added tests actually pass for me. Is this the right patch? Or is it just me?




Comment at: lib/Analysis/CFG.cpp:4746-4750
+  D->for_each_used_expr([this, ](Expr *E) {
+assert(E && "Expected expression.");
+if (CFGBlock *R = Visit(E))
+  B = R;
+  });

Not sure, are these expressions actually evaluated in run-time when the 
directive is hit? When i looked at a few AST dumps it seemed to me as if these 
are just a few `DeclRefExpr`s that refer back to the captured variables; they 
don't really do anything.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64356



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

[PATCH] D64356: [OPENMP]Initial fix PR42392: Improve -Wuninitialized warnings for OpenMP programs.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev created this revision.
ABataev added reviewers: NoQ, Szelethus, dcoughlin, xazax.hun, a.sidorin, 
george.karpenkov, szepet.
Herald added subscribers: jdoerfert, jfb, guansong, rnkovacs.
Herald added a project: clang.

Some OpenMP clauses rely on the values of the variables. If the variable
is not initialized and used in OpenMP clauses that depend on the
variables values, it should be reported that the uninitialized variable
is used in the OpenMP clause expression.
This patch adds initial processing for uninitialized variables in OpenMP
constructs. Currently, it checks for use of the uninitialized variables
in the structured blocks.


Repository:
  rC Clang

https://reviews.llvm.org/D64356

Files:
  include/clang/AST/StmtOpenMP.h
  lib/AST/StmtOpenMP.cpp
  lib/Analysis/CFG.cpp
  lib/Analysis/UninitializedValues.cpp
  test/OpenMP/atomic_messages.c
  test/OpenMP/critical_messages.cpp
  test/OpenMP/distribute_parallel_for_messages.cpp
  test/OpenMP/distribute_parallel_for_simd_misc_messages.c
  test/OpenMP/distribute_simd_misc_messages.c
  test/OpenMP/for_misc_messages.c
  test/OpenMP/for_simd_misc_messages.c
  test/OpenMP/master_messages.cpp
  test/OpenMP/ordered_messages.cpp
  test/OpenMP/parallel_for_messages.cpp
  test/OpenMP/parallel_for_simd_messages.cpp
  test/OpenMP/parallel_messages.cpp
  test/OpenMP/parallel_sections_messages.cpp
  test/OpenMP/sections_misc_messages.c
  test/OpenMP/simd_misc_messages.c
  test/OpenMP/single_misc_messages.c
  test/OpenMP/target_depend_messages.cpp
  test/OpenMP/target_parallel_for_messages.cpp
  test/OpenMP/target_parallel_for_simd_messages.cpp
  test/OpenMP/target_parallel_messages.cpp
  test/OpenMP/target_simd_messages.cpp
  test/OpenMP/target_teams_distribute_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_simd_messages.cpp
  test/OpenMP/target_teams_distribute_simd_messages.cpp
  test/OpenMP/target_teams_messages.cpp
  test/OpenMP/target_update_messages.cpp
  test/OpenMP/task_messages.cpp
  test/OpenMP/taskgroup_messages.cpp
  test/OpenMP/taskloop_misc_messages.c
  test/OpenMP/taskloop_simd_misc_messages.c
  test/OpenMP/teams_distribute_parallel_for_messages.cpp
  test/OpenMP/teams_distribute_parallel_for_simd_messages.cpp
  test/OpenMP/teams_distribute_simd_messages.cpp
  test/OpenMP/teams_messages.cpp

Index: test/OpenMP/teams_messages.cpp
===
--- test/OpenMP/teams_messages.cpp
+++ test/OpenMP/teams_messages.cpp
@@ -2,6 +2,13 @@
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -std=c++11 -o - %s -Wuninitialized
 
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this warning}}
+#pragma omp target
+#pragma omp teams
+  argc = x; // expected-warning {{variable 'x' is uninitialized when used here}}
+}
+
 void foo() {
 }
 
Index: test/OpenMP/teams_distribute_simd_messages.cpp
===
--- test/OpenMP/teams_distribute_simd_messages.cpp
+++ test/OpenMP/teams_distribute_simd_messages.cpp
@@ -2,6 +2,14 @@
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -std=c++11 %s -Wuninitialized
 
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this warning}}
+#pragma omp target
+#pragma omp teams distribute simd
+  for (int i = 0; i < 10; ++i)
+argc = x; // expected-warning {{variable 'x' is uninitialized when used here}}
+}
+
 void foo() {
 }
 
Index: test/OpenMP/teams_distribute_parallel_for_simd_messages.cpp
===
--- test/OpenMP/teams_distribute_parallel_for_simd_messages.cpp
+++ test/OpenMP/teams_distribute_parallel_for_simd_messages.cpp
@@ -2,6 +2,14 @@
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -std=c++11 %s -Wuninitialized
 
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this warning}}
+#pragma omp target
+#pragma omp teams distribute parallel for simd
+  for (int i = 0; i < 10; ++i)
+argc = x; // expected-warning {{variable 'x' is uninitialized when used here}}
+}
+
 void foo() {
 }
 
Index: test/OpenMP/teams_distribute_parallel_for_messages.cpp
===
--- test/OpenMP/teams_distribute_parallel_for_messages.cpp
+++ test/OpenMP/teams_distribute_parallel_for_messages.cpp
@@ -2,6 +2,14 @@
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -std=c++11 %s -Wuninitialized
 
+void xxx(int argc) {
+  int x; // expected-note {{initialize the variable 'x' to silence this warning}}
+#pragma omp target
+#pragma omp teams distribute parallel for
+  for (int i = 0; i < 10; ++i)
+argc = x; // expected-warning {{variable 'x' is uninitialized when used here}}
+}
+
 void foo() {
 }
 
Index: test/OpenMP/taskloop_simd_misc_messages.c
===
--- test/OpenMP/taskloop_simd_misc_messages.c