[OpenMP][clang] declare mapper: fix handling of nested types

[ro-i]

Fix a crash that happened during parsing of a "declare mapper" construct for a struct that contains an element for which we also declared a custom default mapper.

Note: the history of this issue is as follows:

• Initially, ParseOpenMPDeclareMapperDirective() called two functions for acting on the declare mapper directive: ActOnOpenMPDeclareMapperDirectiveStart() and ActOnOpenMPDeclareMapperDirectiveEnd(). These functions were called before/after entering/exiting the parsing scope. In https://reviews.llvm.org/D83261, these functions were merged to ActOnOpenMPDeclareMapperDirective(), which is called after exiting the parsing scope.
• https://reviews.llvm.org/D92195 added support for nested default mappers by adding processImplicitMapsWithDefaultMappers(). This function is called in ActOnOpenMPDeclareMapperDirective(). It is also called by ActOnOpenMPExecutableDirective(), which is by itself unrelated to my PR, but it's noteworthy that ActOnOpenMPExecutableDirective() is called by ParseOpenMPExecutableDirective() within the parsing scope. That's why processImplicitMapsWithDefaultMappers() can add clauses without problems there. For ActOnOpenMPDeclareMapperDirective(), my PR fixes the problem that clauses cannot be added without parsing scope. Otherwise, the DSAStack is empty due to no current directive present, which leads to a crash of the compiler.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Robert Imschweiler (ro-i)

Changes

Fix a crash that happened during parsing of a "declare mapper" construct for a struct that contains an element for which we also declared a custom default mapper.

Note: the history of this issue is as follows:

• Initially, ParseOpenMPDeclareMapperDirective() called two functions for acting on the declare mapper directive: ActOnOpenMPDeclareMapperDirectiveStart() and ActOnOpenMPDeclareMapperDirectiveEnd(). These functions were called before/after entering/exiting the parsing scope. In https://reviews.llvm.org/D83261, these functions were merged to ActOnOpenMPDeclareMapperDirective(), which is called after exiting the parsing scope.
• https://reviews.llvm.org/D92195 added support for nested default mappers by adding processImplicitMapsWithDefaultMappers(). This function is called in ActOnOpenMPDeclareMapperDirective(). It is also called by ActOnOpenMPExecutableDirective(), which is by itself unrelated to my PR, but it's noteworthy that ActOnOpenMPExecutableDirective() is called by ParseOpenMPExecutableDirective() within the parsing scope. That's why processImplicitMapsWithDefaultMappers() can add clauses without problems there. For ActOnOpenMPDeclareMapperDirective(), my PR fixes the problem that clauses cannot be added without parsing scope. Otherwise, the DSAStack is empty due to no current directive present, which leads to a crash of the compiler.

---

Full diff: https://github.com/llvm/llvm-project/pull/143504.diff

4 Files Affected:

• (modified) clang/include/clang/Sema/SemaOpenMP.h (+1-1)
• (modified) clang/lib/Parse/ParseOpenMP.cpp (+9-3)
• (modified) clang/test/OpenMP/declare_mapper_ast_print.c (+25)
• (modified) clang/test/OpenMP/declare_mapper_ast_print.cpp (+26)

