[AST][OpenMP] OpenMP master Construct contains Structured block

[lebedev.ri]

Summary

Much like single construct (D57594):

[[ https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf | OpenMP Application Programming Interface Version 5.0 November 2018 ]], 2.16 master Construct, starting with page 221:

• The master construct specifies a structured block that is executed by the master thread of the team.
• Restrictions • A throw executed inside a master region must cause execution to resume within the same master region, and the same thread that threw the exception must catch it

[ABataev]

lib/Sema/SemaOpenMP.cpp

6058

Generally speaking, this is not required. It is required only for those constructs, that really needs an outlined function. Master construct does not create outlined function, so the "nothrow" flag is useless here.

[lebedev.ri]

@ABataev i'm not sure i have fully followed the https://bugs.llvm.org/show_bug.cgi?id=40563#c1

The outlined function is not generated for the loop, so there is no problem with the standard compatibility.

Are you saying that in these cases of master, critical, single directives, CapturedDecl should not have nothrow bit set too?

[ABataev]

In D57615#1381416, @lebedev.ri wrote:

@ABataev i'm not sure i have fully followed the https://bugs.llvm.org/show_bug.cgi?id=40563#c1

The outlined function is not generated for the loop, so there is no problem with the standard compatibility.

Are you saying that in these cases of master, critical, single directives, CapturedDecl should not have nothrow bit set too?

This flag just does not matter for them.

[lebedev.ri]

In D57615#1381418, @ABataev wrote:

In D57615#1381416, @lebedev.ri wrote:

@ABataev i'm not sure i have fully followed the https://bugs.llvm.org/show_bug.cgi?id=40563#c1

The outlined function is not generated for the loop, so there is no problem with the standard compatibility.

Are you saying that in these cases of master, critical, single directives, CapturedDecl should not have nothrow bit set too?

This flag just does not matter for them.

I'll rephrase:
Are you opposed to providing the correct (as per the specification) knowledge that
no exception will escape out of these CapturedDecl's, because that knowledge
does not matter for the existing sema/codegen, and does not affect produced IR?

[ABataev]

In D57615#1381427, @lebedev.ri wrote:

In D57615#1381418, @ABataev wrote:

In D57615#1381416, @lebedev.ri wrote:

@ABataev i'm not sure i have fully followed the https://bugs.llvm.org/show_bug.cgi?id=40563#c1

The outlined function is not generated for the loop, so there is no problem with the standard compatibility.

Are you saying that in these cases of master, critical, single directives, CapturedDecl should not have nothrow bit set too?

This flag just does not matter for them.

I'll rephrase:
Are you opposed to providing the correct (as per the specification) knowledge that
no exception will escape out of these CapturedDecl's, because that knowledge
does not matter for the existing sema/codegen, and does not affect produced IR?

Again, CapturedDecl and CapturedStmt is not the representation of the structured block. It is a helper structure for the codegen only. This flag does not help with anything, because for such OpenMP constructs the codegen bypasses CapturedStmt/CapturedDecl.
I'm not opposed. I'm saying, that it is not required and not used. If it is not used, why do you want to set it?

[lebedev.ri]

In D57615#1381428, @ABataev wrote:

In D57615#1381427, @lebedev.ri wrote:

In D57615#1381418, @ABataev wrote:

In D57615#1381416, @lebedev.ri wrote:

@ABataev i'm not sure i have fully followed the https://bugs.llvm.org/show_bug.cgi?id=40563#c1

The outlined function is not generated for the loop, so there is no problem with the standard compatibility.

Are you saying that in these cases of master, critical, single directives, CapturedDecl should not have nothrow bit set too?

This flag just does not matter for them.

I'll rephrase:
Are you opposed to providing the correct (as per the specification) knowledge that
no exception will escape out of these CapturedDecl's, because that knowledge
does not matter for the existing sema/codegen, and does not affect produced IR?

Again, CapturedDecl and CapturedStmt is not the representation of the structured block. It is a helper structure for the codegen only. This flag does not help with anything, because for such OpenMP constructs the codegen bypasses CapturedStmt/CapturedDecl.

Okay, enough is enough :)
I think a very important phrase was repeated a number of times, and it highlights the actual problem here:

is not the representation of the structured block

I'm not opposed. I'm saying, that it is not required and not used. If it is not used, why do you want to set it?

Then could you please revoke LG from D57594.

[lebedev.ri]

Let's instead solve the problem of structured block not having an AST representation.

[ABataev]

Okay, enough is enough :)
I think a very important phrase was repeated a number of times, and it highlights the actual problem here:

is not the representation of the structured block

