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

LLVM 3.8 support #385

Closed
wants to merge 14 commits into from
Closed

LLVM 3.8 support #385

wants to merge 14 commits into from

Conversation

rtrembecky
Copy link
Contributor

@rtrembecky rtrembecky commented May 8, 2016

Hello again,
this is based on #383.
Main changes from LLVM 3.7 to LLVM 3.8:

  • ilist pointers and iterators are no longer interchangeable.
    Iterator get be obtained from pointer by getIterator() function, but to get pointer back, explicit cast is needed. I went for static_cast<PtType *> in my implementation, but simple &* would have sufficed, which is little bit less explicit I think. Does anybody see a downside?
  • Archive::Child is now packed in ErrorOr<>, so I added an error check there
  • Linker::linkModules() arguments are now passed by reference+unique_ptr (ownership is passed by latter)
  • createFunctionAttrsPass() and createGlobalsModRefPass() were replaced
  • testing tools (lit, FileCheck, ...) are no longer shipped in precompiled binaries and Travis CI would not be happy about compiling LLVM everytime. I didn't find a way to run tests this way.
  • Tests: I reworked every llvm-3.7 in REQUIRES check to not-llvm-2.9, not-llvm-3.4, not-llvm-3.5, not-llvm-3.6 to cover LLVM 3.8+ as well. Is there a more proper way?
  • Tests: alias now needs explicit type as well, I added versions of 2 tests

Richard Trembecký and others added 13 commits May 2, 2016 01:33
Based on work by @ccadeptic23 and @delcypher.
Formatting fixed by @snf.
includes experimental ones for Z3
The helper function had int return type, while no value was being returned.
Rewritten `call`s, `load`s and `getelementptr`s to match new specification.
Mainly explicit casts from pointers to iterators and back.
Changed `llvm-3.7` in REQUIRES check to `not-llvm-2.9, not-llvm-3.4,
  not-llvm-3.5, not-llvm-3.6` to cover LLVM 3.8+ as well.
+New version of 2 tests for LLVM 3.8 regarding `alias` syntax.
LLVM 3.8 no longer distributes the testing tools (lit, FileCheck, ...) in
precompiled binaries, we need to build LLVM from source to obtain them, but it
takes too long for Travis to handle.
@ccadar
Copy link
Contributor

ccadar commented May 12, 2016

Hi @rtrembecky , thanks for your work on this. We (@MartinNowack , @andreamattavelli and I) are currently working on a better regression suite for KLEE, based on real programs. Once we're done with this, we should be more comfortable integrating such changes.

I see Dirseek is the only test still failing from the basic regression suite, we need to understand whether it exposes a real problem in the patch or the test itself is fragile and needs to be rewritten.

@rtrembecky
Copy link
Contributor Author

Hello @ccadar , thank you for letting me know. I am looking forward to you finishing your current work and reviewing these.

As of http://llvm.org/releases/3.6.0/docs/ReleaseNotes.html#the-opt-option-std-compile-opts-was-removed
the -std-compile-opts was effectively an alias of -O3.
Also, I don't think using -std-link-opts was appropriate in any way.
@jirislaby
Copy link
Contributor

