[OpenMP] Fix problems with the declare variant append_args clause

[mikerice]

Summary

Use ASTContext::getTypeDeclType() to get type of omp_interop_t since
TypeDecl::getTypeForDecl() may return null if TypeForDecl is not
setup yet.

Handle functions where the function type is under an AttributedType.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

7068

Do we always expect omp_interop_t to be a typedef?

[mikerice]

clang/lib/Sema/SemaOpenMP.cpp

7068

I guess I do. The spec says it must be an integral or pointer type. Could it be something other than a TypedefNameDecl?

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

7068

Can we check instead if it is compatible with integral or pointer type, of we drop all typedefs, using, etc?

[mikerice]

clang/lib/Sema/SemaOpenMP.cpp

7068

Sorry I don't think I understand what you are requesting. TypedefNameDecl is the base class used for both typedefs and using decls so this would work for either. Unless there is some C++-only implementation the declaration in omp.h is likely to be a typedef though. I think we'd rather not drop typedefs here so diagnostics are clearer if this shows up in a diagnostic.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

7068

I suggest to desugar omp_interop_t type and use it’s main (integral or pointer) type to be fully compatible with any other runtime library inpelementation/OpenMP specification. And use this desugared type as a type of the extra parameter.

[mikerice]

clang/lib/Sema/SemaOpenMP.cpp

7068

I can do that but are you sure we should? What kind of sugar affects type compatibility? If we do this we get a diagnostic that no longer mentions omp_interop_t and the user will have a harder time understanding the situation.

Old diagnostic:
variant in '#pragma omp declare variant' with type 'void (A::*)(float *, float *, int *)' is incompatible with type 'void (A::*)(float *, float *, int *, omp_interop_t)' with appended arguments

New diagnostic:
variant in '#pragma omp declare variant' with type 'void (A::*)(float *, float *, int *)' is incompatible with type 'void (A::*)(float *, float *, int *, void *)' with appended arguments

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

7068

Ok, I see. And what's current behavior for C code?

[mikerice]

clang/lib/Sema/SemaOpenMP.cpp

7068

The problem that I am trying to fix is just that using TypeDecl::getTypeForDecl() sometimes returns null here crashing the compiler. It seems the ASTContext members should be used instead since they check and set TypeForDecl if needed.

This shows up in some C cases but not in a C++ compile of the same code. I guess there is some subtle difference in how typedefs are handled in C vs. C++.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

7068

Could you check please, why does it return nullptr?

[mikerice]

clang/lib/Sema/SemaOpenMP.cpp

7068

There is a difference in how name lookup is working in C and C++.

In C it is simple: lookup finds a single Decl and returns it without setting TypeForDecl.

In C++ two types of lookups occur and the typedef is found in both. This leaves two Decls that need to be resolved. During that processing ASTContext::getTypeDeclType is called and sets TypeForDecl (around line 528 of SemaLookup.cpp).

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

7068

Can we mimic same behavior for C only? I mean, keep existing code, but if it is C manually call ASTContext::getTypeDeclsForType?

[mikerice]

clang/lib/Sema/SemaOpenMP.cpp

7068

I think the right thing to do is just replace the getTypeForDecl call with a getTypeDeclType call. This will do the right thing after a C lookup and doesn't change the behavior for C++ (internally it will just return TypeForDecl). I'll upload a new patch to look at.

[mikerice]

Addressed review comments.

[ABataev]

LG

[aaron.ballman]

LGTM modulo a formatting nit with the test.

clang/lib/Sema/SemaOpenMP.cpp

7068

I think the right thing to do is just replace the getTypeForDecl call with a getTypeDeclType call.

I agree.

clang/test/OpenMP/declare_variant_clauses_ast_print.c

2

Can you add a whitespace after the comment marker (in the whole file)? Then my eyes won't bleed so badly.

[mikerice]

Added whitespace to the test.

clang/lib/Sema/SemaOpenMP.cpp

