[MinGW] Reject explicit hidden visibility applied to dllexport and hidden/protected applied to dllimport

[MaskRay]

Summary

Hidden visibility is incompatible with dllexport.
Hidden and protected visibilities are incompatible with dllimport.
(PlayStation uses dllexport protected.)

When an explicit visibility attribute applies on a dllexport/dllimport
declaration, report a Frontend error (Sema does not compute visibility).

[mstorsjo]

LGTM, thanks!

clang/lib/CodeGen/CodeGenModule.cpp

1104

I was wondering if we're changing behaviour in some other case since this is now checked before the dllexport/dllimport, but I think it shouldn't make any difference, since we already error out (elsewhere) on dllexport+static today.

[bd1976llvm]

This approach doesn't account for the implementation of -fvisibility-global-new-delete-hidden. The following case now fails after this change:

>type new_del.cpp

namespace std {
  typedef __typeof__(sizeof(int)) size_t;
  struct nothrow_t {};
  struct align_val_t {};
}
__declspec(dllexport) void operator delete(void* ptr) throw() {return;}


>clang.exe --target=x86_64-pc-windows-gnu new_del.cpp -fvisibility-global-new-delete-hidden -c
new_del.cpp:8:28: error: non-default visibility cannot be applied to 'dllexport' declaration
__declspec(dllexport) void operator delete(void* ptr) throw() {return;}
                           ^
1 error generated.

[MaskRay]

In D133266#3775189, @bd1976llvm wrote:

This approach doesn't account for the implementation of -fvisibility-global-new-delete-hidden. The following case now fails after this change:

>type new_del.cpp

namespace std {
  typedef __typeof__(sizeof(int)) size_t;
  struct nothrow_t {};
  struct align_val_t {};
}
__declspec(dllexport) void operator delete(void* ptr) throw() {return;}


>clang.exe --target=x86_64-pc-windows-gnu new_del.cpp -fvisibility-global-new-delete-hidden -c
new_del.cpp:8:28: error: non-default visibility cannot be applied to 'dllexport' declaration
__declspec(dllexport) void operator delete(void* ptr) throw() {return;}
                           ^
1 error generated.

From a glance, -fvisibility-global-new-delete-hidden should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.

clang/lib/CodeGen/CodeGenModule.cpp

1104

Confirmed. We have diagnostics like:

a.c:1:35: error: 'f' must have external linkage when declared 'dllexport'
static __declspec(dllexport) void f() {}
                                  ^
a.c:2:35: error: 'g' must have external linkage when declared 'dllimport'
static __declspec(dllimport) void g() {}
                                  ^

[MaskRay]

In D133266#3775384, @MaskRay wrote:

In D133266#3775189, @bd1976llvm wrote:

This approach doesn't account for the implementation of -fvisibility-global-new-delete-hidden. The following case now fails after this change:

>type new_del.cpp

namespace std {
  typedef __typeof__(sizeof(int)) size_t;
  struct nothrow_t {};
  struct align_val_t {};
}
__declspec(dllexport) void operator delete(void* ptr) throw() {return;}


>clang.exe --target=x86_64-pc-windows-gnu new_del.cpp -fvisibility-global-new-delete-hidden -c
new_del.cpp:8:28: error: non-default visibility cannot be applied to 'dllexport' declaration
__declspec(dllexport) void operator delete(void* ptr) throw() {return;}
                           ^
1 error generated.

From a glance, -fvisibility-global-new-delete-hidden should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.

-fvisibility-global-new-delete-hidden is implemented by adding VisibilityAttr to declarations.
I think -fvisibility-global-new-delete-hidden triggering the error is fine. The alternative, adding a rule that __declspec(dllexport) void operator delete does not get hidden visibility, seems ad-hoc to me.

I think the only needed change is to allow dllexport protected, but then we probably need two diagnostics. Perhaps diag::err_hidden_visiblity_dllexport and diag::err_non_default_visibility_dllimport?

if (LV.isVisibilityExplicit()) {
  // Reject explicit hidden visibility on dllexport and hidden/protected
  // visibility on dllimport.
  if (GV->hasDLLExportStorageClass() &&
      LV.getVisibility() == HiddenVisibility)
    getDiags().Report(D->getLocation(),
                      diag::err_hidden_visibility_dllexport);
  if (GV->hasDLLImportStorageClass() &&
      LV.getVisibility() != DefaultVisibility)
    getDiags().Report(D->getLocation(),
                      diag::err_non_default_visibility_dllimport);
}