Hi @ccadar, we have been using llvm-3.8 klee port from @rtrembecky on SV-COMP programs since May. We have satt tool that shows us regressions on SV-COMP programs immediately and we saw no issues back then (the tool is at http://macdui.fi.muni.cz:3000/, but there are no old results). Dragging the patches in our local repository is pain, so have you had a chance to implement the regression suite so that we can start incorporating the llvm-3.8 support into klee?

@ccadar
Copy link
Contributor

ccadar commented Oct 13, 2016

Hi @jirislaby , we have worked on this, particularly @andreamattavelli and @MartinNowack. I think the infrastructure for comparing patches for the same LLVM version is almost done, but doing it across LLVM versions is much harder, simply because it's more difficult to decide when we regressed. So if you can rerun the SV-COMP benchmarks with both 3.4 and 3.8 and give us numbers on how they fare (e.g., coverage, time to find the bug, etc.) that would be really valuable.

@delcypher
Copy link
Contributor

@ccadar @MartinNowack @andreamattavelli We should really stop dragging our feet here. While it is incredibly useful to know if performance between LLVM versions has notably regressed we have to support newer LLVM versions anyway otherwise KLEE will become completely irrelevant.

Given that it is possible to support newer LLVM versions without changing how KLEE behaves with LLVM 3.4 and 2.9 we should just start incorporating these changes.

The longer we leave this, the harder it will be.

Also LLVM 2.9 supports need to die. Nobody is using it and it is causing a huge maintenance burden.

@ccadar
Copy link
Contributor

ccadar commented Oct 13, 2016

@delcypher, I think it's unfair to say we've been dragging our feet here, as @andreamattavelli and @MartinNowack are putting a lot of work into this, which will pay off for future LLVM upgrades too.

I'm also surprised that you say nobody is using LLVM 2.9 anymore, when you come from a research group where many recent projects (e.g., ZESTI, KATCH, Shadow, Docovery) are based on it :) And based on informal chats at conferences, I know our group is not singular here, others are still using 2.9, because it's still more stable in some ways.

This being said, I have changed the webpage to recommend 3.4 for some time now, as I think it's getting there. But as you also know, there are still problems with it. One such example are vector instructions, which are (more) often generated in 3.4 when optimizations are used.

Finally, I am not against incorporating recent patches for newer LLVM versions, on the contrary. Any help reviewing and incorporating them is appreciated. But I would not give up on LLVM 3.4 yet, and neither on 2.9 for a while, until we are confident the newer versions work fine.

@delcypher
Copy link
Contributor

@delcypher, I think it's unfair to say we've been dragging our feet here, as @andreamattavelli and @MartinNowack are putting a lot of work into this, which will pay off for future LLVM upgrades too.

I meant "dragging our feet" in terms accepting patches that allow KLEE to support newer LLVM versions that don't effect the older LLVM versions. The work that @MartinNowack and @andreamattavelli is doing is great but incorporating support for newer LLVM versions (provided it doesn't influence support for the older versions) doesn't depend on this work so we should start incorporating these patches.

I volunteer to start looking at this by incrementally adding support for newer LLVM versions based on this PR and the others lying around.

I'm also surprised that you say nobody is using LLVM 2.9 anymore, when you come from a research group where many recent projects (e.g., ZESTI, KATCH, Shadow, Docovery) are based on it :)

All those projects are forks of KLEE that are not actively maintained and forked from KLEE a long time ago (some before we even had support for LLVM 3.4). For those projects at the time it was likely LLVM 2.9 was the better choice but that isn't really the case anymore. But I should rephrase that. nobody should be using LLVM 2.9.

@andreamattavelli
Copy link
Contributor

@ccadar @delcypher @MartinNowack In an attempt to reconcile everyone, what do you think about creating a branch in which deprecating 2.9 and incorporating the new LLVM versions (3.5-3.8)? In this way we could also start a wider investigation of regression issues.

@rtrembecky
Copy link
Contributor Author

Hey @ccadar @delcypher @andreamattavelli @MartinNowack , I would like to let you to know I am still available for any questions (regarding my code) that may arise. Do you need any help rebasing these PRs on current master? I can't say I am rushing towards this work, but I can provide some help.

@MartinNowack
Copy link
Contributor

MartinNowack commented Oct 26, 2016

@rtrembecky Hehe, what a timing. I started to review and rebase your code and cleaned up some issues from the rebase. I preserved all the commits, which were useful. So far, your changes look quite promising.
I didn't do a PR yet, as I would like to have the one failing benchmark fixed beforehand.
Up to 3.7 is running; for 3.8 we need either autoconf based packages of LLVM or the cmake support.
(Have a look here: https://github.com/MartinNowack/klee/commits/support_llvm_38)

@delcypher
Copy link
Contributor

@MartinNowack Thanks for starting to work on this. It's on my to do list but I've been very busy lately and haven't time to do it. @jirislaby has started CMake support but it some ways of from being ready. I think it might be better for you to get LLVM 3.5-3.7 working and land it in master and in parallel have @jirislaby and I work on landing a CMake build system for KLEE so that working with LLVM 3.8 (and in fact all other LLVM versions) easier.

@@ -89,7 +94,12 @@ ExternalDispatcher::ExternalDispatcher() {
dispatchModule = new Module("ExternalDispatcher", getGlobalContext());

std::string error;
#if LLVM_VERSION_CODE >= LLVM_VERSION(3, 6)
dispatchModule_uniptr.reset(dispatchModule);
executionEngine = EngineBuilder(std::move(dispatchModule_uniptr)).create();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also chain in setErrorStr before the call to create to provide an error string. Otherwise for LLVM versions >= 3.6 if the EngineBuilder fails then the "unable to make jit" error message will be empty.

@adrianherrera
Copy link
Contributor

Hey guys,

I have also done some work in getting support for LLVM 3.9 up-and-running. The main differences are:

  • Removal of the getGlobalContext() function
  • Some methods now return an Either object rather than a ErrorOr object
  • Probably some other minor things that I can't remember off the top of my head

What do you think is the best way to include these changes? I don't really want to create a new PR while this one is still under review. Is there a way to include it in @rtrembecky's PR? Or should I just wait until this one is sorted out?

@rtrembecky
Copy link
Contributor Author

Well, I don't know if there is currently anybody working on this, @MartinNowack may be the last one with some work done. In your place, I would take his support_llvm_38 branch (https://github.com/MartinNowack/klee/commits/support_llvm_38) and work on top of it, so there are fewest conflicts to solve later.

@delcypher
Copy link
Contributor

@MartinNowack How's the work on this going? Now we have the CMake build system I might jump in and try to help out with adding support for newer LLVM versions.

@MartinNowack
Copy link
Contributor

@rtrembecky @delcypher I already rebased the stuff and it should work.
The main problem holding back this PR is the failing test case, which should be investigated first. I was planning to investigate that during the weekend.

@ryosa
Copy link

ryosa commented Nov 21, 2016

Hello. I'm building klee against LLVM 3.8.1 and encountered errors saying 'undefined reference' to pthread and other POSIX functions in linking kleaver objects with libLLVMSupport.a. Is this an issue caused by klee? Or LLVM? Should g++ libraries be added to the link list? Please advise. Thank you.

@jirislaby
Copy link
Contributor

On 11/21/2016, 09:07 AM, Ryo wrote:

Hello. I'm building klee against LLVM 3.8.1 and encountered errors
saying 'undefined reference' to pthread and other POSIX functions in
linking /kleaver/ objects with /libLLVMSupport.a/. Is this an issue
caused by klee? Or LLVM? Should g++ libraries be added to the link list?
Please advise. Thank you.

Coul you paste here the exact errors (the linker output)?

js

@ryosa
Copy link

ryosa commented Nov 22, 2016

Solved! I managed to build klee against LLVM 3.8.

It turned out that llvm-config of LLVM 3.5 and later doesn't output the needed system libraries by --ldflags. So /cmake/find_llvm.cmake should be as follows:

`find_llvm.cmake`
@@ -153,7 +152,11 @@
     # FIXME: This is a hack. We should really create imported
     # targets for the LLVM libraries and have them depend
     # on the necessary system libraries.
-    _run_llvm_config(_system_libs "--ldflags")
+    if (${LLVM_PACKAGE_VERSION} VERSION_LESS "3.5")
+      _run_llvm_config(_system_libs "--ldflags")
+    else()
+      _run_llvm_config(_system_libs "--system-libs")
+    endif()
     string_to_list("${_system_libs}" _system_libs_list)
     set(_filtered_system_libs_list "")
     foreach (l ${_system_libs_list})

@delcypher
Copy link
Contributor

@ryosa Thanks for letting me know. You should know that the CMake build system has only just landed so issues like this should be expected until it has received more testing.

@adrianherrera
Copy link
Contributor

Hey everyone,

Just a heads up I've started some work on getting LLVM 3.9 support up and running. It's based off what we are using in S2E, which unfortunately is a very old version of KLEE. So I've taken @rtrembecky's llvm-3.8 branch, rebased it with the current KLEE master (which you can find at https://github.com/adrianherrera/klee/tree/llvm-3.8) and started working off that.

It's still not 100% compiling, and unfortunataly (or fortunately, depending on your perspective) I'm heading off on holidays today so I won't be able to finish it until I get back early next year. However, I had some requests to make it available to people as a starting point.

It's pretty rough, and I'm sure I'm missing some includes and things for <= LLVM 3.8, so I do apologise for that. However hopefully it's somewhat useful and it can be used as a starting point. You can find it at https://github.com/adrianherrera/klee/tree/llvm-3.9

And @rtrembecky, please feel free to grab what I've done in https://github.com/adrianherrera/klee/tree/llvm-3.8 if it is at all useful

@jbuening
Copy link
Contributor

Is there anything in this PR that has not made it into KLEE yet, now that #900 has been merged?

@MartinNowack
Copy link
Contributor

@jbuening Yes, @adrianherrera's comment about the creation of the jit engine is correct. In case you have some time, can you open a PR for this?

jbuening added a commit to jbuening/klee that referenced this pull request Jul 23, 2018
MartinNowack pushed a commit that referenced this pull request Jul 23, 2018
@MartinNowack
Copy link
Contributor

Now that LLVM 3.8 support has landed in master #900, we can close this one.
A big thanks to everybody involved, especially @rtrembecky .

Sorry for that big delay.

ccadar pushed a commit to ccadar/klee that referenced this pull request Oct 4, 2018
sebymiano pushed a commit to sebymiano/klee that referenced this pull request Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants