Add support for LLVM 3.8 #2993

omarroth · 2016-07-13T21:55:11Z

Incompatible with 3.7, but is backwards compatible for currently supported versions of LLVM. Closes #1614. It is also necessary for #26, as TLS is now supported on cygwin targets with emutls.

Build times using 3.8 appear to be slower on average.

sdogruyol · 2016-07-14T08:43:12Z

Wow, this is amazing 💚

jhass · 2016-07-14T08:57:05Z

@asterite so, do you want to retain 3.5 compatibility? If so we probably need to figure out a way to test against multiple LLVM versions on CI.

ozra · 2016-07-14T10:25:16Z

Hurray!

lbguilherme · 2016-07-14T11:04:35Z

Great job!!

Builds using 3.8 appear to be slower on average.

You mean that the build time is slower or that the resulting compiled binary is slower?

omarroth · 2016-07-14T12:46:05Z

@lbguilherme Sorry about that, build times appear slower.
samples/2048.cr:

1.00s user 0.52s system 144% cpu 1.050 total

Compared with installed crystal:

0.48s user 0.76s system 162% cpu 0.762 total

And then samples/conway.cr:

0.86s user 0.40s system 139% cpu 0.904 total

vs.

0.31s user 0.28s system 118% cpu 0.495 total

The builds themselves don't appear to be any slower.

jhass · 2016-07-14T13:11:53Z

That is with --release everywhere? (while compiling the new LLVM 3.8 based compiler too)?

ysbaddaden · 2016-07-14T13:17:54Z

Thanks for the hard work!

We currently support both LLVM 3.5 and 3.6. Now with 3.8 it's a little difficult to understand what part is for what version. Most notably the default is kept to LLVM 3.6, and it's not clear from reading the code since there are no HAVE_LLVM_36 definitions. Maybe the default should be LLVM 3.8 now (so it could also target LLVM 3.9).

asterite · 2016-07-14T15:24:08Z

@omarroth This is amazing! And travis is happy! You are a hero! ❤️❤️❤️

I would personally drop support for LLVM < 3.8, since we are distributing LLVM libs with Crystal so there shouldn't be any problem with that.

Can you check build times by passing -s when compiling programs? Then we'll see which part gets slower.

jhass · 2016-07-14T15:53:01Z

I can update the docker images for Travis to Ubuntu xenial (16.04), if I find a 32bit image, which would give 3.8: http://packages.ubuntu.com/xenial/llvm However trusty (14.04) only has 3.6: http://packages.ubuntu.com/trusty-updates/llvm-3.6

For Debian, stretch (testing) has 3.8: https://packages.debian.org/stretch/llvm-3.8, jessie (stable) only has 3.5 still: https://packages.debian.org/jessie/llvm (https://packages.debian.org/stretch/llvm-3.6, https://packages.debian.org/stretch/llvm-3.7) However LLVM seems to still have "nightlies" for jessie for 3.8: http://apt.llvm.org/

The clang package from http://llvm.org/releases/download.html for CentOS should include LLVM too, CentOS 6 doesn't seem to have LLVM at all, CentOS 7 has 3.4 still http://centos-packages.com/7/package/llvm/ EPEL isn't any help https://dl.fedoraproject.org/pub/epel/7/x86_64/l/ Whether we can use the official package instead of updating our custom omnibus version is certainly worth an investigation. @asterite / @waj Since you do the release process, do you want to look into that?

Regarding OS X, the homebrew formula seems to still be at LLVM 3.6? https://github.com/Homebrew/homebrew-core/blob/master/Formula/llvm.rb but there's this: https://github.com/Homebrew/homebrew-versions/blob/master/llvm38.rb Is this easily usable and we just have to brew install llvm3.8 on Travis?

jhass · 2016-07-14T16:35:08Z

I get a segfault with this branch when running bin/crystal build -d samples/2048.cr after building the compiler against LLVM 3.8.

On the plus side this seems to fix #2789, so we can switch Travis to std_spec crystal spec.

ysbaddaden · 2016-07-14T18:57:46Z

LLVM is providing binaries and repositories for major Linux distributions (Debian, Ubuntu, Fedora, Suse) as well as FreeBSD, so it's not thard to compile Crystal if LLVM 3.8 isn't in the official repositories.

Alpine provides LLVM 3.8 in edge so I should be capable to compile against it. I may struggle a bit since they didn't distribute static libraries for LLVM 3.6, but I'll eventually bootstrap a new compiler.

ozra · 2016-07-14T19:12:10Z

@ysbaddaden - I might remember wrong - but I think there are static libs for 3.8 edge in Alpine (but not earlier versions).

omarroth · 2016-07-15T04:56:50Z

@jhass I wasn't getting a segfault when passing -d to the compiler, but I did manage to fix a problem I was having, so you might try it again and see if it works for you.
I also changed the way crystal interfaces with LLVM a bit, not anything major, but it is enough to break the build on 3.6 (unless there is way to detect the current version of LLVM inside crystal).
@ysbaddaden I added a HAVE_LLVM_OR_LOWER flag for currently supported versions, hopefully that is a little bit clearer.

jhass · 2016-07-15T09:30:10Z

Yes, that indeed seems to have fixed the segfault for me.

Let's try this for CI:

diff --git a/bin/ci b/bin/ci
index 991c0f8..078819f 100755
--- a/bin/ci
+++ b/bin/ci
@@ -147,9 +147,10 @@ EOF
 prepare_build() {
   on_linux verify_linux_environment

-  on_linux docker pull "jhass/crystal-build-$ARCH"
+  on_linux docker pull "jhass/crystal-build-$ARCH:llvm38"

   on_osx brew install libevent crystal-lang
+  on_osx /bin/bash -c "'curl \"http://llvm.org/releases/3.8.0/clang+llvm-3.8.0-x86_64-apple-darwin.tar.xz\" | tar xz -C /tmp'"
   on_osx /bin/bash -c "'curl \"http://crystal-lang.s3.amazonaws.com/llvm/llvm-3.5.0-1-darwin-x86_64.tar.gz\" | tar xz -C /tmp'"
 }

