[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#1215685, @mstorsjo wrote:

> In https://reviews.llvm.org/D51288#1215140, @rnk wrote:
>
> > I think you need to change clang lib/CodeGen/CodeGenModule.cpp `shouldAssumeDSOLocal` to stop marking these things as dso_local, and then `shouldAssumeDSOLocal` will return false, and you can apply your MO_COFFSTUB flag in `classifyGlobalReference`. Hopefully then everything will make a little more sense.
>
>
> Right, I'll give that a try. That sounds like it would make a little more sense.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D51288