[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 inline comments.


================
Comment at: lib/Target/X86/X86Subtarget.cpp:146
+      !GV->hasDLLImportStorageClass() && GV->hasExternalLinkage() &&
+      GV->isDeclaration() && isa<GlobalVariable>(GV))
+    return X86II::MO_COFFSTUB;
----------------
mstorsjo wrote:
> rnk wrote:
> > mstorsjo wrote:
> > > I guess this condition also should be changed similarly, replacing `GV->hasExternalLinkage() && GV->isDeclaration()` with `GV->isDeclarationForLinker()`? It doesn't seem to change any of the existing testcases here in LLVM though.
> > I wouldn't expect it to, because by the time we get to codegen we've already discarded all available_externally definitions and turned them into declarations.
> > 
> > I just noticed this is x86-specific logic. Do we need COFF stubs more often on ARM, or can we change shouldAssumeDSOLocal to have this logic for all mingw targets?
> Ok - so even if it doesn't change anything, I guess it's good to have the expression match the one used in clang.
> 
> Yes, this is x86 specific - I've got later patches for arm and aarch64 in line afterwards. If we were to change shouldAssumeDSOLocal, we'd need to add all of the coffstub logic to arm and aarch64 all at once, and I'd rather do it gradually. Once they all support it, I could try to move the logic into the common shouldAssumeDSOLocal, as a NFC refactoring.
> 
> The stubs are essential on arm/aarch64, since the pseudo relocs don't support the arm instruction encodings, only plain pointers/offsets like in x86 relocs. For x86_64 the stubs fix cases when an autoimported variable gets loaded more than 4GB away from %rip at runtime. For x86_32, they avoid having to make the text section writable while fixing the relocs.
Gradually makes sense.


https://reviews.llvm.org/D51288