IRGen: prevent WeakExternal on COFF

[compnerd]

We cannot use ExternalWeakLinkage on PE/COFF as the semantics for that
do not exist. It is theoretically possible to emulate this by manually
creating a default value for the symbol that evaluates to 0, but that
requires adding a set of linker directives and variable definitions.
Disable the weak linkage rather than generate an invalid module.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

[compnerd]

CC: @rjmccall @jckarter

[compnerd]

@swift-ci please test

[compnerd]

I'm really kinda torn on this change. I am half tempted to sink this into the IRLinkage applicator where during the application we "promote" the ExternalWeakLinkage to ExternalLinkage as we have the target triple at that point. But then should we sink the visibility handling to that point ("reducing" the Protected visibility to Public for MachO/COFF)? If we do that, we completely remove any special handling for the IRLinkage calculation types, we choose ELF semantics and adjust that during the application to the target object file format. The reservation that I have with that is that it feels like we do this at the wrong time then as we have lost some of the additional information about the entity that we are working with.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - df1acfd

[jckarter]

Why are you getting weak-linkage symbols on Windows to begin with? That's pretty much an Apple specific feature.

[compnerd]

@jckarter because the frontend doesn't track the triple, SIL declarations will often get PublicExternal linkage which we blindly translate into weak_extern.

[jckarter]

That doesn't sound right. Looking at the code, LinkEntity::isWeakImported would only return true if the SIL definition was marked isWeakLinked. Going back to the discussion in #20822, there's no reason for that to be true on non-Apple platforms.

[compnerd]

@jckarter - hmm, maybe I'm misunderstanding something, because my reading is that SILGen really just blindly assumes weak linkage is available...

SILFunctionBuilder::getOrCreateFunction invokes setWeakLinked unconditionally, which means that SIL will definitely indicate weak linkage. LinkEntity::isWeakImported will see that and simply just check ->isAvailableExternally(). I don't see anywhere where there is a check for MachO or ELF.

Note that weak external linkage is also supported on ELF.

[jckarter]

Where are you seeing it get set unconditionally? I see:

    if (decl->isWeakImported(/*fromModule=*/nullptr))
      F->setWeakLinked();

It would be a bug for decl->isWeakImported to be true for anything not from an Apple SDK. @_weakLinked is used for testing some things, I guess, but we should flag the attribute as unsupported on non-Apple platforms and conditionalize those tests.

[compnerd]

@jckarter - right, that seems to not take into account the object file format ... or is there a code path that I'm not finding that checks that?

[jckarter]

The object format isn't the point. isWeakImported shouldn't ever be true to begin with if you're targeting Windows.

[compnerd]

Right, but that's what I'm not seeing, how does it actually account for the fact that everything may match but because it is windows, it should return false. To be more clear, how does this get handled:

@_weakLinked public func f() {}

[jckarter]

That's what we should figure out—why are we getting in this state to begin with? It shouldn't need any Windows-specific logic in the compiler because there's nothing in the Windows SDK that should ever be able to indicate a C definition is weak-linked because that concept doesn't exist on the platform.

[jckarter]

For instance, if it's coming from declarations marked @_weakLinked in the compiler tests, then we should make it an error to use that attribute on platforms it's not supported, and conditionalize those tests to only run on Apple platforms.

[compnerd]

Hmm, how do you get the target triple during the SIL parsing phase?

[jckarter]

It should be in the LangOptions in the ASTContext. I don't think you want to look at this during SIL parsing or generation, though; the problem looks like it comes earlier than that.

[jrose-apple]

I don't think you should do anything trying to get those tests to run or error. Just mark them unsupported and move on. (Or separate out the parts that depend on weak linking, if they're mixed in with larger files.) We don't have any non-underscored user-visible features that would result in weak linking.

[jckarter]

Making the attribute error out would at least let the compiler help find all the test cases that need to be marked unsupported. There's a chance something else might be wrong too, so having an assertion in place in IRGen or somewhere else if we try to form a weak reference on a non-Darwin platform seems to me like a nice canary to have as well.

[jrose-apple]

Oh, I figured there was already an assertion and that that would be good enough, but I guess pushing the assertion up higher is reasonable.

[compnerd]

Okay, #21798 should take care of promoting this to a nice diagnostic rather than a crash, even with user input.