diff --git a/clang/include/clang/Sema/SemaOpenMP.h b/clang/include/clang/Sema/SemaOpenMP.h
index 6498390fe96f7..145001dd1160b 100644
--- a/clang/include/clang/Sema/SemaOpenMP.h
+++ b/clang/include/clang/Sema/SemaOpenMP.h
@@ -283,7 +283,7 @@ class SemaOpenMP : public SemaBase {
   /// mapper' construct.
   QualType ActOnOpenMPDeclareMapperType(SourceLocation TyLoc,
                                         TypeResult ParsedType);
-  /// Called on start of '#pragma omp declare mapper'.
+  /// Called for '#pragma omp declare mapper'.
   DeclGroupPtrTy ActOnOpenMPDeclareMapperDirective(
       Scope *S, DeclContext *DC, DeclarationName Name, QualType MapperType,
       SourceLocation StartLoc, DeclarationName VN, AccessSpecifier AS,
diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp
index e41e5ba8596b9..bf6e69e32517f 100644
--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -576,6 +576,7 @@ Parser::ParseOpenMPDeclareMapperDirective(AccessSpecifier AS) {
     return DeclGroupPtrTy();
   }
 
+  Scope *OuterScope = getCurScope();
   // Enter scope.
   DeclarationNameInfo DirName;
   SourceLocation Loc = Tok.getLocation();
@@ -614,12 +615,17 @@ Parser::ParseOpenMPDeclareMapperDirective(AccessSpecifier AS) {
     IsCorrect = false;
   }
 
+  // This needs to be called within the scope because
+  // processImplicitMapsWithDefaultMappers may add clauses when analyzing nested
+  // types. The scope used for calling ActOnOpenMPDeclareMapperDirective,
+  // however, needs to be the outer one, otherwise declared mappers don't become
+  // visible.
+  DeclGroupPtrTy DG = Actions.OpenMP().ActOnOpenMPDeclareMapperDirective(
+      OuterScope, Actions.getCurLexicalContext(), MapperId, MapperType,
+      Range.getBegin(), VName, AS, MapperVarRef.get(), Clauses);
   // Exit scope.
   Actions.OpenMP().EndOpenMPDSABlock(nullptr);
   OMPDirectiveScope.Exit();
-  DeclGroupPtrTy DG = Actions.OpenMP().ActOnOpenMPDeclareMapperDirective(
-      getCurScope(), Actions.getCurLexicalContext(), MapperId, MapperType,
-      Range.getBegin(), VName, AS, MapperVarRef.get(), Clauses);
   if (!IsCorrect)
     return DeclGroupPtrTy();
 
diff --git a/clang/test/OpenMP/declare_mapper_ast_print.c b/clang/test/OpenMP/declare_mapper_ast_print.c
index 3c554a106fe49..bb83f23a0c18a 100644
--- a/clang/test/OpenMP/declare_mapper_ast_print.c
+++ b/clang/test/OpenMP/declare_mapper_ast_print.c
@@ -49,6 +49,23 @@ struct dat {
 #pragma omp declare mapper(struct dat d) map(to: d.d)
 // CHECK: #pragma omp declare mapper (default : struct dat d) map(to: d.d){{$}}
 
+// Verify that nested default mappers do not lead to a crash during parsing / sema.
+// CHECK: struct inner {
+struct inner {
+  int size;
+  int *data;
+};
+#pragma omp declare mapper(struct inner i) map(i, i.data[0 : i.size])
+// CHECK: #pragma omp declare mapper (default : struct inner i) map(tofrom: default::i,i.data[0:i.size]){{$}}
+
+// CHECK: struct outer {
+struct outer {
+  int a;
+  struct inner i;
+};
+#pragma omp declare mapper(struct outer o) map(o)
+// CHECK: #pragma omp declare mapper (default : struct outer o) map(tofrom: default::o) map(tofrom: o.i){{$}}
+
 // CHECK: int main(void) {
 int main(void) {
 #pragma omp declare mapper(id: struct vec v) map(v.len)
@@ -77,6 +94,14 @@ int main(void) {
 #pragma omp declare mapper(id1: struct vec vvec) map(iterator(it=0:vvec.len:2), tofrom:vvec.data[it])
 // OMP52: #pragma omp declare mapper (id1 : struct vec vvec) map(iterator(int it = 0:vvec.len:2),tofrom: vvec.data[it]);
 #endif
+
+  {
+    struct outer outer;
+#pragma omp target map(outer)
+// CHECK: #pragma omp target map(tofrom: outer)
+    { }
+  }
+
   return 0;
 }
 // CHECK: }
diff --git a/clang/test/OpenMP/declare_mapper_ast_print.cpp b/clang/test/OpenMP/declare_mapper_ast_print.cpp
index 422fa9981672e..9ca3412e3e3d9 100644
--- a/clang/test/OpenMP/declare_mapper_ast_print.cpp
+++ b/clang/test/OpenMP/declare_mapper_ast_print.cpp
@@ -34,6 +34,28 @@ class vecchild : public vec {
 // CHECK: }
 // CHECK: ;
 
+// Verify that nested default mappers do not lead to a crash during parsing / sema.
+// CHECK: namespace N2 {
+namespace N2
+{
+// CHECK: struct inner {
+struct inner {
+  int size;
+  int *data;
+};
+#pragma omp declare mapper(struct inner i) map(i, i.data[0 : i.size])
+// CHECK: #pragma omp declare mapper (default : struct inner i) map(tofrom: N2::default::i,i.data[0:i.size]){{$}}
+
+// CHECK: struct outer {
+struct outer {
+  int a;
+  struct inner i;
+};
+#pragma omp declare mapper(struct outer o) map(o)
+// CHECK: #pragma omp declare mapper (default : struct outer o) map(tofrom: N2::default::o) map(tofrom: o.i){{$}}
+} // namespace N2
+// CHECK: }
+
 template <class T>
 class dat {
 public:
@@ -122,6 +144,7 @@ T foo(T a) {
 int main() {
   N1::vec vv, vvv;
   N1::vecchild vc;
+  N2::outer outer;
   dat<double> dd;
 #pragma omp target map(mapper(N1::id) tofrom: vv) map(mapper(dat<double>::id) alloc: vvv)
 // CHECK: #pragma omp target map(mapper(N1::id),tofrom: vv) map(mapper(dat<double>::id),alloc: vvv)
@@ -132,6 +155,9 @@ int main() {
 #pragma omp target map(mapper(default) tofrom: dd)
 // CHECK: #pragma omp target map(mapper(default),tofrom: dd)
   { dd.d++; }
+#pragma omp target map(outer)
+// CHECK: #pragma omp target map(tofrom: outer)
+  { }
 
 #pragma omp target update to(mapper(N1::id) : vc)
 // CHECK: #pragma omp target update to(mapper(N1::id): vc)

[ro-i]

As far as I can see the test failures (TypeSanitizer-x86_64) are unrelated to this PR.

[ro-i]

Merged. Thanks for the review :)