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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In https://reviews.llvm.org/D51288#1216484, @rnk wrote:

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


Yes, if I understood correctly from the commit history, the whole method was supposed to go away, more or less.

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

Yup, that makes sense. I'll see what I can do to allow omitting .refptr for things marked dso_local, without refactoring everything all at once.


Repository:
  rL LLVM

https://reviews.llvm.org/D51288