@@ -169,7 +170,7 @@ with_build_env() {
     "jhass/crystal-build-$ARCH" \
     "$ARCH_CMD" /bin/bash -c "'$command'"

-  on_osx PATH="/tmp/llvm-3.5.0-1/bin:\$PATH" \
+  on_osx PATH="/tmp/clang+llvm-3.8.0-x86_64-apple-darwin/bin:\$PATH" \
     CRYSTAL_CACHE_DIR="/tmp/crystal" \
     /bin/bash -c "'$command'"

@@ -220,4 +221,4 @@ case $command in
       exit 1
     fi
     ;;
-esac
\ No newline at end of file
+esac

jhass · 2016-07-15T16:17:35Z

Take two:

diff --git a/bin/ci b/bin/ci
index 3974350..a4b1c84 100755
--- a/bin/ci
+++ b/bin/ci
@@ -84,7 +84,7 @@ prepare_system() {

 build() {
   with_build_env 'make std_spec clean'
-  with_build_env 'make crystal std_spec compiler_spec doc'
+  with_build_env 'make crystal spec doc'
   with_build_env 'find samples -name "*.cr" | xargs -L 1 ./bin/crystal build --no-codegen'
   with_build_env './bin/crystal tool format --check'
 }
@@ -166,10 +166,10 @@ with_build_env() {
     -v $(pwd):/mnt \
     -w /mnt \
     -e LIBRARY_PATH="/opt/crystal/embedded/lib/" \
-    "jhass/crystal-build-$ARCH" \
+    "jhass/crystal-build-$ARCH:llvm38" \
     "$ARCH_CMD" /bin/bash -c "'$command'"

-    on_osx PATH="/tmp/clang+llvm-3.8.0-x86_64-apple-darwin/bin:\$PATH" \
+  on_osx PATH="/tmp/clang+llvm-3.8.0-x86_64-apple-darwin/bin:\$PATH" \
     CRYSTAL_CACHE_DIR="/tmp/crystal" \
     /bin/bash -c "'$command'"

@@ -220,4 +220,4 @@ case $command in
       exit 1
     fi
     ;;
-esac
\ No newline at end of file
+esac

I'll have leave the warnings in https://travis-ci.org/crystal-lang/crystal/jobs/145038717#L166 to you and we need an OS X person to figure out https://travis-ci.org/crystal-lang/crystal/jobs/145038718#L110

Also please run the formatter https://travis-ci.org/crystal-lang/crystal/jobs/145038717#L441

omarroth · 2016-07-15T17:02:19Z

I'm inclined to think that the warnings on OS X are caused by an outdated compiler as they don't appear on either of the linux builds.

jhass · 2016-07-15T17:21:24Z

Let's try this for the OpenSSL failures:

diff --git a/src/openssl/ssl/context.cr b/src/openssl/ssl/context.cr
index 4bb5dba..3136ab2 100644
--- a/src/openssl/ssl/context.cr
+++ b/src/openssl/ssl/context.cr
@@ -138,6 +138,9 @@ abstract class OpenSSL::SSL::Context

     set_default_verify_paths

+    # Reset any default options, since they're version/OS dependent
+    LibSSL.ssl_ctx_ctrl(@handle, LibSSL::SSL_CTRL_CLEAR_OPTIONS, 0xFFFFFFFF, nil)
+
     add_options(OpenSSL::SSL::Options.flags(
       ALL,
       NO_SSLV2,

jhass · 2016-07-15T18:29:13Z

Meh, looks like we can't unset NO_SSLV3 on 16.04, so this way then I guess:

diff --git a/spec/std/openssl/ssl/context_spec.cr b/spec/std/openssl/ssl/context_spec.cr
index df14afa..3651eee 100644
--- a/spec/std/openssl/ssl/context_spec.cr
+++ b/spec/std/openssl/ssl/context_spec.cr
@@ -80,21 +80,24 @@ describe OpenSSL::SSL::Context do
   it "adds options" do
     context = OpenSSL::SSL::Context::Client.new
     context.remove_options(context.options) # reset
-    context.add_options(OpenSSL::SSL::Options::ALL).should eq(OpenSSL::SSL::Options::ALL)
+    default_options = context.options # options we can't unset
+    context.add_options(OpenSSL::SSL::Options::ALL).should eq(default_options | OpenSSL::SSL::Options::ALL)
     context.add_options(OpenSSL::SSL::Options.flags(NO_SSLV2, NO_SSLV3))
            .should eq(OpenSSL::SSL::Options.flags(ALL, NO_SSLV2, NO_SSLV3))
   end

   it "removes options" do
     context = OpenSSL::SSL::Context::Client.insecure
-    context.add_options(OpenSSL::SSL::Options.flags(ALL, NO_SSLV2))
-    context.remove_options(OpenSSL::SSL::Options::ALL).should eq(OpenSSL::SSL::Options::NO_SSLV2)
+    default_options = context.options
+    context.add_options(OpenSSL::SSL::Options.flags(NO_TLSV1, NO_SSLV2))
+    context.remove_options(OpenSSL::SSL::Options::NO_TLSV1).should eq(default_options | OpenSSL::SSL::Options::NO_SSLV2)
   end

   it "returns options" do
     context = OpenSSL::SSL::Context::Client.insecure
+    default_options = context.options
     context.add_options(OpenSSL::SSL::Options.flags(ALL, NO_SSLV2))
-    context.options.should eq(OpenSSL::SSL::Options.flags(ALL, NO_SSLV2))
+    context.options.should eq(default_options | OpenSSL::SSL::Options.flags(ALL, NO_SSLV2))
   end

   it "adds modes" do
diff --git a/src/openssl/ssl/context.cr b/src/openssl/ssl/context.cr
index 3136ab2..4bb5dba 100644
--- a/src/openssl/ssl/context.cr
+++ b/src/openssl/ssl/context.cr
@@ -138,9 +138,6 @@ abstract class OpenSSL::SSL::Context

     set_default_verify_paths

-    # Reset any default options, since they're version/OS dependent
-    LibSSL.ssl_ctx_ctrl(@handle, LibSSL::SSL_CTRL_CLEAR_OPTIONS, 0xFFFFFFFF, nil)
-
     add_options(OpenSSL::SSL::Options.flags(
       ALL,
       NO_SSLV2,

omarroth · 2016-07-15T20:21:30Z

On the XCode7.3 build there seems to be a version conflict. Clang is looking for a 10.10 SDK but the worker is running 10.11.

instance: 9a8a3148-3b97-412d-98e8-b28ffb11fecc:travis-ci-osx10.11-xcode7.3-1467942550

clang: warning: no such sysroot directory: '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk'

Besides changing the version of the worker, it might help to build LLVM on 10.11 and pulling from there instead.

jhass · 2016-07-15T20:47:26Z

So back to pushing some custom build to S3? :( Why's there no proper binary package manager for OS X, sigh.

omarroth · 2016-07-15T21:21:11Z

My mistake, pulling from homebrew works fine.

jhass · 2016-07-15T23:14:06Z

bin/ci

    "$ARCH_CMD" /bin/bash -c "'$command'"

-  on_osx PATH="/tmp/llvm-3.5.0-1/bin:\$PATH" \
+    on_osx PATH="/usr/local/opt/llvm/bin:\$PATH" \


Fix the indentation here and then I think this should be good to go.

david50407 · 2016-07-17T08:15:06Z

After this patch, I will keep supporting for Windows porting. ❤️

jhass · 2016-07-17T08:16:57Z

Done for me, @asterite anything you still find missing?

asterite · 2016-07-17T16:11:59Z

@jhass I need to review this with @waj, check how to integrate it with omnibus, etc.

jhass · 2016-07-17T16:13:38Z

etc? because integrating it into omnibus doesn't block this from being merged, it only blocks the next release until that's done.

asterite · 2016-07-17T16:21:30Z

If we are sure that we can provide LLVM 3.8 with omnibus, then I guess we can merge this. My only worry was: we merge this, later we have trouble making omnibus work with this, we can't release.

jhass · 2016-07-17T16:23:52Z

Can't we just pull in the latest official tarballs for each architecture on omnibus? Also if I follow correctly this is still compatible to 3.5, just not 3.6 or 3.7.

asterite · 2016-07-17T16:34:04Z

Ah! Then yes, let's merge :-)

jhass · 2016-07-17T17:29:59Z

Actually I just tried locally and it no longer compiles against 3.5 for me. @omarroth would that be easily fixable?

omarroth · 2016-07-17T18:32:06Z

@jhass This section and this section would need to be able to change depending on the LLVM version used. Everything else should be backwards-compatible. I'm not quite sure how that should be handled.

jhass · 2016-07-17T18:37:14Z

That should be possible with shelling out to llvm-config at compile time. The OpenSSL binding provides an example to something very similar, see https://github.com/crystal-lang/crystal/blob/master/src/openssl/lib_ssl.cr#L169-L177 and https://github.com/crystal-lang/crystal/blob/master/src/openssl/ssl/context.cr#L281

However I'm getting errors compiling the extension:

$ llvm-config --version
3.5.2
$ make clean
$ make
clang++ -c -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc `/usr/bin/llvm-config --cxxflags`
clang -fPIC   -c -o src/ext/sigfault.o src/ext/sigfault.c
ar -rcs src/ext/libcrystal.a src/ext/sigfault.o
src/llvm/ext/llvm_ext.cc:383:28: error: temporary of type 'llvm::MDNode' has private destructor
  Node->replaceAllUsesWith(unwrapDI<MDNode>(New));
                           ^
src/llvm/ext/llvm_ext.cc:55:18: note: expanded from macro 'unwrapDI'
#define unwrapDI unwrapDIptr
                 ^
/usr/include/llvm/IR/Metadata.h:114:3: note: implicitly declared private here
  ~MDNode();
  ^
src/llvm/ext/llvm_ext.cc:383:28: error: no viable conversion from 'llvm::MDNode' to 'llvm::Value *'
  Node->replaceAllUsesWith(unwrapDI<MDNode>(New));
                           ^~~~~~~~~~~~~~~~~~~~~
src/llvm/ext/llvm_ext.cc:55:18: note: expanded from macro 'unwrapDI'
#define unwrapDI unwrapDIptr
                 ^
/usr/include/llvm/IR/Value.h:238:34: note: passing argument to parameter 'V' here
  void replaceAllUsesWith(Value *V);
                                 ^
src/llvm/ext/llvm_ext.cc:42:14: error: no matching conversion for functional-style cast from 'llvm::MDNode *' to 'llvm::MDNode'
  return v ? T(unwrap<MDNode>(v)) : T();
             ^~~~~~~~~~~~~~~~~~~
src/llvm/ext/llvm_ext.cc:383:28: note: in instantiation of function template specialization 'unwrapDIptr<llvm::MDNode>' requested here
  Node->replaceAllUsesWith(unwrapDI<MDNode>(New));
                           ^
src/llvm/ext/llvm_ext.cc:55:18: note: expanded from macro 'unwrapDI'
#define unwrapDI unwrapDIptr
                 ^
/usr/include/llvm/IR/Metadata.h:75:3: note: candidate constructor not viable: no known conversion from 'llvm::MDNode *' to 'const llvm::MDNode' for 1st argument;
      dereference the argument with *
  MDNode(const MDNode &) LLVM_DELETED_FUNCTION;
  ^
/usr/include/llvm/IR/Metadata.h:116:3: note: candidate constructor not viable: requires 3 arguments, but 1 was provided
  MDNode(LLVMContext &C, ArrayRef<Value*> Vals, bool isFunctionLocal);
  ^
3 errors generated.
make: *** [Makefile:77: src/llvm/ext/llvm_ext.o] Error 1

asterite · 2016-07-17T19:20:21Z

I don't think keeping LLVM 3.5 compatibility is something that we need to do. I don't mind dropping it. In fact, keeping compatibility with LLVM 3.6 or 3.7 also doesn't matter, we should move forward and try to use the latest version of LLVM. Specially because we provide LLVM with the compiler.

jhass · 2016-07-17T19:23:26Z

I think keeping compatibility in the short term is good, it allows us to move forward without blocking on necessary updates to the build infrastructure. It's a chicken-egg problem otherwise.

omarroth · 2016-07-17T22:46:37Z

The problem I'm having with macros though is that the generated code only seems to be callable in the same file. Something like this:

{% if `(command -v llvm-config-3.6 || command -v llvm-config-36 || command -v llvm-config-3 || command -v llvm-config-35) > /dev/null && printf %s true || printf %s false` == "true" %}
lib LibLLVMExt
  HAVE_LLVM_36_OR_LOWER = true
end
{% else %}
lib LibLLVMExt
  HAVE_LLVM_36_OR_LOWER = false
end
{% end %}

Throws an undefined constant error or appears to produce nothing when used elsewhere.

{% if LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
fn_metadata = di_builder.create_function(scope, target_def.name, target_def.name, scope,
  location.line_number, fun_metadata_type, 1, 1, location.line_number, 0_u32, 0, context.fun)
{% else %}
fn_metadata = di_builder.create_function(scope, target_def.name, target_def.name, scope,
  location.line_number, fun_metadata_type, true, true, location.line_number, 0_u32, false, context.fun)
{% end %}
fun_metadatas[context.fun] = fn_metadata

Produces

 undefined local variable or method 'fn_metadata' (did you mean 'fun_metadatas'?)

      fun_metadatas[context.fun] = fn_metadata
                                   ^~~~~~~~~~~

jhass · 2016-07-17T22:57:27Z

Try wrapping it into another {%begin%}:

{% begin %}
{% if LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
fn_metadata = di_builder.create_function(scope, target_def.name, target_def.name, scope,
  location.line_number, fun_metadata_type, 1, 1, location.line_number, 0_u32, 0, context.fun)
{% else %}
fn_metadata = di_builder.create_function(scope, target_def.name, target_def.name, scope,
  location.line_number, fun_metadata_type, true, true, location.line_number, 0_u32, false, context.fun)
{% end %}
fun_metadatas[context.fun] = fn_metadata
{% end %}

omarroth · 2016-07-17T23:41:19Z

That works fine, other problem is when doing something like this:

  {% if LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
  def insert_declare_at_end(storage, var_info, expr, block)
    LibLLVMExt.di_builder_insert_declare_at_end(self, storage, var_info, expr, block)
  end
  {% else %}
  def insert_declare_at_end(storage, var_info, expr, dl, block)
    LibLLVMExt.di_builder_insert_declare_at_end(self, storage, var_info, expr, dl, block)
  end
  {% end %}

Which outputs

undefined constant LibLLVMExt::HAVE_LLVM_36_OR_LOWER

  {% if LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

ysbaddaden · 2016-07-18T07:55:08Z

Honestly, I see no reason to keep compatibility with LLVM 3.5 but lose compatibility with LLVM 3.6...

jhass · 2016-07-18T08:49:05Z

@omarroth I have a suspicion or two, but it's very hard to tell without the whole context, can you share your work in progress somewhere?

@ysbaddaden it allows us to merge this now rather than when somebody gets around to update omnibus and merging it now also makes testing easier while updating omnibus. It would be a shame to see this sitting around for a couple of weeks and eventually get out of date and getting conflicts.

omarroth · 2016-07-18T12:35:57Z

@jhass Have a look here.

@ysbaddaden The hope is to support both 3.5 and 3.6.

ysbaddaden · 2016-07-18T13:00:32Z

@jhass I forgot omnibus was still using 3.5...

jhass · 2016-07-18T13:34:49Z

@omarroth The issue can be resolved with a couple explicit requires to have explicit load order. While at it I redesigned the whole detection a bit into a more general construct:

diff --git a/src/compiler/crystal/codegen/debug.cr b/src/compiler/crystal/codegen/debug.cr
index e7c578e..587a14e 100644
--- a/src/compiler/crystal/codegen/debug.cr
+++ b/src/compiler/crystal/codegen/debug.cr
@@ -127,7 +127,7 @@ module Crystal
       expr = di_builder.create_expression(nil, 0)

       {% begin %}
-      {% if LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
+      {% if LibLLVM::IS_36 || LibLLVM::IS_35 %}
       declare = di_builder.insert_declare_at_end(alloca, var, expr, alloca_block)
       {% else %}
       declare = di_builder.insert_declare_at_end(alloca, var, expr, builder.current_debug_location, alloca_block)
@@ -211,9 +211,9 @@ module Crystal
       file, dir = file_and_dir(filename)
       scope = di_builder.create_file(file, dir)
       {% begin %}
-      {% if LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
+      {% if LibLLVM::IS_36 || LibLLVM::IS_35 %}
       fn_metadata = di_builder.create_function(scope, MAIN_NAME, MAIN_NAME, scope,
-        0, fun_metadata_type, 1, 1, 0, 0_u32, 0, main_fun)      
+        0, fun_metadata_type, 1, 1, 0, 0_u32, 0, main_fun)
       {% else %}
       fn_metadata = di_builder.create_function(scope, MAIN_NAME, MAIN_NAME, scope,
         0, fun_metadata_type, true, true, 0, 0_u32, false, main_fun)
@@ -229,7 +229,7 @@ module Crystal
       file, dir = file_and_dir(location.filename)
       scope = di_builder.create_file(file, dir)
       {% begin %}
-      {% if LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
+      {% if LibLLVM::IS_36 || LibLLVM::IS_35 %}
       fn_metadata = di_builder.create_function(scope, target_def.name, target_def.name, scope,
         location.line_number, fun_metadata_type, 1, 1, location.line_number, 0_u32, 0, context.fun)
       {% else %}
diff --git a/src/llvm/di_builder.cr b/src/llvm/di_builder.cr
index aab951a..4923b31 100644
--- a/src/llvm/di_builder.cr
+++ b/src/llvm/di_builder.cr
@@ -1,3 +1,5 @@
+require "./lib_llvm"
+
 struct LLVM::DIBuilder
   def initialize(llvm_module)
     @unwrap = LibLLVMExt.create_di_builder(llvm_module)
@@ -41,7 +43,7 @@ struct LLVM::DIBuilder
     LibLLVMExt.di_builder_create_expression(self, addr, length)
   end

-  {% if LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
+  {% if LibLLVM::IS_36 || LibLLVM::IS_35 %}
   def insert_declare_at_end(storage, var_info, expr, block)
     LibLLVMExt.di_builder_insert_declare_at_end(self, storage, var_info, expr, block)
   end
diff --git a/src/llvm/lib_llvm.cr b/src/llvm/lib_llvm.cr
index 6a3a4e0..40636d2 100644
--- a/src/llvm/lib_llvm.cr
+++ b/src/llvm/lib_llvm.cr
@@ -1,7 +1,27 @@
 require "./enums"

-@[Link("stdc++")]
-@[Link(ldflags: "`$(command -v llvm-config-3.6 || command -v llvm-config36 || command -v llvm-config-3.5 || command -v llvm-config35 || command -v llvm-config) --libs --system-libs --ldflags 2> /dev/null`")]
+{% begin %}
+lib LibLLVM
+  LLVM_CONFIG = {{`command -v llvm-config-3.8 || command -v llvm-config38 || (command -v llvm-config > /dev/null && (case "$(llvm-config --version)" in 3.8*) command -v llvm-config;; *) false;; esac)) || command -v llvm-config-3.6 || command -v llvm-config36 || command -v llvm-config-3.5 || command -v llvm-config35 || command -v llvm-config `.chomp.stringify}}
+end
+{% end %}
+
+{% begin %}
+  @[Link("stdc++")]
+  @[Link(ldflags: "`{{LibLLVM::LLVM_CONFIG.id}} --libs --system-libs --ldflags 2> /dev/null`")]
+  lib LibLLVM
+    VERSION = {{`#{LibLLVM::LLVM_CONFIG} --version`.chomp.stringify}}
+  end
+{% end %}
+
+{% begin %}
+  lib LibLLVM
+    IS_38 = {{LibLLVM::VERSION.starts_with?("3.8")}}
+    IS_36 = {{LibLLVM::VERSION.starts_with?("3.6")}}
+    IS_35 = {{LibLLVM::VERSION.starts_with?("3.5")}}
+  end
+{% end %}
+
 lib LibLLVM
   type ContextRef = Void*
   type ModuleRef = Void*
diff --git a/src/llvm/lib_llvm_ext.cr b/src/llvm/lib_llvm_ext.cr
index eaf0c78..fabbd34 100644
--- a/src/llvm/lib_llvm_ext.cr
+++ b/src/llvm/lib_llvm_ext.cr
@@ -1,3 +1,5 @@
+require "./lib_llvm"
+
 @[Link(ldflags: "#{__DIR__}/ext/llvm_ext.o")]
 lib LibLLVMExt
   type DIBuilder = Void*
@@ -5,7 +7,7 @@ lib LibLLVMExt

   fun create_di_builder = LLVMNewDIBuilder(LibLLVM::ModuleRef) : DIBuilder
   fun di_builder_finalize = LLVMDIBuilderFinalize(DIBuilder)
-{% if LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
+{% if LibLLVM::IS_36 || LibLLVM::IS_35 %}
   fun di_builder_create_function = LLVMDIBuilderCreateFunction(
                                                                builder : DIBuilder, scope : Metadata, name : LibC::Char*,
                                                                linkage_name : LibC::Char*, file : Metadata, line : LibC::UInt,
@@ -42,7 +44,7 @@ lib LibLLVMExt
                                                                           name : LibC::Char*, file : Metadata, line : LibC::UInt, type : Metadata,
                                                                           always_preserve : LibC::Int, flags : LibC::UInt, arg_no : LibC::UInt) : Metadata

-{% LibLLVMExt::HAVE_LLVM_36_OR_LOWER %}
+{% if LibLLVM::IS_36 || LibLLVM::IS_35 %}
   fun di_builder_insert_declare_at_end = LLVMDIBuilderInsertDeclareAtEnd(builder : DIBuilder,
                                                                          storage : LibLLVM::ValueRef,
                                                                          var_info : Metadata,
@@ -89,12 +91,3 @@ lib LibLLVMExt
   fun set_current_debug_location = LLVMSetCurrentDebugLocation2(LibLLVM::BuilderRef, LibC::Int, LibC::Int, Metadata, Metadata)
 end

-{% if `(command -v llvm-config-3.6 || command -v llvm-config-36 || command -v llvm-config-3 || command -v llvm-config-35) > /dev/null && printf %s true || printf %s false` == "true" %}
-lib LibLLVMExt
-  HAVE_LLVM_36_OR_LOWER = true
-end
-{% else %}
-lib LibLLVMExt
-  HAVE_LLVM_36_OR_LOWER = false
-end
-{% end %}
\ No newline at end of file

That patch is against your -fix branch.

omarroth · 2016-07-18T14:09:42Z

LGTM

jhass · 2016-07-18T14:12:08Z

src/llvm/lib_llvm.cr

-@[Link(ldflags: "`$(command -v llvm-config-3.6 || command -v llvm-config36 || command -v llvm-config-3.5 || command -v llvm-config35 || command -v llvm-config) --libs --system-libs --ldflags 2> /dev/null`")]
+{% begin %}
+lib LibLLVM
+  LLVM_CONFIG = {{`command -v llvm-config-3.8 || command -v llvm-config38 || command -v llvm-config-3.6 || command -v llvm-config36 || command -v llvm-config-3.5 || command -v llvm-config35 || command -v llvm-config`.chomp.stringify}}


I got an improved version:

LLVM_CONFIG = {{`command -v llvm-config-3.8 || command -v llvm-config38 || (command -v llvm-config > /dev/null && (case "$(llvm-config --version)" in 3.8*) command -v llvm-config;; *) false;; esac)) || command -v llvm-config-3.6 || command -v llvm-config36 || command -v llvm-config-3.5 || command -v llvm-config35 || command -v llvm-config `.chomp.stringify}}

This should

prefer a side installed LLVM 3.8 over any other

prefer the main LLVM if it's 3.8 already

prefer a side installed 3.6 over the main LLVM not being 3.8 or a side installed 3.5

prefer a side installed 3.5 over the main LLVM not being 3.8

fallback to the main LLVM

jhass · 2016-07-18T14:28:05Z

Build failed due to formatting changes, please run the formatter.

jhass · 2016-07-18T18:48:09Z

Thank you!

jhass reviewed Jul 15, 2016
View reviewed changes

jhass added the topic:compiler label Jul 17, 2016

jhass assigned waj Jul 17, 2016

jhass reviewed Jul 18, 2016
View reviewed changes

jhass mentioned this pull request Jul 18, 2016

Crystal does not compile with the latest LLVM version #1614

Closed

omarroth added 2 commits July 18, 2016 10:12

Add support for LLVM 3.8

53f1977

Fix build

0e8a04b

jhass merged commit 5859965 into crystal-lang:master Jul 18, 2016

jhass unassigned waj Jul 18, 2016

jhass added this to the 0.19.0 milestone Jul 18, 2016

asterite pushed a commit that referenced this pull request Jul 18, 2016

Fixed Makefile to work with LLVM 3.8. Related to #2993

19ddb4c

omarroth deleted the llvm-3.8 branch July 19, 2016 04:18

ysbaddaden mentioned this pull request Sep 19, 2016

Can't compile with --debug when IO::ByteFormat is memoized #3320

Closed

Add support for LLVM 3.8 #2993

Add support for LLVM 3.8 #2993

Conversation

omarroth commented Jul 13, 2016 • edited Loading

sdogruyol commented Jul 14, 2016

jhass commented Jul 14, 2016

ozra commented Jul 14, 2016

lbguilherme commented Jul 14, 2016

omarroth commented Jul 14, 2016 • edited Loading

jhass commented Jul 14, 2016

ysbaddaden commented Jul 14, 2016

asterite commented Jul 14, 2016

jhass commented Jul 14, 2016

jhass commented Jul 14, 2016

ysbaddaden commented Jul 14, 2016

ozra commented Jul 14, 2016 • edited Loading

omarroth commented Jul 15, 2016 • edited Loading

jhass commented Jul 15, 2016

jhass commented Jul 15, 2016 • edited Loading

omarroth commented Jul 15, 2016

jhass commented Jul 15, 2016

jhass commented Jul 15, 2016

omarroth commented Jul 15, 2016

jhass commented Jul 15, 2016

omarroth commented Jul 15, 2016

jhass Jul 15, 2016

Choose a reason for hiding this comment

david50407 commented Jul 17, 2016

jhass commented Jul 17, 2016

asterite commented Jul 17, 2016

jhass commented Jul 17, 2016

asterite commented Jul 17, 2016

jhass commented Jul 17, 2016 • edited Loading

asterite commented Jul 17, 2016

jhass commented Jul 17, 2016

omarroth commented Jul 17, 2016 • edited Loading

jhass commented Jul 17, 2016 • edited Loading

asterite commented Jul 17, 2016

jhass commented Jul 17, 2016 • edited Loading

omarroth commented Jul 17, 2016

jhass commented Jul 17, 2016

omarroth commented Jul 17, 2016

ysbaddaden commented Jul 18, 2016

jhass commented Jul 18, 2016

omarroth commented Jul 18, 2016 • edited Loading

ysbaddaden commented Jul 18, 2016

jhass commented Jul 18, 2016 • edited Loading

omarroth commented Jul 18, 2016

jhass Jul 18, 2016 • edited Loading

Choose a reason for hiding this comment

jhass commented Jul 18, 2016 • edited Loading

jhass commented Jul 18, 2016

omarroth commented Jul 13, 2016 •

edited

Loading

omarroth commented Jul 14, 2016 •

edited

Loading

ozra commented Jul 14, 2016 •

edited

Loading

omarroth commented Jul 15, 2016 •

edited

Loading

jhass commented Jul 15, 2016 •

edited

Loading

jhass commented Jul 17, 2016 •

edited

Loading

omarroth commented Jul 17, 2016 •

edited

Loading

jhass commented Jul 17, 2016 •

edited

Loading

jhass commented Jul 17, 2016 •

edited

Loading

omarroth commented Jul 18, 2016 •

edited

Loading

jhass commented Jul 18, 2016 •

edited

Loading

jhass Jul 18, 2016 •

edited

Loading

jhass commented Jul 18, 2016 •

edited

Loading