[Verifier] Reject dllexport with non-default visibility

[MaskRay]

Summary

Add a visibility check for dllimport and dllexport. Note: dllimport with a
non-default visibility (implicit dso_local) is already rejected, but with a less
clear dso_local error.

The MC level visibility MCSA_Exported (D123951) is mapped from IR level
default visibility when dllexport is specified. The D123951 error is now very
difficult to trigger (needs to disable the IR verifier).

[MaskRay]

test

[mstorsjo]

LGTM

llvm/test/Verifier/dllstorage.ll

6 ↗

(On Diff #457833)

I presume we can't easily get the symbol names into these error messages?

[MaskRay]

improve test

[bd1976llvm]

Hi! Sorry for not raising this issue prior to commit. The dllimport restriction seems correct to us. However, in PlayStation we are using dllexportclass=dllexport visibility=protected GlobalValues in LLVM IR. The protected visibility indicates that the symbol cannot be preempted and could be exported and dllexport indicates that it should be exported. This allows for implementing export control from C/C++ source code. This change has made this usage illegal. It doesn't makes sense semantically to restrict dllexport to visibility=default in LLVM IR, the restriction should be to visibility != hidden as hidden symbols cannot be exported by ELF linkers. Thanks.

[MaskRay]

In D133267#3771894, @bd1976llvm wrote:

Hi! Sorry for not raising this issue prior to commit. The dllimport restriction seems correct to us. However, in PlayStation we are using dllexportclass=dllexport visibility=protected GlobalValues in LLVM IR. The protected visibility indicates that the symbol cannot be preempted and could be exported and dllexport indicates that it should be exported. This allows for implementing export control from C/C++ source code. This change has made this usage illegal. It doesn't makes sense semantically to restrict dllexport to visibility=default in LLVM IR, the restriction should be to visibility != hidden as hidden symbols cannot be exported by ELF linkers. Thanks.

Oh, didn't know. Pushed 97d00b72a2b0a7aca631e1402a647f32c4e8bafb to allow dllexport protected.
Does PlayStation need to relax the clang check D133266, too? (I have noticed that this combo makes sense but rejected it for simplicity.)

[bd1976llvm]

In D133267#3773235, @MaskRay wrote:

In D133267#3771894, @bd1976llvm wrote:

Hi! Sorry for not raising this issue prior to commit. The dllimport restriction seems correct to us. However, in PlayStation we are using dllexportclass=dllexport visibility=protected GlobalValues in LLVM IR. The protected visibility indicates that the symbol cannot be preempted and could be exported and dllexport indicates that it should be exported. This allows for implementing export control from C/C++ source code. This change has made this usage illegal. It doesn't makes sense semantically to restrict dllexport to visibility=default in LLVM IR, the restriction should be to visibility != hidden as hidden symbols cannot be exported by ELF linkers. Thanks.

Oh, didn't know. Pushed 97d00b72a2b0a7aca631e1402a647f32c4e8bafb to allow dllexport protected.
Does PlayStation need to relax the clang check D133266, too? (I have noticed that this combo makes sense but rejected it for simplicity.)

Thanks, much appreciated :) I think we should relax the D133266 as well for completeness and to prevent customers having to edit their source code when it worked with previous compilers. However, I have spotted a potential problem with D133266. I will add a comment on that review.

llvm/lib/IR/Verifier.cpp

if (GV.hasAppendingLinkage()) {
     const GlobalVariable *GVar = dyn_cast<GlobalVariable>(&GV);
     Check(GVar && GVar->getValueType()->isArrayTy(),
           "Only global arrays can have appending linkage!", GVar);
   }
 
   if (GV.isDeclarationForLinker())
     Check(!GV.hasComdat(), "Declaration may not be in a Comdat!", &GV);
 
+  if (GV.hasDLLExportStorageClass()) {
+    Check(GV.hasDefaultVisibility(),
+          "dllexport GlobalValue must have default visibility", &GV);
+  }
   if (GV.hasDLLImportStorageClass()) {
+    Check(GV.hasDefaultVisibility(),
+          "dllimport GlobalValue must have default visibility", &GV);
     Check(!GV.isDSOLocal(), "GlobalValue with DLLImport Storage is dso_local!",
           &GV);
 
     Check((GV.isDeclaration() &&
            (GV.hasExternalLinkage() || GV.hasExternalWeakLinkage())) ||
               GV.hasAvailableExternallyLinkage(),
           "Global is marked as dllimport, but not external", &GV);
   }

llvm/test/CodeGen/PowerPC/aix-xcoff-exported-nondefault.ll

-; RUN: not --crash llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff 2>&1 < %s | \
-; RUN:   FileCheck %s
-; RUN: not --crash llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff 2>&1 < %s |\
-; RUN:   FileCheck %s
+; RUN: not llc -filetype=null -mtriple powerpc-ibm-aix-xcoff 2>&1 %s | FileCheck %s
+; RUN: not llc -filetype=null -mtriple powerpc64-ibm-aix-xcoff 2>&1 %s | FileCheck %s
 
-; RUN: not --crash llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff \
-; RUN:     -filetype=obj -o %t.o < %s 2>&1 | \
-; RUN:   FileCheck %s
-
-; RUN: not --crash llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff \
-; RUN:     -filetype=obj -o %t.o 2>&1 < %s 2>&1 | \
-; RUN:   FileCheck %s
-
-; CHECK: LLVM ERROR: Cannot not be both dllexport and non-default visibility
+; CHECK: dllexport GlobalValue must have default visibility
 @b_e = hidden dllexport global i32 0, align 4

llvm/test/Feature/globalvars.ll

 
 define i32 @foo(i32 %blah) {
         store i32 5, i32* @MyVar
         %idx = getelementptr { i32*, i32 }, { i32*, i32 }* @MyIntList, i64 0, i32 1             ; <i32*> [#uses=1]
         store i32 12, i32* %idx
         ret i32 %blah
 }
 
-@1 = hidden dllexport global i32 42
+@1 = default dllexport global i32 42
 @2 = dllexport global i32 42