It is not a problem! It is the internal implementation design! What do you want to get? Why do you need all these flags?

Then could you please revoke LG from D57594.

Done!

[ABataev]

In D57615#1381443, @lebedev.ri wrote:

Let's instead solve the problem of structured block not having an AST representation.

Again, this is not a problem for the compiler. If you want this idiom for the analyzer, you need to teach the analyzer to handle the AST nodes in accordance with the OpenMP standard.

[lebedev.ri]

In D57615#1381447, @ABataev wrote:

Okay, enough is enough :)
I think a very important phrase was repeated a number of times, and it highlights the actual problem here:

is not the representation of the structured block

It is not a problem! It is the internal implementation design! What do you want to get? Why do you need all these flags?

Uhm, i did state that in the commit message of rL352882 :

 Summary:
I'm working on a clang-tidy check, much like existing [[ http://clang.llvm.org/extra/clang-tidy/checks/bugprone-exception-escape.html | bugprone-exception-escape ]],
to detect when an exception might escape out of an OpenMP construct it isn't supposed to escape from.

and in https://bugs.llvm.org/show_bug.cgi?id=40563#c4 :

(In reply to Roman Lebedev from comment #4)

(In reply to Alexey Bataev from comment #3)

(In reply to Roman Lebedev from comment #2)

(In reply to Alexey Bataev from comment #1)
CapturedDecl is not the representation of the structured block. It used just
to simplify the parsing/sema/codegen. The outlined function is not generated
for the loop, so there is no problem with the standard compatibility.

Exactly. Perhaps i have poorly titled the bug.

The real question was formulated in the last phrase:

There needs to be some way to represent the fact that
only the body is nothrow.

What for? It does not help with anything. Standard does not require to
perform a thorough analysis of the structured blocks for the single
entry/single exit criteria. It just states that such OpenMP directives are
not compatible with the standard. What do you want to get with this?

Such syntactic sugar in AST potentially helps in the same way the very
existence
of well-defined AST helps - it is much easier to further operate on an
well-defined
AST, rather than on something less specified for that. (IR, asm, ...)

In this case, i'm going through the OP spec, through the every

A throw executed inside a ??? region must cause execution to resume within the
same iteration of the ??? region, and the same thread that threw the exception must
catch it.

note, and trying to verify that it is what clang does, either via
CapturedDecl's 'nothrow' tag,
or, like in this case, by filing an issue.

I want this info, so i can use this finely-structured info to do at least
partial
validation that these notes "no exception may escape" in the standard are
not violated.

In my case, it will be a clang-tidy check, because there is existing infra
for that.
A clang static analyzer check may appear later, or it may not.
(Though it would be especially interesting in the presence of cross-TU
analysis.)

Therefore i *insist* that it would be nice to have this info in the AST
:)

This is not a bug in a form "omg your implementation is so broken, fix it
immediately",
i'm simply using it to track the remaining issues. When i'm done with the
easy cases,
i'll try to cycle back here, and look how this could be solved.

Does that answer the question?

Then could you please revoke LG from D57594.

Done!

Thanks.

[ABataev]

In D57615#1381467, @lebedev.ri wrote:

In D57615#1381447, @ABataev wrote:

Okay, enough is enough :)
I think a very important phrase was repeated a number of times, and it highlights the actual problem here:

is not the representation of the structured block

It is not a problem! It is the internal implementation design! What do you want to get? Why do you need all these flags?

Uhm, i did state that in the commit message of rL352882 :

 Summary:
I'm working on a clang-tidy check, much like existing [[ http://clang.llvm.org/extra/clang-tidy/checks/bugprone-exception-escape.html | bugprone-exception-escape ]],
to detect when an exception might escape out of an OpenMP construct it isn't supposed to escape from.

and in https://bugs.llvm.org/show_bug.cgi?id=40563#c4 :

(In reply to Roman Lebedev from comment #4)

(In reply to Alexey Bataev from comment #3)

(In reply to Roman Lebedev from comment #2)

(In reply to Alexey Bataev from comment #1)
CapturedDecl is not the representation of the structured block. It used just
to simplify the parsing/sema/codegen. The outlined function is not generated
for the loop, so there is no problem with the standard compatibility.

Exactly. Perhaps i have poorly titled the bug.

The real question was formulated in the last phrase:

There needs to be some way to represent the fact that
only the body is nothrow.

What for? It does not help with anything. Standard does not require to
perform a thorough analysis of the structured blocks for the single
entry/single exit criteria. It just states that such OpenMP directives are
not compatible with the standard. What do you want to get with this?

Such syntactic sugar in AST potentially helps in the same way the very
existence
of well-defined AST helps - it is much easier to further operate on an
well-defined
AST, rather than on something less specified for that. (IR, asm, ...)

In this case, i'm going through the OP spec, through the every

A throw executed inside a ??? region must cause execution to resume within the
same iteration of the ??? region, and the same thread that threw the exception must
catch it.

note, and trying to verify that it is what clang does, either via
CapturedDecl's 'nothrow' tag,
or, like in this case, by filing an issue.

I want this info, so i can use this finely-structured info to do at least
partial
validation that these notes "no exception may escape" in the standard are
not violated.

In my case, it will be a clang-tidy check, because there is existing infra
for that.
A clang static analyzer check may appear later, or it may not.
(Though it would be especially interesting in the presence of cross-TU
analysis.)

Therefore i *insist* that it would be nice to have this info in the AST
:)

This is not a bug in a form "omg your implementation is so broken, fix it
immediately",
i'm simply using it to track the remaining issues. When i'm done with the
easy cases,
i'll try to cycle back here, and look how this could be solved.

Does that answer the question?

Then could you please revoke LG from D57594.

Done!

Thanks.

AST already has all the required information. You just need to teach your analyzer how to get it. The structured blocks are not represented by CapturedStmt/CapturedDecl, in many cases they are hidden inside of them. You just need to get it. But it does not mean that this a compiler problem.
For some constructs, you may have 2,3 or 4 captured statements, but the structured block is just 1 and it is hidden somewhere inside for better codegen support.
And you need to teach the analyzer how to get this data from the existing AST nodes.

[lebedev.ri]

In D57615#1381461, @ABataev wrote:

In D57615#1381443, @lebedev.ri wrote:

Let's instead solve the problem of structured block not having an AST representation.

Again, this is not a problem for the compiler. If you want this idiom for the analyzer, you need to teach the analyzer to handle the AST nodes in accordance with the OpenMP standard.

I'm sorry, but to me that just sounds completely wrong.

It sounds remotely like the gcc argument of keeping everything in
the least modular fashion just so no one does anything bad with it.

OpenMP spec gives a pretty good idea when something is an 'structured block'.
I'm specifically looking for these OpenMP structured blocks.
Are you telling me that i should forget everything i know about the 'goals' of clang AST,
(and i do believe it is reasonable to expect to have an OMPStructuredBlock opaque entry)
and reimplement the 'OpenMP structured blocks' finding in whatever code i'm writing,
instead of doing the right thing, and teaching the AST about it?

Obviously, said teaching should not have any negative impact on the existing clang OpenMP code.
Obviously, everyone's time is limited, and i won't be able to convince anyone to do that for me :)

I can see just how quickly i have derailed this disscussion, in less than 12h.
I guess i should stop commenting, and follow "words are cheap, show the code".

lib/Sema/SemaOpenMP.cpp

 StmtResult Sema::ActOnOpenMPMasterDirective(Stmt *AStmt,
                                             SourceLocation StartLoc,
                                             SourceLocation EndLoc) {
   if (!AStmt)
     return StmtError();
 
   assert(isa<CapturedStmt>(AStmt) && "Captured statement expected");
 
+  // 1.2.2 OpenMP Language Terminology
+  // Structured block - An executable statement with a single entry at the
+  // top and a single exit at the bottom.
+  // The point of exit cannot be a branch out of the structured block.
+  // longjmp() and throw() must not violate the entry/exit criteria.
+  cast<CapturedStmt>(AStmt)->getCapturedDecl()->setNothrow();
+
   setFunctionHasBranchProtectedScope();
 
   return OMPMasterDirective::Create(Context, StartLoc, EndLoc, AStmt);
 }
 
 StmtResult Sema::ActOnOpenMPCriticalDirective(
     const DeclarationNameInfo &DirName, ArrayRef<OMPClause *> Clauses,
     Stmt *AStmt, SourceLocation StartLoc, SourceLocation EndLoc) {

test/AST/ast-dump-openmp-master.cpp

+// RUN: %clang_cc1 -verify -fopenmp -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp-simd -ast-dump %s | FileCheck %s
+// expected-no-diagnostics
+
+void master() {
+#pragma omp master
+  {
+  }
+}
+
+// CHECK: `-FunctionDecl
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT:     `-OMPMasterDirective
+// CHECK-NEXT:       `-CapturedStmt
+// CHECK-NEXT:         `-CapturedDecl {{.*}} nothrow
+// CHECK-NEXT:           |-CompoundStmt
+// CHECK-NEXT:           `-ImplicitParamDecl