[OPENMP]Referencing a static function defined in declare variant is generating an erroneous warning.

[zahiraam]

Summary

When a static function is defined inside an OpenMP clause, this warning “function 'foo' has internal linkage but is not defined”. See https://godbolt.org/z/ajKPc36M7
But this warning shouldn’t be issued because the declared variant function is called.
The warning should be generated only when the function is not in an OpenMP clause, or if the function is an implicit base of the declared variant function. This patch does that.

[zahiraam]

Review anyone please? Thanks.

[ABataev]

In D103529#2802346, @zahiraam wrote:

Review anyone please? Thanks.

Add more description to the summary. Would be good to see an analysis, why do we get this error message.

[ABataev]

clang/test/OpenMP/declare_variant.cpp

7

No need for semicolon after braces

[zahiraam]

In D103529#2802350, @ABataev wrote:

In D103529#2802346, @zahiraam wrote:

Review anyone please? Thanks.

Add more description to the summary. Would be good to see an analysis, why do we get this error message.

The warning warning: function 'foo' has internal linkage but is not defined

[-Wundefined-internal]

is generated at https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L814

In ActoOnstartFunctionDefinitioninOpenMPDeclareVariantScope there is new Decl that is created here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaOpenMP.cpp#L6685. This Decl is added to the UndefinedButUsed map here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExpr.cpp#L17118
This newly created Decl was marked 'imiplicit' but not marked 'used' as expected here https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/DeclBase.h#L578.

[ABataev]

In D103529#2802564, @zahiraam wrote:

In D103529#2802350, @ABataev wrote:

In D103529#2802346, @zahiraam wrote:

Review anyone please? Thanks.

Add more description to the summary. Would be good to see an analysis, why do we get this error message.

The warning warning: function 'foo' has internal linkage but is not defined

[-Wundefined-internal]

is generated at https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L814

In ActoOnstartFunctionDefinitioninOpenMPDeclareVariantScope there is new Decl that is created here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaOpenMP.cpp#L6685. This Decl is added to the UndefinedButUsed map here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExpr.cpp#L17118
This newly created Decl was marked 'imiplicit' but not marked 'used' as expected here https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/DeclBase.h#L578.

Thanks for the context. Not sure that your fix is correct. I assume we still need to emit this warning if the same code is compiled for the device, since there is no variant for the device. Your fix will suppress this warning completely. Also, we have to emit this warning if we have a variant for the device but use it on the host.

clang/test/OpenMP/declare_variant.cpp

6–8

Add a test for:

#pragma omp begin declare variant match(device = {kind(device)})
static void bar(){}
#pragma omp end declare variant
...
int main() {
...
  bar();
...
}

We have to see a warning (or some other diagnostics) since we don't have variant for the host.

[jdoerfert]

Does this by any chance also fix https://bugs.llvm.org/show_bug.cgi?id=49650 ?

clang/test/OpenMP/declare_variant.cpp

6–8

The standard says a base declaration is created, so this is legal and before the file is completely scanned we could not even try to emit a warning.

If there is no other variant for the host the linker will complain, but that is as expected.

[ABataev]

clang/test/OpenMP/declare_variant.cpp

6–8

Right. But I assume this warning is emitted at the end of the compilation module, when we do the analysis in ActOnEndOfTranslationUnit. Definitely, we can rely on the linker but would be good to reuse the existing framework for the analysis, if possible.

[zahiraam]

In D103529#2802659, @jdoerfert wrote:

Does this by any chance also fix https://bugs.llvm.org/show_bug.cgi?id=49650 ?

No. This doesn't fix the issue https://bugs.llvm.org/show_bug.cgi?id=49650

[zahiraam]

clang/test/OpenMP/declare_variant.cpp

6–8

There is a diagnostic triggered by https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseOpenMP.cpp#L889

[ABataev]

clang/test/OpenMP/declare_variant.cpp

13

It is triggered by the wrong kind. Should be nohost, I assume, or something like this. Check the standard

[ABataev]

I assume the proper fix is to handle declare variants in checkUndefinedButUsed in Sema.cpp. This should fix both PRs, mentioned here

[ABataev]

clang/test/OpenMP/declare_variant.cpp

15–17

Better to rename this function to baz and call it in main(). This should result in a warning.

[zahiraam]

In D103529#2802893, @ABataev wrote:

I assume the proper fix is to handle declare variants in checkUndefinedButUsed in Sema.cpp. This should fix both PRs, mentioned here

Let's see if this is better.
I have added to the LIT test the code that is in https://bugs.llvm.org/show_bug.cgi?id=49650
As for the one you proposed:
#pragma omp begin declare variant match(device = {kind(device)})
static void bar(){}
#pragma omp end declare variant
...
int main() {
...

bar();

...
}'=
I see that it is failing with the same error message that I found locally, https://godbolt.org/z/WWs9EnGhh
Unless you meant something else?

[zahiraam]

clang/test/OpenMP/declare_variant.cpp

15–17

Doing that results into an (expected?) error:
error: use of undeclared identifier 'baz'; did you mean 'bar'?

baz();
^~~
bar

[ABataev]

