Fix compilation with LLVM 6.0.1 #6380

felixbuenemann · 2018-07-13T17:04:27Z

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 · 2018-07-13T17:12:36Z

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

felixbuenemann · 2018-07-13T17:14:48Z

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 · 2018-07-13T17:15:13Z

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 · 2018-07-13T17:17:10Z

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

felixbuenemann · 2018-07-13T17:18:30Z

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

felixbuenemann · 2018-07-13T17:26:41Z

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 · 2018-07-13T17:28:01Z

$ 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 · 2018-07-13T17:32:30Z

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

felixbuenemann · 2018-07-13T17:44:59Z

@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 · 2018-07-13T17:53:51Z

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 · 2018-07-13T17:56:26Z

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 · 2018-07-13T18:22:33Z

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

Same failure.

felixbuenemann · 2018-07-13T18:25:50Z

Thanks for the link, that saves me some time.

matiasgarciaisaia · 2018-07-13T18:28:30Z

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 · 2018-07-13T18:30:50Z

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

RX14 · 2018-07-13T18:41:20Z

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 · 2018-07-13T18:44:31Z

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 · 2018-07-13T21:22:42Z

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 · 2018-07-13T21:27:49Z

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 · 2018-07-13T21:35:15Z

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.

This fixes symbol clashes with the LLVM C-API DebugInfo that was added in LLVM 6.0.

RX14 · 2018-07-13T22:50:26Z

@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 · 2018-07-13T22:55:10Z

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

felixbuenemann · 2018-07-13T22:55:18Z

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 · 2018-07-13T23:09:38Z

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

felixbuenemann · 2018-07-13T23:12:28Z

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 · 2018-07-14T11:58:23Z

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 · 2018-07-14T11:56:45Z

src/llvm/ext/llvm_ext.cc

@@ -56,14 +60,16 @@ LLVMDIBuilderRef LLVMNewDIBuilder(LLVMModuleRef mref) {
  return wrap(new DIBuilder(*m));
 }

+#if LLVM_VERSION_LE(5, 9)


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

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.

@RX14 @ysbaddaden I fixed this in #6383.

RX14 · 2018-07-14T12:25:28Z

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

RX14 requested a review from ysbaddaden July 13, 2018 21:23

Fix compilation with LLVM 6.0

070c766

This fixes symbol clashes with the LLVM C-API DebugInfo that was added in LLVM 6.0.

felixbuenemann force-pushed the llvm-6.0-support branch from 595fc48 to 070c766 Compare July 13, 2018 21:49

RX14 approved these changes Jul 13, 2018

View reviewed changes

ysbaddaden approved these changes Jul 14, 2018

View reviewed changes

RX14 merged commit 249a2ed into crystal-lang:master Jul 14, 2018

RX14 added this to the Next milestone Jul 14, 2018

RX14 added kind:bug topic:stdlib labels Jul 14, 2018

felixbuenemann mentioned this pull request Jul 14, 2018

Fix version check for LLVM < 6.0 #6383

Merged

RX14 modified the milestones: Next, 0.26.0 Jul 30, 2018

DomT4 mentioned this pull request Aug 13, 2018

Updates llvm to the current package Homebrew/homebrew-core#31057

Closed

felixbuenemann mentioned this pull request Aug 14, 2018

crystal 0.26.0 Homebrew/homebrew-core#31086

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compilation with LLVM 6.0.1 #6380

Fix compilation with LLVM 6.0.1 #6380

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018

RX14 commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018 •

edited

ilovezfs commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

matiasgarciaisaia commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018 •

edited

RX14 commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018

RX14 commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018 •

edited

RX14 commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018

RX14 left a comment

ysbaddaden commented Jul 14, 2018

ysbaddaden Jul 14, 2018

felixbuenemann Jul 14, 2018

felixbuenemann Jul 14, 2018

RX14 commented Jul 14, 2018

Fix compilation with LLVM 6.0.1 #6380

Fix compilation with LLVM 6.0.1 #6380

Conversation

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018

RX14 commented Jul 13, 2018 • edited

felixbuenemann commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018 • edited

felixbuenemann commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018 • edited

felixbuenemann commented Jul 13, 2018 • edited

felixbuenemann commented Jul 13, 2018 • edited

ilovezfs commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

matiasgarciaisaia commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018 • edited

felixbuenemann commented Jul 13, 2018 • edited

RX14 commented Jul 13, 2018 • edited

felixbuenemann commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018

RX14 commented Jul 13, 2018

RX14 commented Jul 13, 2018

felixbuenemann commented Jul 13, 2018 • edited

RX14 commented Jul 13, 2018 • edited

felixbuenemann commented Jul 13, 2018

RX14 left a comment

Choose a reason for hiding this comment

ysbaddaden commented Jul 14, 2018

ysbaddaden Jul 14, 2018

Choose a reason for hiding this comment

felixbuenemann Jul 14, 2018

Choose a reason for hiding this comment

felixbuenemann Jul 14, 2018

Choose a reason for hiding this comment

RX14 commented Jul 14, 2018

RX14 commented Jul 13, 2018 •

edited

RX14 commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018 •

edited

RX14 commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018 •

edited

RX14 commented Jul 13, 2018 •

edited

felixbuenemann commented Jul 13, 2018 •

edited

RX14 commented Jul 13, 2018 •

edited