Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cl.exe c:crystalllvm-10.0.0.srcReleaseinllvm-config.exe --libs --system-libs --ldflags 2> /dev/null #9089

Closed
oprypin opened this issue Apr 15, 2020 · 13 comments

Comments

@oprypin
Copy link
Member

oprypin commented Apr 15, 2020

With PR #9054 in (currently declared as the last step for bootstrapping the compiler on Windows), one can almost almost compile the compiler for Windows and then on Windows, but at the end, the following is passed to the linker as part of the command line:

cl.exe[…]C:\crystal\src\llvm/ext/llvm_ext.o `c:crystalllvm-10.0.0.srcRelease�inllvm-config.exe --libs --system-libs --ldflags 2> /dev/null` stdc++.lib[…]

(sure, one can edit that and run the linker manually, which works, but that kinda sours the experience)

Let's break this one down.

llvm-config

The main culprit, of course, is this line:

@[Link(ldflags: "`{{LibLLVM::LLVM_CONFIG.id}} --libs --system-libs --ldflags 2> /dev/null`")]
Which declares that, whenever this lib is involved, we should insert a POSIX shell command expansion into the linker command:

  1. starting with a string (LLVM path) pasted into source code, causing funny characters \c→c, \b→backspace, etc. but that's easy to fix with correct interpolation, whatever.
  2. wanting to run llvm-config, which actually would've worked fine with these flags.
  3. trying to redirect stderr into a POSIX-specific device.
  4. expects the result of that command to be expanded dynamically and inserted back.

Sooo yes, we can't do it like this on Windows, at all. I'm kinda blanking on what the alternative solution would be. That's why I created this issue rather than just fixing it.

We need to not rely on shell expansion and in general ldflags is not cross-platform.
We could devise a custom way to do intentional delayed expansion of a subcommand within the linker command, but it's messy.

llvm_ext

Then there's also the other part to this but it's really minor in comparison:

@[Link(ldflags: "#{__DIR__}/ext/llvm_ext.o")]
which

  1. should be llvm_ext.obj
  2. we don't have a Makefile to rely on to pre-build this file from llvm_ext.cc

stdc++.lib

And I guess this shouldn't be there on Windows?

@[Link("stdc++")]

@RX14
Copy link
Contributor

RX14 commented Apr 16, 2020

And I guess this shouldn't be there on Windows?

It shouldn't be there on any platform to be honest, but everything's broken anyway.

Makefiles work on windows with nmake. It'd be neat if we could provide one.

As for llvm-config, it can be run during the compile on windows, just remove the quotes.

@asterite
Copy link
Member

@oprypin Thank you for everything you've done lately for Windows support. It seems you took charge of it and you are doing great! I'm so happy this is almost the final step.

What do you think of compiling it like:

crystal build some_file.cr -LLibLLVM "some linker flags" -LLibLLVMExt "some other linker flags"

That way the linker flags can be specified outside of the code, and they can be computed in any way before passing it to the program.

Do you think that could work?

@RX14
Copy link
Contributor

RX14 commented Apr 16, 2020

@asterite that works as a hack but It's obviously not a long-term solution. Better hacks to Just Make It Link exist already.

@asterite
Copy link
Member

Why is it a hack? What's a way to configure linker flags associated to each lib?

Like with the timestamp macro, instead of computing things in code, which we thought was a great idea, computing them outside of the program is the most flexible way. That's not to say we should drop @[Link], because in most cases it's just mentioning the linked library. But I still think configuring stuff outside of code is the way to go.

@RX14
Copy link
Contributor

RX14 commented Apr 16, 2020

Oh right! I think I misunderstood what you meant! I proposed something similar in #8972 (comment), but I haven't got around to implementing it yet.

Having the defaults baked into code is a must, because otherwise the crystal toolchain is hard to use, but being able to override is also neccesary.

@oprypin
Copy link
Member Author

oprypin commented Apr 16, 2020

I think specifying the libraries to link within the code is very nice.
Right now this works, and I'd hate to lose it: bin/crystal build src/compiler/crystal.cr

So I'm still hoping for a solution that is "nice" in that regard.

That llvm-config command expands to a list of library file names. It could be an interesting exercise to manually declare only the libs that Crystal actually uses.

Btw I don't know if I'm doing something wrong but I get quite different results from llvm-config --libs --system-libs --ldflags on Linux vs Windows:

-L/usr/lib 
-lLLVM-9
-LIBPATH:C:\crystal\llvm-10.0.0.src\Release\lib
C:\crystal\llvm-10.0.0.src\Release\lib\LLVMXRay.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMWindowsManifest.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMTestingSupport.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMTableGen.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMSymbolize.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMDebugInfoPDB.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMOrcJIT.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMOrcError.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMJITLink.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMObjectYAML.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMMIRParser.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMMCA.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMLTO.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMPasses.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMObjCARCOpts.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMLineEditor.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMLibDriver.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMInterpreter.lib C:\crystal\llvm-10.0.0.src\Release\lib\gtest_main.lib C:\crystal\llvm-10.0.0.src\Release\lib\gtest.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMFuzzMutate.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMFrontendOpenMP.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMMCJIT.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMExecutionEngine.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMRuntimeDyld.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMDWARFLinker.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMDlltoolDriver.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMOption.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMDebugInfoGSYM.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMCoverage.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMCoroutines.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMipo.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMInstrumentation.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMVectorize.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMLinker.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMIRReader.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMAsmParser.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMX86Disassembler.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMX86AsmParser.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMX86CodeGen.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMCFGuard.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMGlobalISel.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMSelectionDAG.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMAsmPrinter.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMDebugInfoDWARF.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMCodeGen.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMTarget.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMScalarOpts.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMInstCombine.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMAggressiveInstCombine.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMTransformUtils.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMBitWriter.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMAnalysis.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMProfileData.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMX86Desc.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMObject.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMTextAPI.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMMCParser.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMBitReader.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMCore.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMRemarks.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMBitstreamReader.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMMCDisassembler.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMMC.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMDebugInfoCodeView.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMDebugInfoMSF.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMBinaryFormat.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMX86Utils.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMX86Info.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMSupport.lib C:\crystal\llvm-10.0.0.src\Release\lib\LLVMDemangle.lib
psapi.lib shell32.lib ole32.lib uuid.lib advapi32.lib

I don't know really.

oprypin added a commit to oprypin/crystal that referenced this issue Apr 16, 2020
This is a hack to get the compiler to compile itself on Windows without interventions.

See crystal-lang#9089
@oprypin
Copy link
Member Author

oprypin commented Apr 16, 2020

@oprypin
Copy link
Member Author

oprypin commented Apr 19, 2020

I am looking for a way to avoid declaring the dependency on "stdc++" within Crystal code, when really it's the obj file depending on it.
This came up in my own project as well.
Sure, we can just condition it away on Windows, but that's not even the point.

Is there any way to compile an equivalent of llvm_ext.cc in a way that it declares the dependency on "stdc++"?
My naive attempt of passing cc -c -lstdc++ … -o *.o *.cc didn't help. Would turning it into a .a file help? I have no idea what I'm doing

@oprypin
Copy link
Member Author

oprypin commented Apr 19, 2020

Oh I see. Building a shared library is the only way to achieve that, and I guess we don't want that.

@jan-zajic
Copy link
Contributor

Hi @oprypin i have working compiler on windows for some time, don't remember if i address problems you described here, but linker work just fine for me. Please feel free to see this branch: https://github.com/jan-zajic/crystal/commits/feature/win_preview and see if you already addressed same issues, cherry pick some commits if you like, or get inspired.

For example here are some nmake build scripts: jan-zajic@4e0d1e6 .

Or here jan-zajic@c3f212c (use linker directly on windows and use CRYSTAL_LIBRARY_PATH)
you can see, that i change linker to LINK command directly, because cl is compiler which call linker, like gcc call ld. I change how expansion is handled and LINK have different order of arguments. If you want to try it, see subseqent related commits like jan-zajic@f4077b4 (add missing exe to output filename after change command to linker) and jan-zajic@41f8f38 (use .obj extension on windows to suppress linker warnings).

I'm sorry for late comment, i don't have time to keep up with your contribution speed right now because my regular work...

@oprypin
Copy link
Member Author

oprypin commented Apr 20, 2020

For the most part looks like crystal@master has progressed past the state of your branch (yes, sadly I hadn't seen it), though it has some nice ideas for improvements that I originally managed to do without.

In particular, it doesn't seem like you had a goal of using Crystal on Windows to compile the compiler. This problem is specific to that, so it's also not surprising that it's not addressed in your branch. I have resolved this, anyway, but haven't closed the issue because maybe it's not the best solution. 

@RX14
Copy link
Contributor

RX14 commented Apr 23, 2020

Declaring the stdc++ dependency in crystal code is really the only way to do it. Typically, static libraries would generate a pkgconf file describing their dependencies so that pkg-config could tell the linkers about the dependencies. Clearly, that's overkill here, so it's best to do it in Crystal code.

Obviously it needs to be removed on windows though.

@straight-shoota
Copy link
Member

As far as I can see, this has mostly been resolved by #9106 and some other PRs. Is there anything left to do here?
Maybe the stdc++ thing, but that should be a different issue. Not sure it's worth tracking.

@oprypin oprypin closed this as completed Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants