[PATCH] D51452: [MinGW] [AArch64] Add stubs for potential automatic dllimported variables

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: lib/Target/AArch64/AArch64Subtarget.cpp:204
+           !GV->isDSOLocal() && GV->isDeclarationForLinker() &&
+           isa<GlobalVariable>(GV))
+    Flags = AArch64II::MO_COFFSTUB | AArch64II::MO_GOT;
----------------
efriedma wrote:
> efriedma wrote:
> > mstorsjo wrote:
> > > efriedma wrote:
> > > > I don't see why it matters whether the value is a GlobalVariable, specifically, as opposed to an alias or function.
> > > > 
> > > > I don't see why this applies specifically to MinGW, as opposed to all Windows targets.
> > > > 
> > > > I'm not sure what the point of the isDeclarationForLinker check is... it seems redundant with the isDSOLocal check.
> > > Sorry, there's probably not nearly enough context here - this is the AArch64 followup to D51382 and D51288; it should do the same as the latter but for the AArch64 target.
> > > 
> > > This relates to MinGW and automatically importing data variables from another DLL, even though the code referencing the variable didn't use a dllimport attribute.
> > > 
> > > For functions, in case they turn out to need to be imported from a DLL even though the access didn't know that, the linker will just insert a thunk and use that instead (and even MSVC supports this).
> > > 
> > > To handle the same for data, though, the linker must create a list of deferred relocations that the MinGW runtime will process on load, to fix things up for cases when a variable turned out to be automatically imported from a DLL. All of this is a MinGW specific concept; MSVC's runtime and linker don't support it.
> > > 
> > > This patch reroutes accesses to variables that potentially might need to be imported from a DLL via a stub pointer, to avoid having the runtime fixup touch the code section.
> > > 
> > > As for aliases, I'm not familiar enough with them on the IR level to know whether they should get the same treatment or not.
> > Okay, that makes sense.
> > 
> > For the alias thing, I guess the right way to deal with it is to check something like `!isa<FunctionType>(GV->getValueType())`.
> > 
> > Still not sure why you need the isDeclarationForLinker check, given you already check isDSOLocal().  Does the isDSOLocal() check not work correctly in some cases?
> Oh, just read the discussion on D51288; refactoring the DSOLocal stuff incrementally should be fine.
Ok, checking for functions like that might be good. We have a bunch of similar checks added in both of the earlier commits, and since I think that's a fringe case (and I'm planning to refactor parts of this to reduce the duplication, once all of X86, ARM and AArch64 supports this) perhaps it makes sense to postpone that change until then?

As for `isDeclarationForLinker()`, originally I had `hasExternalLinkage() && isDeclaration()` (with the intent to only do this for variables where we don't see any definition within the current scope). In D51382 (in clang, to skip setting the `dso_local` for potentially imported variables), @rnk suggested to use `isDeclarationForLinker()`. Even though the distinction between the two might not make sense at this spot (while it does when generating IR in clang), I think it makes sense to try to keep the (currently quite scattered) conditions as similar as possible.


Repository:
  rL LLVM

https://reviews.llvm.org/D51452