[PATCH] D51288: [MinGW] [X86] Add stubs for references to data variables that might end up imported from a dll

[Reid Kleckner via Phabricator via llvm-commits]

rnk added subscribers: smeenai, compnerd.
rnk added a comment.

In https://reviews.llvm.org/D51288#1215735, @mstorsjo wrote:

> Ok, I started off doing that, but once I stop setting DSOLocal on all variables, how would you suggest I'd proceed next? In TargetMachine::shouldAssumeDSOLocal we've got pretty much the same code as in Clang, more or less - even if we don't have the DSOLocal flag set, we run into this block:
>
>   bool TargetMachine::shouldAssumeDSOLocal(const Module &M,
>                                            const GlobalValue *GV) const {
>     if (GV && GV->isDSOLocal())
>       return true;
>  
>     if (GV && GV->hasDLLImportStorageClass())
>       return false;
>    
>     // Every other GV is local on COFF.
>     if (TT.isOSBinFormatCOFF() || (TT.isOSWindows() && TT.isOSBinFormatMachO()))
>       return true;
>
>
> So even if we remove the DSOLocal flag, we end up doing pretty much the same.
>
> It might make sense in the long run to add the same "if (mingw) don'tAssumeEverythingIsDSOLocal();" there - however TargetMachine::shouldAssumeDSOLocal is used in varying ways in X86/ARM/AArch64, so if we change that one, we'd need all three of them to be ready at the same time.
>
> And if I don't change TargetMachine::shouldAssumeDSOLocal quite yet, I don't really see much difference from what this patch does right now. I guess I could use `!GV->isDSOLocal()` for part of the condition though...


Well, I think that COFF-specific check in shouldAssumeDSOLocal is intended to go away at some point, once frontends have migrated over to setting dso_local appropriately. Rafael would be able to tell us what he was thinking here, but obviously he's no longer involved with LLVM. I think it would be reasonable to restrict the "if COFF then local" check only apply to non-gnu environments.

Mainly, I don't want the dso_local marker to become a lie, and I think it's nice to be able to mark specific symbols as being local so that we can avoid the .refptr stubs. A little more duplication between clang and llvm is a reasonable cost for that.

@compnerd @smeenai, do you want these .refptr stubs for x86_64-windows-itanium? It seems like they fix a number of COFF/ELF portability issues.


Repository:
  rL LLVM

https://reviews.llvm.org/D51288