[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#3775384 <https://reviews.llvm.org/D133266#3775384>, @MaskRay wrote:

> 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.

`-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);
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133266