clang/test/OpenMP/declare_variant.cpp

15–17

Could you add a declaration outside of the declare variant?

static void baz();
#pragma omp begin declare variant match(device = {kind(nohost)})
static void baz() {}
#pragma omp end declare variant
...
baz();

[ABataev]

clang/lib/Sema/Sema.cpp

815–831

The check does not look correct. It will affect an existing behavior without OpenMP. The warning should be emitted if either in non-OpenMP mode, or if this is an implicit base of the declare variant function.

[zahiraam]

clang/lib/Sema/Sema.cpp

815–831

@ABataev This is what I came up with. It didn't fix the issue here https://bugs.llvm.org/show_bug.cgi?id=49650 because there is no implicit base. So not sure if this is quite correct, but wanted to run it by you. Please let me know.

[ABataev]

clang/lib/Sema/Sema.cpp

816

auto *

825

This check looks strange to me. What is it for?

[zahiraam]

clang/lib/Sema/Sema.cpp

825

Here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaOpenMP.cpp#L6693 the name of base function is mangled. In the attached LIT test "foo" is mangled to "foo$ompvariant{{.*}}". This is to test that FuncName and BaseName have the same "prefix". But may be they can never be different? In which case this test is un-necessary.

[ABataev]

clang/lib/Sema/Sema.cpp

825

No need to check for the name, better to check for something like BaseD->getIdentifier()->isMangledOpenMPVariantName()

[zahiraam]

clang/lib/Sema/Sema.cpp

825

Func is the one that is mangled. Agree?

[ABataev]

clang/lib/Sema/Sema.cpp

817

const auto *

821–822

const auto * and cast instead of dyn_cast

[ABataev]

clang/lib/Sema/Sema.cpp

817

const auto *

clang/test/OpenMP/declare_variant.cpp

5

Add //TODO that this must be fixed, please.

[zahiraam]

clang/test/OpenMP/declare_variant.cpp

5

Yes! Was wondering if it is not better to remove the this part of the test case all together?

[zahiraam]

clang/lib/Sema/Sema.cpp

817

auto* only because the const quallifier is not expected for getTraitInfo().

[ABataev]

LG, but add an extra description in summary

[zahiraam]

In D103529#2811434, @ABataev wrote:

LG, but add an extra description in summary

Thanks for taking time to review.

clang/lib/Sema/Sema.cpp

if (S.isExternalWithNoLinkageType(VD)) {
       S.Diag(VD->getLocation(), isExternallyVisible(VD->getType()->getLinkage())
                                     ? diag::ext_undefined_internal_type
                                     : diag::err_undefined_internal_type)
         << isa<VarDecl>(VD) << VD;
     } else if (!VD->isExternallyVisible()) {
       // FIXME: We can promote this to an error. The function or variable can't
       // be defined anywhere else, so the program must necessarily violate the
       // one definition rule.
+      bool IsImplicitBase = false;
+      if (auto *BaseD = dyn_cast<FunctionDecl>(VD)) {
+        OMPDeclareVariantAttr *DVAttr = BaseD->getAttr<OMPDeclareVariantAttr>();
+        if (DVAttr && !DVAttr->getTraitInfo().isExtensionActive(
+                          llvm::omp::TraitProperty::
+                              implementation_extension_disable_implicit_base)) {
+          FunctionDecl *Func = dyn_cast<FunctionDecl>(
+              cast<DeclRefExpr>(DVAttr->getVariantFuncRef())->getDecl());
+          IsImplicitBase =
+              BaseD->isImplicit() &&
+              Func->getIdentifier()->isMangledOpenMPVariantName();
+        }
+      }
+      if (!S.getLangOpts().OpenMP || !IsImplicitBase)
         S.Diag(VD->getLocation(), diag::warn_undefined_internal)
             << isa<VarDecl>(VD) << VD;
     } else if (auto *FD = dyn_cast<FunctionDecl>(VD)) {
       (void)FD;
       assert(FD->getMostRecentDecl()->isInlined() &&
              "used object requires definition but isn't inline or internal?");
       // FIXME: This is ill-formed; we should reject.
       S.Diag(VD->getLocation(), diag::warn_undefined_inline) << VD;
     } else {
       assert(cast<VarDecl>(VD)->getMostRecentDecl()->isInline() &&
              "used var requires definition but isn't inline or internal?");

clang/test/OpenMP/declare_variant.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -verify %s
+
+namespace {
+  // expected-warning@+1{{function '(anonymous namespace)::bar' has internal linkage but is not defined}}
+void bar();
+}
+
+#pragma omp begin declare variant match(user={condition(1)})
+void bar() {
+}
+#pragma omp end declare variant
+
+// expected-warning@+1{{function 'baz' has internal linkage but is not defined}}
+static void baz();
+#pragma omp begin declare variant match(device = {kind(nohost)})
+static void baz() {}
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(device = {kind(host)})
+static void foo() {}
+#pragma omp end declare variant
+
+int main() {
+  foo();
+// expected-note@+1{{used here}}
+  baz();
+// expected-note@+1{{used here}}
+  bar();
+
+  return 0;
+}