[PATCH] D133266: [MinGW] Reject explicit non-default visibility applied to dllexport/dllimport declaration

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D133266#3775189 <https://reviews.llvm.org/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.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1104
   if (GV->hasLocalLinkage()) {
     GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
     return;
----------------
mstorsjo wrote:
> 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.
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() {}
                                  ^
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133266/new/

https://reviews.llvm.org/D133266