[PATCH] D159085: [lli] Make sure the exported __chkstk functions are included when exporting them

[Mateusz Mikuła via Phabricator via llvm-commits]

mati865 accepted this revision.
mati865 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/tools/lli/lli.cpp:1204
 // Therefore, manually export the relevant symbol from lli, to let it be
 // found at runtime during tests.
 //
----------------
mstorsjo wrote:
> mati865 wrote:
> > mstorsjo wrote:
> > > mati865 wrote:
> > > > Should we update this comment?
> > > > This workaround is also required for linking to shared libraries.
> > > Do you mean if building an user application that does JIT, against the LLVM libraries? Yeah, the same issue applies there as well (regardless of whether it's linked shared or statically) - doing it within the `lli` executable just fixes it for the executable and none of the lib users. There's a TODO down below that this really should be handled by ORC.
> > > 
> > > And ideally we shouldn't just be exporting this function, ideally ORC should find the right `libclang_rt.builtins*.a` and link in that, it's just that the chkstk functions are the most blatant examples of it.
> > > 
> > > Put another way - I wasn't really planning on working on the JIT stuff, I just wanted to make `check-llvm` run successfully in mingw builds; first I tried just mark most of the JIT tests unsupported for mingw configs, but @lhames made and suggested some fixes that brought us to this point instead. The root issue isn't really solved but the tests at least run.
> > Sorry, I've put this comment in the wrong place, I meant lines 1206-1207.
> > 
> > > Do you mean if building an user application that does JIT, against the LLVM libraries? Yeah, the same issue applies there as well (regardless of whether it's linked shared or statically) - doing it within the lli executable just fixes it for the executable and none of the lib users. There's a TODO down below that this really should be handled by ORC.
> > 
> > I mean without this workaround `lli` is not buildable when linking to shared LLVM libs (IIUC this is what happened at MSYS2). I don't expect fixing ORC here.
> Ah - indeed, this fix is precisely for the shared LLVM libs issue found in MSYS2.
> 
> I don't see how this change affects the comments on lines 1206-1207. If the JIT would link in the real `libclang_rt.builtins*.a` at runtime, it would find `__chkstk` the right way from there. Since JIT doesn't do that at the moment, we need to make sure there's another exported `__chkstk` symbol somewhere in a loaded DLL, that the JIT can find - hence the dllexport trick. And here we have a tweak to make the dllexport trick work "when no functions allocate a large enough stack area" - which practically is when the majority of other code is in a different DLL, i.e. the shared libs or dylibs build modes.
Ah, so without this hack there would be no build error. I had missed it somehow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159085