[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, &B](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/

https://reviews.llvm.org/D64356