Fix compilation with LLVM 6.0.1

[felixbuenemann]

This fixes the following compile error on LLVM 6.0.1 (and 6.0.0):

Using /usr/local/opt/llvm/bin/llvm-config [version=6.0.1]
c++ -c  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc -I/usr/local/Cellar/llvm/6.0.1/include  -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -DNDEBUG   -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
CRYSTAL_CONFIG_PATH="/Users/felix/dev/misc/crystal/src" ./bin/crystal build --mcpu=broadwell --release  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
duplicate symbol _LLVMDIBuilderCreateCompileUnit in:
    /Users/felix/dev/misc/crystal/src/llvm/ext/llvm_ext.o
    /usr/local/Cellar/llvm/6.0.1/lib/libLLVMCore.a(DebugInfo.cpp.o)
duplicate symbol _LLVMDIBuilderFinalize in:
    /Users/felix/dev/misc/crystal/src/llvm/ext/llvm_ext.o
    /usr/local/Cellar/llvm/6.0.1/lib/libLLVMCore.a(DebugInfo.cpp.o)
duplicate symbol _LLVMDIBuilderCreateFile in:
    /Users/felix/dev/misc/crystal/src/llvm/ext/llvm_ext.o
    /usr/local/Cellar/llvm/6.0.1/lib/libLLVMCore.a(DebugInfo.cpp.o)
ld: 3 duplicate symbols for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Error: execution of command failed with code: 1: `cc "${@}" -o '/Users/felix/dev/misc/crystal/.build/crystal'  -rdynamic  /Users/felix/dev/misc/crystal/src/llvm/ext/llvm_ext.o `/usr/local/opt/llvm/bin/llvm-config --libs --system-libs --ldflags 2> /dev/null` -lstdc++ -lpcre -lgc -lpthread /Users/felix/dev/misc/crystal/src/ext/libcrystal.a -levent -liconv -ldl -L/usr/lib -L/usr/local/lib`
make: *** [.build/crystal] Error 1

Since the functions LLVMDIBuilderCreateCompileUnit and LLVMDIBuilderCreateFile exist in LLVM 6.0, but have a much different signature, I chose to instead rename the conflicting methods by adding a "2" suffix, like already done for LLVMSetCurrentDebugLocation.

This is related to #5556.

I'm currently running into issue #6372 during make spec, but it also happens with LLVM 5.0.2.
The specs pass fine if I apply the fix described in #6372 (comment).

[RX14]

But crystal builds on LLVM6 right now, and has done for months on archlinux. What changed?

[felixbuenemann]

I'm not sure – have you tested with 6.0.1 on archlinux?

I'll see if I still have 6.0.0 on my mac to compare.

[RX14]

And none of the arch patches to LLVM 6.0.1 are to anything but the build system: https://git.archlinux.org/svntogit/packages.git/tree/trunk?h=packages/llvm

[RX14]

Did you remember to run make clean after changing LLVM versions? The llvm_ext.o needs to be recompiled.

[felixbuenemann]

I started with a clean slate, make clean and rm -rf ~/.cache/crystal/*.

[felixbuenemann]

Btw. the patch also works against 0.25.1 and HEAD when compiling crystal with the homebrew formula and llvm (currently 6.0.1) instead of llvm@5 (currently 5.0.2).

[RX14]

$ make clean crystal
Using /usr/bin/llvm-config [version=6.0.1]
rm -rf .build
rm -rf ./docs
rm -rf src/llvm/ext/llvm_ext.o
rm -rf src/ext/sigfault.o src/ext/libcrystal.a
g++ -c  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc -I/usr/include -march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong -fno-plt -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -ffunction-sections -fdata-sections -O3 -DNDEBUG  -fno-exceptions -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
cc -fPIC    -c -o src/ext/sigfault.o src/ext/sigfault.c
ar -rcs src/ext/libcrystal.a src/ext/sigfault.o
CRYSTAL_CONFIG_PATH="/data/programming/crystal-lang/crystal/master/src" ./bin/crystal build  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib

$ bin/crystal -v
Using compiled compiler at `.build/crystal'
Crystal 0.25.1+38 [8d28cbb1f] (2018-07-13)

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

works for me. What's different between the homebrew and archlinux sources here? Is this platform-specific weirdness? I hope not.

[felixbuenemann]

Maybe the symbols just don't clash on Linux. Linking on Darwin differs a fair bit from Linux.

[felixbuenemann]

@RX14 If you like I can update the prepare_build step in the bin/ci script so that it uses "llvm" instead of "llvm@5". It's currently using whatever version is used by the crystal formula in homebrew.

[felixbuenemann]

Hmm, this is kind of a chicken-egg problem. The ci script installs crystal from homebrew and then links the matching llvm version that the crystal formula depends on.

In oder to fix that I should update the homebrew formula to build against llvm 6 and they will ask if the patch was upstreamed :-)

What I could do is create a new PR in homebre-core that only updates the dependency but does not yet include the patch and wait for it to fail. Then I could point you at the homebrew-core jenkins build results and then push the patch in the homebrew formula in another commit to show if it fixes the build.

[felixbuenemann]

Oh well, since @ilovezfs mentioned interest in bumping crystal's llvm dependency to 6.0 in comment Homebrew/homebrew-core#28930 (comment) of the 0.25.0 release discussion I might get it through.

[ilovezfs]

See also Homebrew/homebrew-core#29774 (comment)

Same failure.

[felixbuenemann]

Thanks for the link, that saves me some time.

[matiasgarciaisaia]

Same error happens on my macOS 10.12.6, building with the updated LLVM 6.0.1 from homebrew.

With this PR, it builds, and then make spec fails - I think Felix's already aware of that.

[felixbuenemann]

@matiasgarciaisaia do you get the same make spec error as described in #6372?

[RX14]

In oder to fix that I should update the homebrew formula to build against llvm 6 and they will ask if the patch was upstreamed :-)

You don't need the CI to run on LLVM 6 to patch crystal to support LLVM 6 on osx

[felixbuenemann]

You don't need the CI to run on LLVM 6 to patch crystal to support LLVM 6

No, only if you want to reproduce the compile error in the CI on macOS.

[RX14]

Okay, I've looked into this a bit further and i'm okay with this patch. These calls can be replaced with their official counterparts but i'd rather merge a quickfix patch then one that uses the new llvm-c versions. My only feedback is to s/2/Crystal/ in the new method names.

[felixbuenemann]

My only feedback is to s/2/Crystal/ in the new method names.

I did that at first, but then looked at LLVMSetCurrentDebugLocation2 which already uses the "2" suffix to avoid clashing with the existing LLVM LLVMSetCurrentDebugLocation function, so I followed the same naming schema to be consistent.

I'm fine with either convention, but I think using two different naming schemes in the same file is confusing.

[felixbuenemann]

Oh, and I think we should add #include <llvm-c/DebugInfo.h> header for LLVM 6.0, since that's where LLVMDIBuilderFinalize is defined and it's not included by <llvm-c/Core.h>.

I'll push an update for that shortly.

[RX14]

@felixbuenemann it doesn't need to include llvm-c/DebugInfo.h. The technically correct solution would to be to bind LLVMDIBuilderFinalize in LibLLVMExt in < 6 and LibLLVM in > 6, but it really doesn't matter because we always require both libs or neither. Please leave this as-is.

[RX14]

and yes, if we already use the 2 convention, then lets keep it.

[felixbuenemann]

Too late :-( (regarding DebugInfo.h inclusion)

Including the header allows us to spot conflicts like this easier, since they blow up at compile time instead of at link time. It would also have allowed to spot the error on Linux earlier where the symbols somehow didn't clash.

If you still think it should be removed let me now and I'll revert to the previous version.

[RX14]

if that's the case, we should include it in every LLVM version.

[felixbuenemann]

The header was added in LLVM 6.0 so that will be very difficult to achieve ;-)

In the future the bindings should probably be rewritten to use the new LLVM C DebugInfo API instead of the C++ bindings, but that would break backwards compatibility (or there would be two different implementations).

[RX14]

Sorry for my stupid suggestions, this is good as-is.

[ysbaddaden]

This is probably an acceptable quick fix, but since LLVM is adding C bindings for Debug Information, we should evolve our custom set of C++ to match the new API for older LLVM releases, so we don't break anything, and can move on. Hopefully dropping the custom C++ someday.

[ysbaddaden]

Note: there won't be any 5.x other than 5.0. LE(5, 0) is enough, no need to indroduce unknown version numbers :)

[felixbuenemann]

Good catch, I just fixed this locally, but @RX14 merged in parallel before I pushed it.

I'll send a tiny new PR to fix it and reference it here.

[felixbuenemann]

@RX14 @ysbaddaden I fixed this in #6383.

[RX14]

Indeed, this is a good quick fix but I'd love a patch which got the LLVM bindings in-sync.