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

Support LLVM6 #5556

Closed
ysbaddaden opened this issue Jan 8, 2018 · 20 comments
Closed

Support LLVM6 #5556

ysbaddaden opened this issue Jan 8, 2018 · 20 comments

Comments

@ysbaddaden
Copy link
Contributor

LLVM 6.0 was branched, we may start investigating what got broken, and more importantly: what got added to the C API (surprise).

@asterite
Copy link
Member

asterite commented Jan 8, 2018

Surprise?? 😮

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Jan 8, 2018

From an initial attempt (ubuntu trusty, using prebuilt https://apt.llvm.org packages), I got a few hiccups. For example missing support for GCC 4.8.5 (no option -Wdate-time), and a missing symbol (LLVMInitializeInstCombine) probably due to the packages, since we never call the function directly, and fixed with the --link-static flag, such as:

$ make clean crystal \
  LLVM_CONFIG="llvm-config-6.0 --link-static" \
  CXX=clang++-5.0 CC=clang-5.0

I then got actual compatibility issues:

/home/github/crystal/src/llvm/ext/llvm_ext.o: In function `LLVMDIBuilderCreateCompileUnit':
src/llvm/ext/llvm_ext.cc:(.text.LLVMDIBuilderCreateCompileUnit+0x12f): undefined reference to `llvm::DIBuilder::createCompileUnit(unsigned int, llvm::DIFile*, llvm::StringRef, bool, llvm::StringRef, unsigned int, llvm::StringRef, llvm::DICompileUnit::DebugEmissionKind, unsigned long, bool, bool, bool)'
/home/github/crystal/src/llvm/ext/llvm_ext.o: In function `LLVMDIBuilderCreateFunction':
src/llvm/ext/llvm_ext.cc:(.text.LLVMDIBuilderCreateFunction+0xf1): undefined reference to `llvm::DIBuilder::createFunction(llvm::DIScope*, llvm::StringRef, llvm::StringRef, llvm::DIFile*, unsigned int, llvm::DISubroutineType*, bool, bool, unsigned int, llvm::DINode::DIFlags, bool, llvm::MDTupleTypedArrayWrapper<llvm::DITemplateParameter>, llvm::DISubprogram*, llvm::MDTupleTypedArrayWrapper<llvm::DIType>)'
/home/github/crystal/src/llvm/ext/llvm_ext.o: In function `LLVMDIBuilderCreatePointerType':
src/llvm/ext/llvm_ext.cc:(.text.LLVMDIBuilderCreatePointerType+0x52): undefined reference to `llvm::DIBuilder::createPointerType(llvm::DIType*, unsigned long, unsigned int, llvm::Optional<unsigned int>, llvm::StringRef)'
/home/github/crystal/src/llvm/ext/llvm_ext.o: In function `llvm::IRBuilder<llvm::ConstantFolder, llvm::IRBuilderDefaultInserter>::CreateAtomicCmpXchg(llvm::Value*, llvm::Value*, llvm::Value*, llvm::AtomicOrdering, llvm::AtomicOrdering, unsigned char)':
src/llvm/ext/llvm_ext.cc:(.text._ZN4llvm9IRBuilderINS_14ConstantFolderENS_24IRBuilderDefaultInserterEE19CreateAtomicCmpXchgEPNS_5ValueES5_S5_NS_14AtomicOrderingES6_h[_ZN4llvm9IRBuilderINS_14ConstantFolderENS_24IRBuilderDefaultInserterEE19CreateAtomicCmpXchgEPNS_5ValueES5_S5_NS_14AtomicOrderingES6_h]+0x60): undefined reference to `llvm::AtomicCmpXchgInst::AtomicCmpXchgInst(llvm::Value*, llvm::Value*, llvm::Value*, llvm::AtomicOrdering, llvm::AtomicOrdering, unsigned char, llvm::Instruction*)'
/home/github/crystal/src/llvm/ext/llvm_ext.o: In function `LLVMWriteBitcodeWithSummaryToFile':
src/llvm/ext/llvm_ext.cc:(.text.LLVMWriteBitcodeWithSummaryToFile+0xa9): undefined reference to `llvm::WriteBitcodeToFile(llvm::Module const*, llvm::raw_ostream&, bool, llvm::ModuleSummaryIndex const*, bool, std::array<unsigned int, 5ul>*)'

No fun: our custom C bindings for C++ functions are broken. Namely DebugInfo, LTO (I think) and Atomics. Sigh.

Surprise: llvm-c/DebugInfo.h can be found in official C bindings. It's experimental and unstable, but there is some initial, official, C bindings for DIBuilder. Woot! See https://github.com/llvm-mirror/llvm/blob/release_60/include/llvm-c/DebugInfo.h which provides

@asterite
Copy link
Member

asterite commented Jan 8, 2018

Ooooh... I thought the surprise was going to be fun :-(

But well, hopefully little by little they will be completing the C API so that we don't need those ugly wrappers anymore.

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Jan 8, 2018

Sorry, it's a surprise for LLVM maintainers that don't have to maintain C++ anymore!

A better one would have been: compiles just fine 😁

@RX14
Copy link
Contributor

RX14 commented Mar 14, 2018

So LLVM6 has been released, we probably want to start seriously working on this before we're caught out by early adopter distributions like archlinux wanting to upgrade.

@RX14
Copy link
Contributor

RX14 commented Mar 14, 2018

Also:

The Debugify pass was added to opt to facilitate testing of debug info preservation. This pass attaches synthetic DILocations and DIVariables to the instructions in a Module. The CheckDebugify pass determines how much of the metadata is lost.

Hopefully this should help future LLVM releases improve on the pretty bad debug information of crystal binaries, since IIRC the problem is largely LLVM instead of crystal.

@ysbaddaden
Copy link
Contributor Author

I'm feeling lazy this time. If someone wants to take a stab at this.

There are mostly C++ API changes but they seem to touch all the extensions we wrote (debug info, atomics, ...), and the introduction of a very minimal debug info support in the C API (i.e. declare file:line:column locations).

@RX14
Copy link
Contributor

RX14 commented Mar 14, 2018

I'm super confused: LLVMInitializeInstCombine is still in the llvm6 sourcecode and headers, plus apparently I can compile llvm_ext.cc with no changes?

@RX14
Copy link
Contributor

RX14 commented Mar 14, 2018

OK, LLVMInitializeInstCombine is a bug: https://reviews.llvm.org/D44140. Will be fixed for 6.0.1

@RX14
Copy link
Contributor

RX14 commented Mar 16, 2018

LLVM 6 with the LLVMInitializeInstCombine works without any changes from our side:

$ bin/crystal -v
Using compiled compiler at `.build/crystal'
Crystal 0.24.2+184 [863f301cf] (2018-03-16)

LLVM: 6.0.0
Default target: x86_64-pc-linux-gnu

The errors @ysbaddaden saw must have been fixed between the prerelease and release.

@RX14 RX14 closed this as completed Mar 16, 2018
@anatol
Copy link
Contributor

anatol commented Mar 17, 2018

I am building crystal 0.24.2 and I see this weird compilation issue

Using /usr/bin/llvm-config [version=6.0.0]
g++ -c -march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong -fno-plt  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc `/usr/bin/llvm-config --cxxflags`
cc -march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong -fno-plt -fPIC  -D_FORTIFY_SOURCE=2  -c -o src/ext/sigfault.o src/ext/sigfault.c
ar -rcs src/ext/libcrystal.a src/ext/sigfault.o
./bin/crystal build --release --no-debug -D i_know_what_im_doing -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
Using /usr/bin/llvm-config [version=6.0.0]
./bin/crystal docs src/docs_main.cr
Error in src/docs_main.cr:8: while requiring "./csv"

require "./csv"
^

in src/csv.cr:422: while requiring "./csv/**"

require "./csv/**"
^

in src/csv/builder.cr:1: while requiring "csv"

require "csv"
^

in share/crystal/src/csv.cr:59: already initialized constant CSV::DEFAULT_SEPARATOR

  DEFAULT_SEPARATOR  = ','
  ^~~~~~~~~~~~~~~~~

make: *** [Makefile:93: docs] Error 1

@j8r
Copy link
Contributor

j8r commented Mar 17, 2018

already initialized constant CSV::DEFAULT_SEPARATOR - not linked to LLVM6. @anatol, you should report this to a separated issue.

@ghost
Copy link

ghost commented Mar 17, 2018

iirc llvm6 has initial support for coroutines and crystal has them as well?
for reference zig

and the homebrew bottle could be updated because llvm was bumped to v6, v5 is now a versioned formula and installed additionally for anyone upgrading homebrew lately (after llvm version bump, because before the bump llvm was installed as an unversioned formula whoich was v5)
crystal-lang.rb

@RX14
Copy link
Contributor

RX14 commented Mar 17, 2018

@anatol Looks like an issue with CRYSTAL_PATH. How did you work around libLLVM-5.0.so being required by the old compiler and libLLVM-6.0.so being required by the new compiler? The error is likely related to that workaround. A PKGBUILD or something reproducible would help.

EDIT: it definitely is. See

in src/csv.cr

and

in share/crystal/src/csv.cr

@anatol
Copy link
Contributor

anatol commented Mar 17, 2018

I see this DEFAULT_SEPARATOR issue with LLVM5, so indeed it might be a config/build issue. Let's move this discussion to #5835

@foutrelis
Copy link
Contributor

How did you work around libLLVM-5.0.so being required by the old compiler and libLLVM-6.0.so being required by the new compiler?

Copying libLLVM-5.0.so into the chroot before the build started worked out OK. :P

(I did the rebuild for LLVM 6 a couple days after the LLVMInitializeInstCombine fix landed.)

@joshuapinter
Copy link

joshuapinter commented Mar 4, 2019

Likely not the right place for this but it's related.

Trying to build crystal on Ubuntu 18.04 so I can do some additional testing for PR #7507 and getting the following error:

$ make
Using /usr/bin/llvm-config-6.0 [version=6.0.0]
CRYSTAL_CONFIG_PATH="/home/joshuapinter/Development/crystal/src" CRYSTAL_CONFIG_BUILD_COMMIT="88a238e2d" ./bin/crystal build -D preview_overflow -D compiler_rt  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
L-L-V-M-5858P-assR-egistry.o: In function `initialize_inst_combine':
/home/joshuapinter/Development/crystal/src/llvm/pass_registry.cr:11: undefined reference to `LLVMInitializeInstCombine'
collect2: error: ld returned 1 exit status
Error: execution of command failed with code: 1: `cc "${@}" -o '/home/joshuapinter/Development/crystal/.build/crystal'  -rdynamic  /home/joshuapinter/Development/crystal/src/llvm/ext/llvm_ext.o `/usr/bin/llvm-config-6.0 --libs --system-libs --ldflags 2> /dev/null` -lstdc++ -lpcre -lm -lgc -lpthread /home/joshuapinter/Development/crystal/src/ext/libcrystal.a -levent -lrt -ldl -L/usr/lib -L/usr/local/lib`
Makefile:122: recipe for target '.build/crystal' failed
make: *** [.build/crystal] Error 1

Version

$ crystal --version
Crystal 0.27.2 [60760a546] (2019-02-05)

LLVM: 4.0.0
Default target: x86_64-unknown-linux-gnu

It looks like I've got LLVM 4.0.0 installed but using llvm-config-6.0.

Sorry, a little out of my element here. Any help would be great.

@straight-shoota
Copy link
Member

@joshuapinter Please ask this question in the forum or on Gitter/IRC.

@joshuapinter
Copy link

@straight-shoota Will do, thanks!

@girng
Copy link
Contributor

girng commented Oct 1, 2019

@straight-shoota This is a very nasty bug, not a question for the forum (where developers rarely check). It needs to be fixed and addressed by core developers, we can't even compile Crystal when we follow the guide. I made an official issue about it and I sincerely hope it can be fixed permanently, not randomly show up a year later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants