NoQ 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;
+ });
----------------
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
