[PATCH] D55229: [COFF] Statically link certain runtime library functions

Thu Dec 13 12:58:31 PST 2018

rnk added inline comments.

================
Comment at: CodeGenObjC/gnu-init.m:103
+// Make sure we do not have dllimport on the load function
+// CHECK-WIN: declare dso_local void @__objc_load

----------------
theraven wrote:
> compnerd wrote:
> > rnk wrote:
> > > compnerd wrote:
> > > > rnk wrote:
> > > > > mgrang wrote:
> > > > > > I had to remove dllimport in this and the next unit test. I am not sure of the wider implications of this change. Maybe controlling this via a flag (like -flto-visibility-public-std) is a better/more localized option?
> > > > > These are the ones that I think @compnerd really cares about since the objc runtime is typically dynamically linked and frequently called. We might want to arrange things such that we have a separate codepath for ObjC runtime helpers. I'm surprised we don't already have such a code path.
> > > > I think that @theraven would care more about this code path than I.  I think that this change may be wrong, because the load here is supposed to be in the ObjC runtime, and this becoming local to the module would break the global registration.
> > > We just set dso_local whenever something isn't dllimport, even if it's a lie sometimes. I think everything would work as intended with a linker thunk. It's "true" as far as LLVM is concerned for the purposes of emitting relocations.
> > Ah, okay, then, this might be okay.  However, the use of `dllimport` here would force that the runtime to be called.  Then again it is possible to statically link ...
> `__objc_load` is a function defined in objc.dll.  It absolutely does want to be dlimport, or the linker won't be able to find it.
I don't agree, it doesn't have to be marked dllimport. MSVC, ld.bfd, and lld will all synthesize a linker thunk so that the call works. Marking it dllimport is a performance optimization.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55229/new/

https://reviews.llvm.org/D55229