[COFF] Don't generate -exclude-symbols for dllexported symbols

[mstorsjo]

Summary

A symbol can end up both with hidden visibility and a dllexport
attribute (in builds that set global flags like -fvisibility=hidden
or similar). In such cases, the hidden visibility doesn't have any
effect, as the dllexport directives take precedence, and no
autoexporting of all eligible symbols take place.

Therefore, it's entirely redundant to produce both -export: and
-exclude-symbols: for such a symbol - only produce export.

Even if an object file does contain one or more -export: directives,
it's relevant to include -exclude-symbols: for all other hidden
symbols though, because the linker could be invoked with
--export-all-symbols.

[rnk]

lgtm

[MaskRay]

This looks somewhat weird. How does a symbol end up with hidden dllexport? In other object file formats (ELF, Mach-O) this can't happen, since a hidden symbol cannot be exported.
In particular, in ELF, a STV_HIDDEN symbol's binding is changed to STB_LOCAL. Such a symbol cannot be a symbol resolution (relocation processing or dlsym) target. ld.lld never places a local symbol in the dynamic symbol table.

[mstorsjo]

In D133107#3764655, @MaskRay wrote:

This looks somewhat weird. How does a symbol end up with hidden dllexport? In other object file formats (ELF, Mach-O) this can't happen, since a hidden symbol cannot be exported.

It can happen if you build with e.g. -fvisibility=hidden, coupled with source that have __declspec(dllexport) for the symbols that should be exported.

Consider a project that has been built with -fvisibility=hidden traditionally (which has had no effect at all on windows targets, up until a month ago). Such a project would have e.g. an EXPORT define for symbols that should be exported. For windows, it expands to __declspec(dllexport) while it expands to __attribute__((visibility(“default”))) on other OSes.

So in one sense, I guess one could say that dllexport should imply default visibility on the clang level too.

[MaskRay]

In D133107#3764769, @mstorsjo wrote:

In D133107#3764655, @MaskRay wrote:

This looks somewhat weird. How does a symbol end up with hidden dllexport? In other object file formats (ELF, Mach-O) this can't happen, since a hidden symbol cannot be exported.

It can happen if you build with e.g. -fvisibility=hidden, coupled with source that have __declspec(dllexport) for the symbols that should be exported.

Consider a project that has been built with -fvisibility=hidden traditionally (which has had no effect at all on windows targets, up until a month ago). Such a project would have e.g. an EXPORT define for symbols that should be exported. For windows, it expands to __declspec(dllexport) while it expands to __attribute__((visibility(“default”))) on other OSes.

So in one sense, I guess one could say that dllexport should imply default visibility on the clang level too.

Yes. I think this is better. __attribute__((visibility("hidden"))) __declspec(dllexport) should probably lead to an error.

Sent D133180 to suppress hidden for dllexport as we do for dllimport.

[mstorsjo]

In D133107#3765989, @MaskRay wrote:

Yes. I think this is better.

Sent D133180 to suppress hidden for dllexport as we do for dllimport.

Thanks. What do you think about this patch then - I think it still might be best to do this change? E.g. if other frontends than clang generate such IR - even if it is a bit tautological.

__attribute__((visibility("hidden"))) __declspec(dllexport) should probably lead to an error.

Should, but doesn't yet - how hard do you think it'd be to add checking for that?

[MaskRay]

In D133107#3766175, @mstorsjo wrote:

In D133107#3765989, @MaskRay wrote:

Yes. I think this is better.

Sent D133180 to suppress hidden for dllexport as we do for dllimport.

Thanks. What do you think about this patch then - I think it still might be best to do this change? E.g. if other frontends than clang generate such IR - even if it is a bit tautological.

__attribute__((visibility("hidden"))) __declspec(dllexport) should probably lead to an error.

Should, but doesn't yet - how hard do you think it'd be to add checking for that?

Not too difficult:) I have read a bit the visibility code. Created D133266 to reject the construct in Clang and D133267 to reject hidden dllexport IR (note that we already reject hidden dllimport IR).

[mstorsjo]

Superseded by D133180, D133266 and D133267 - thanks @MaskRay!

llvm/lib/IR/Mangler.cpp

if (NeedQuotes)
       OS << "\"";
 
     if (!GV->getValueType()->isFunctionTy()) {
       if (TT.isWindowsMSVCEnvironment())
         OS << ",DATA";
       else
         OS << ",data";
     }
-  }
-  if (GV->hasHiddenVisibility() && !GV->isDeclaration() && TT.isOSCygMing()) {
+  } else if (GV->hasHiddenVisibility() && !GV->isDeclaration() &&
+             TT.isOSCygMing()) {
 
     OS << " -exclude-symbols:";
 
     bool NeedQuotes = GV->hasName() && !canBeUnquotedInDirective(GV->getName());
     if (NeedQuotes)
       OS << "\"";
 
     std::string Flag;

llvm/test/CodeGen/X86/mingw-hidden.ll

define void @notHidden() {
   ret void
 }
 
 ; CHECK: .globl _f1
 define hidden void @f1() {
   ret void
 }
 
+; CHECK: .globl _exportedHidden
+define hidden dllexport void @exportedHidden() {
+  ret void
+}
+
 ; CHECK: .globl _f2
 define hidden void @f2() unnamed_addr {
   ret void
 }
 
 declare hidden void @notDefined()
 
 ; CHECK: .globl _stdfun@0

 
 
 ; Verify items that should not be marked hidden do not appear in the directives.
 ; We use a separate check prefix to avoid confusion between -NOT and -SAME.
 ; NOTEXPORTED: .section .drectve
 ; NOTEXPORTED-NOT: :notHidden
 ; NOTEXPORTED-NOT: :notDefined
 
-; CHECK-MSVC-NOT: .section .drectve
+; CHECK-MSVC: .section .drectve
+; CHECK-MSVC: .ascii " /EXPORT:_exportedHidden"
 ; CHECK-MINGW: .section .drectve
 ; CHECK-MINGW: .ascii " -exclude-symbols:f1"
+; CHECK-MINGW: .ascii " -export:exportedHidden"
+; CHECK-MINGW-NOT: -exclude-symbols:exportedHidden
 ; CHECK-MINGW: .ascii " -exclude-symbols:f2"
 ; CHECK-MINGW: .ascii " -exclude-symbols:stdfun@0"
 ; CHECK-MINGW: .ascii " -exclude-symbols:lnk1"
 ; CHECK-MINGW: .ascii " -exclude-symbols:lnk2"
 ; CHECK-MINGW: .ascii " -exclude-symbols:Var1"
 ; CHECK-MINGW: .ascii " -exclude-symbols:Var2"
 ; CHECK-MINGW: .ascii " -exclude-symbols:Var3"
 ; CHECK-MINGW: .ascii " -exclude-symbols:\"complex-name\""
 ; CHECK-MINGW: .ascii " -exclude-symbols:\"complex.name\""