auto HandleNonConstantScoresAndConditions = [this](Expr *&E,
     }
     return true;
   };
   if (TI.anyScoreOrCondition(HandleNonConstantScoresAndConditions))
     return None;
 
   QualType AdjustedFnType = FD->getType();
   if (NumAppendArgs) {
-    if (isa<FunctionNoProtoType>(FD->getType())) {
+    const auto *PTy = AdjustedFnType->getAsAdjusted<FunctionProtoType>();
+    if (!PTy) {
       Diag(FD->getLocation(), diag::err_omp_declare_variant_prototype_required)
           << SR;
       return None;
     }
     // Adjust the function type to account for an extra omp_interop_t for each
     // specified in the append_args clause.
     const TypeDecl *TD = nullptr;
     LookupResult Result(*this, &Context.Idents.get("omp_interop_t"),
                         SR.getBegin(), Sema::LookupOrdinaryName);
     if (LookupName(Result, getCurScope())) {
       NamedDecl *ND = Result.getFoundDecl();
       TD = dyn_cast_or_null<TypeDecl>(ND);
     }
     if (!TD) {
       Diag(SR.getBegin(), diag::err_omp_interop_type_not_found) << SR;
       return None;
     }
-    QualType InteropType = QualType(TD->getTypeForDecl(), 0);
-    auto *PTy = cast<FunctionProtoType>(FD->getType());
+    QualType InteropType = Context.getTypeDeclType(TD);
     if (PTy->isVariadic()) {
       Diag(FD->getLocation(), diag::err_omp_append_args_with_varargs) << SR;
       return None;
     }
     llvm::SmallVector<QualType, 8> Params;
     Params.append(PTy->param_type_begin(), PTy->param_type_end());
     Params.insert(Params.end(), NumAppendArgs, InteropType);
     AdjustedFnType = Context.getFunctionType(PTy->getReturnType(), Params,

clang/test/OpenMP/declare_variant_clauses_ast_print.c

+//RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-version=51 \
+//RUN:   -Wno-source-uses-openmp -Wno-openmp-clauses                       \
+//RUN:   -ast-print -o - %s | FileCheck %s --check-prefix=PRINT
+
+//RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fopenmp -fopenmp-version=51 \
+//RUN:   -Wno-source-uses-openmp -Wno-openmp-clauses -DWIN -fms-compatibility \
+//RUN:   -ast-print -o - %s | FileCheck %s --check-prefixes=PRINT,PRINTW
+
+//RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-version=51 \
+//RUN:   -Wno-source-uses-openmp -Wno-openmp-clauses                       \
+//RUN:   -ast-dump -o - %s | FileCheck %s --check-prefix=DUMP
+
+//RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fopenmp -fopenmp-version=51 \
+//RUN:   -Wno-source-uses-openmp -Wno-openmp-clauses -DWIN -fms-compatibility \
+//RUN:   -ast-dump -o - %s | FileCheck %s --check-prefixes=DUMP,DUMPW
+
+typedef void *omp_interop_t;
+
+#ifdef WIN
+//DUMPW: FunctionDecl{{.*}}win_foov
+//PRINTW: void win_foov(int n, double *y, void *interop_obj);
+
+void win_foov(int n, double *y, void *interop_obj);
+
+//DUMPW: FunctionDecl{{.*}}win_foo
+//DUMPW: OMPDeclareVariantAttr
+//DUMPW-NEXT: DeclRefExpr{{.*}}win_foov
+//PRINTW: #pragma omp declare variant(win_foov) match(construct={dispatch}, device={arch(x86_64)}) append_args(interop(targetsync))
+//PRINTW: void win_foo(int n, double *y);
+
+#pragma omp declare variant (win_foov) \
+  match(construct={dispatch}, device={arch(x86_64)}) \
+  append_args(interop(targetsync))
+void _cdecl win_foo(int n, double *y);
+#endif // WIN
+
+//DUMP: FunctionDecl{{.*}}c_foov
+//PRINT: void c_foov(int n, double *y, void *interop_obj);
+
+void c_foov(int n, double *y, void *interop_obj);
+
+//DUMP: FunctionDecl{{.*}}c_foo
+//DUMP: OMPDeclareVariantAttr
+//DUMP-NEXT: DeclRefExpr{{.*}}c_foov
+//PRINT: #pragma omp declare variant(c_foov) match(construct={dispatch}, device={arch(x86_64)}) append_args(interop(targetsync))
+//PRINT: void c_foo(int n, double *y);
+
+#pragma omp declare variant (c_foov) \
+  match(construct={dispatch}, device={arch(x86_64)}) \
+  append_args(interop(targetsync))
+void c_foo(int n, double *y);

clang/test/OpenMP/declare_variant_clauses_messages.cpp

 // RUN: %clang_cc1 -verify -triple x86_64-unknown-linux -fopenmp -fopenmp-version=51 -std=c++11 -o - %s
 // RUN: %clang_cc1 -verify -triple x86_64-unknown-linux -fopenmp -fopenmp-version=51 -std=c++11 \
 // RUN:  -DNO_INTEROP_T_DEF -o - %s
 // RUN: %clang_cc1 -verify -triple x86_64-unknown-linux -fopenmp -fopenmp-version=50 -std=c++11 -o - %s
 // RUN: %clang_cc1 -verify -triple x86_64-unknown-linux -fopenmp -fopenmp-version=51 -DC -x c -o - %s
+// RUN: %clang_cc1 -verify -triple x86_64-pc-windows-msvc -fms-compatibility \
+// RUN:  -fopenmp -fopenmp-version=51 -DC -DWIN -x c -o - %s
 
 #ifdef NO_INTEROP_T_DEF
 void foo_v1(float *, void *);
 // expected-error@+1 {{'omp_interop_t' must be defined when 'append_args' clause is used; include <omp.h>}}
 #pragma omp declare variant(foo_v1) append_args(interop(target)) \
   match(construct={dispatch})
 void foo_v1(float *);
 #else

 
 void foo(float *AAA, float *BBB, int *I) { return; }
 
 #endif // NO_INTEROP_T_DEF
 
 #ifdef C
 void c_variant(omp_interop_t);
 // expected-error@+3 {{function with '#pragma omp declare variant' must have a prototype when 'append_args' is used}}
-#pragma omp declare variant(foo_v1)                            \
+#pragma omp declare variant(c_variant)                         \
    append_args(interop(target)) match(construct={dispatch})
 void c_base() {}
+#ifdef WIN
+void _cdecl win_c_variant(omp_interop_t);
+// expected-error@+3 {{function with '#pragma omp declare variant' must have a prototype when 'append_args' is used}}
+#pragma omp declare variant(win_c_variant)                     \
+   append_args(interop(target)) match(construct={dispatch})
+void _cdecl win_c_base() {}
+#endif // WIN
 #endif