[bd1976llvm]

In D133266#3775832, @MaskRay wrote:

In D133266#3775384, @MaskRay wrote:

In D133266#3775189, @bd1976llvm wrote:

From a glance, -fvisibility-global-new-delete-hidden should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.

-fvisibility-global-new-delete-hidden is implemented by adding VisibilityAttr to declarations.
I think -fvisibility-global-new-delete-hidden triggering the error is fine. The alternative, adding a rule that __declspec(dllexport) void operator delete does not get hidden visibility, seems ad-hoc to me.

I'm not sure why this visibility option (-fvisibility-global-new-delete-hidden) is implemented differently to the others (e.g. -fvisibility=hidden)? __declspec(dllexport) void operator delete does not get hidden visibility, might be difficult to implement; but OTOH, the dllstorageclass overrides the visibility set by a visibility option for the other visibility options (e.g. -fvisibility=hidden) and it would be nice to have consistent behaviour for all the visibility options. Would be great to get other peoples opinions on whether an error here would be a problem?

I think the only needed change is to allow dllexport protected, but then we probably need two diagnostics. Perhaps diag::err_hidden_visiblity_dllexport and diag::err_non_default_visibility_dllimport?

SGTM!

[MaskRay]

In D133266#3776051, @bd1976llvm wrote:

In D133266#3775832, @MaskRay wrote:

In D133266#3775384, @MaskRay wrote:

In D133266#3775189, @bd1976llvm wrote:

From a glance, -fvisibility-global-new-delete-hidden should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.

-fvisibility-global-new-delete-hidden is implemented by adding VisibilityAttr to declarations.
I think -fvisibility-global-new-delete-hidden triggering the error is fine. The alternative, adding a rule that __declspec(dllexport) void operator delete does not get hidden visibility, seems ad-hoc to me.

I'm not sure why this visibility option (-fvisibility-global-new-delete-hidden) is implemented differently to the others (e.g. -fvisibility=hidden)? __declspec(dllexport) void operator delete does not get hidden visibility, might be difficult to implement; but OTOH, the dllstorageclass overrides the visibility set by a visibility option for the other visibility options (e.g. -fvisibility=hidden) and it would be nice to have consistent behaviour for all the visibility options. Would be great to get other peoples opinions on whether an error here would be a problem?

First, does COFF use -fvisibility-global-new-delete-hidden? The impl of -fvisibility-global-new-delete-hidden is very different from -fvisibility. -fvisibility changed visibility is considered !isVisibilityExplicit while -fvisibility-global-new-delete-hidden's is isVisibilityExplicit.

dllspec and visibility are extensions for different object file formats and here we are mixing them together.
Personally I don't favor defining too many "let A override B" designs. Adding if (GV->hasDLLExportStorageClass() || GV->hasDLLImportStorageClass()) is mostly only due to compatibility consideration...

I think the only needed change is to allow dllexport protected, but then we probably need two diagnostics. Perhaps diag::err_hidden_visiblity_dllexport and diag::err_non_default_visibility_dllimport?

SGTM!

[MaskRay]

.

[bd1976llvm]

In D133266#3776164, @MaskRay wrote:

In D133266#3776051, @bd1976llvm wrote:

In D133266#3775832, @MaskRay wrote:

In D133266#3775384, @MaskRay wrote:

In D133266#3775189, @bd1976llvm wrote:

From a glance, -fvisibility-global-new-delete-hidden should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.

-fvisibility-global-new-delete-hidden is implemented by adding VisibilityAttr to declarations.
I think -fvisibility-global-new-delete-hidden triggering the error is fine. The alternative, adding a rule that __declspec(dllexport) void operator delete does not get hidden visibility, seems ad-hoc to me.

I'm not sure why this visibility option (-fvisibility-global-new-delete-hidden) is implemented differently to the others (e.g. -fvisibility=hidden)? __declspec(dllexport) void operator delete does not get hidden visibility, might be difficult to implement; but OTOH, the dllstorageclass overrides the visibility set by a visibility option for the other visibility options (e.g. -fvisibility=hidden) and it would be nice to have consistent behaviour for all the visibility options. Would be great to get other peoples opinions on whether an error here would be a problem?

First, does COFF use -fvisibility-global-new-delete-hidden? The impl of -fvisibility-global-new-delete-hidden is very different from -fvisibility. -fvisibility changed visibility is considered !isVisibilityExplicit while -fvisibility-global-new-delete-hidden's is isVisibilityExplicit.

dllspec and visibility are extensions for different object file formats and here we are mixing them together.
Personally I don't favor defining too many "let A override B" designs. Adding if (GV->hasDLLExportStorageClass() || GV->hasDLLImportStorageClass()) is mostly only due to compatibility consideration...

Sorry for the slow reply. -fvisibility-global-new-delete-hidden was implemented here: https://reviews.llvm.org/D53787, it is Clang only (it is not in GCC) and, as you pointed out, the changed visibility is deliberately isVisibilityExplicit. We are happy to accept the change in behaviour as we don't think it will impact customers.

I think the only needed change is to allow dllexport protected, but then we probably need two diagnostics. Perhaps diag::err_hidden_visiblity_dllexport and diag::err_non_default_visibility_dllimport?

SGTM!

LGTM. Thanks for your patience.

clang/lib/CodeGen/CodeGenModule.cpp

1116

Opinion: I don't think these comments are needed as the code is clear. I think it would be better to have a single comment at the top of this block e.g. "Reject incompatible dlllstorageclass and visibility annotations" or just remove them.

[MaskRay]

clang/lib/CodeGen/CodeGenModule.cpp

1116

Will change. Thanks!

[bd1976llvm]

Whilst reviewing the impact of these dllimport/export restrictions I noticed that LLVM IR does not reject dllimport/export on local linkage globals. I have put up a PR for adding that restriction: D134784 (https://reviews.llvm.org/D134784).

clang/include/clang/Basic/DiagnosticFrontendKinds.td

 def warn_alias_to_weak_alias : Warning<
   "%select{alias|ifunc}2 will always resolve to %0 even if weak definition of "
   "%1 is overridden">,
   InGroup<IgnoredAttributes>;
 def err_duplicate_mangled_name : Error<
   "definition with same mangled name '%0' as another definition">;
 def err_cyclic_alias : Error<
   "%select{alias|ifunc}0 definition is part of a cycle">;
+def err_hidden_visibility_dllexport : Error<
+  "hidden visibility cannot be applied to 'dllexport' declaration">;
+def err_non_default_visibility_dllimport : Error<
+  "non-default visibility cannot be applied to 'dllimport' declaration">;
 def err_ifunc_resolver_return : Error<
   "ifunc resolver function must return a pointer">;
 
 def warn_atomic_op_misaligned : Warning<
   "misaligned atomic operation may incur "
   "significant performance penalty"
   "; the expected alignment (%0 bytes) exceeds the actual alignment (%1 bytes)">,
   InGroup<AtomicAlignment>;

clang/lib/CodeGen/CodeGenModule.cpp

 }
 
 llvm::ConstantInt *CodeGenModule::getSize(CharUnits size) {
   return llvm::ConstantInt::get(SizeTy, size.getQuantity());
 }
 
 void CodeGenModule::setGlobalVisibility(llvm::GlobalValue *GV,
                                         const NamedDecl *D) const {
-  if (GV->hasDLLExportStorageClass() || GV->hasDLLImportStorageClass())
-    return;
   // Internal definitions always have default visibility.
   if (GV->hasLocalLinkage()) {
     GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
     return;
   }
   if (!D)
     return;
   // Set visibility for definitions, and for declarations if requested globally
   // or set explicitly.
   LinkageInfo LV = D->getLinkageAndVisibility();
+  if (GV->hasDLLExportStorageClass() || GV->hasDLLImportStorageClass()) {
+    // Reject incompatible dlllstorage and visibility annotations.
+    if (!LV.isVisibilityExplicit())
+      return;
+    if (GV->hasDLLExportStorageClass()) {
+      if (LV.getVisibility() == HiddenVisibility)
+        getDiags().Report(D->getLocation(),
+                          diag::err_hidden_visibility_dllexport);
+    } else if (LV.getVisibility() != DefaultVisibility) {
+      getDiags().Report(D->getLocation(),
+                        diag::err_non_default_visibility_dllimport);
+    }
+    return;
+  }
+
   if (LV.isVisibilityExplicit() || getLangOpts().SetVisibilityForExternDecls ||
       !GV->isDeclarationForLinker())
     GV->setVisibility(GetLLVMVisibility(LV.getVisibility()));
 }
 
 static bool shouldAssumeDSOLocal(const CodeGenModule &CGM,
                                  llvm::GlobalValue *GV) {
   if (GV->hasLocalLinkage())

clang/test/CodeGenCXX/dllstorage-visibility.cpp

 /// dllimport and dllexport express non-hidden intention. Don't apply hidden visibility.
 
 // RUN: %clang_cc1 -emit-llvm -triple x86_64-windows-msvc -fdeclspec -fvisibility-inlines-hidden -o - %s | FileCheck %s --check-prefix=MSVC
 // RUN: %clang_cc1 -emit-llvm -triple x86_64-windows-gnu -fdeclspec -fvisibility-inlines-hidden -o - %s | FileCheck %s --check-prefix=GNU
 // RUN: %clang_cc1 -emit-llvm -triple x86_64-windows-gnu -fdeclspec -fvisibility=hidden -o - %s | FileCheck %s --check-prefix=GNU
 
+// RUN: %clang_cc1 -emit-llvm-only -verify -triple x86_64-windows-gnu -fdeclspec -DERR1 -o - %s
+// RUN: %clang_cc1 -emit-llvm-only -verify -triple x86_64-windows-gnu -fdeclspec -fvisibility=hidden -DERR1 -o - %s
+// RUN: %clang_cc1 -emit-llvm-only -verify -triple x86_64-windows-gnu -fdeclspec -DERR2 -o - %s
+// RUN: %clang_cc1 -emit-llvm-only -verify -triple x86_64-windows-gnu -fdeclspec -DERR3 -o - %s
+
 #define CONCAT2(x, y) x##y
 #define CONCAT(x, y) CONCAT2(x, y)
 #define USE(func) void CONCAT(use, __LINE__)() { func(); }
 
+#define HIDDEN __attribute__((visibility("hidden")))
+#define PROTECTED __attribute__((visibility("protected")))
+#define DEFAULT __attribute__((visibility("default")))
+
 // MSVC-DAG: declare dllimport void @"?bar@foo@@QEAAXXZ"(
 // GNU-DAG: define linkonce_odr hidden void @_ZN3foo3barEv(
 
 struct __attribute__((dllimport)) foo {
   void bar() {}
 };
 void zed(foo *p) { p->bar(); }
 
 // MSVC-DAG: define dso_local dllexport void @"?exported@@YAXXZ"(
 // GNU-DAG: define dso_local dllexport void @_Z8exportedv(
 __attribute__((dllexport)) void exported() {}
 
 // MSVC-DAG: define weak_odr dso_local dllexport void @"?exported_inline@@YAXXZ"(
 // GNU-DAG: define weak_odr dso_local dllexport void @_Z15exported_inlinev(
 __declspec(dllexport) inline void exported_inline() {}
 USE(exported_inline)
+
+__attribute__((dllexport)) DEFAULT void exported_default() {}
+
+// MSVC-DAG: define protected dllexport void @"?exported_protected@@YAXXZ"(
+// GNU-DAG: define protected dllexport void @_Z18exported_protectedv(
+#pragma GCC visibility push(protected)
+__attribute__((dllexport)) void exported_protected() {}
+#pragma GCC visibility pop
+
+#if defined(ERR1)
+// expected-error@+1 {{non-default visibility cannot be applied to 'dllimport' declaration}}
+__attribute__((dllimport)) HIDDEN void imported_hidden();
+
+// expected-error@+1 {{hidden visibility cannot be applied to 'dllexport' declaration}}
+__attribute__((dllexport)) HIDDEN void exported_hidden() { imported_hidden(); }
+
+#elif defined(ERR2)
+// expected-error@+1 {{non-default visibility cannot be applied to 'dllimport' declaration}}
+__attribute__((dllimport)) PROTECTED void imported_protected();
+
+void foo() { imported_protected(); }
+
+#elif defined(ERR3)
+struct HIDDEN C2 {
+  // expected-error@+1 {{hidden visibility cannot be applied to 'dllexport' declaration}}
+  __attribute__((dllexport)) void exported_hidden() {}
+};
+#endif