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

Initial LLVM3.5 support #141

Closed
wants to merge 28 commits into from
Closed

Initial LLVM3.5 support #141

wants to merge 28 commits into from

Conversation

delcypher
Copy link
Contributor

LLVM 3.5 is going to be released very soon and RC2 is already available so the LLVM3.5 API should be stable. These commits patch KLEE to compile against LLVM3.5.

Some of the big changes are ....

  • we must compile with C++11 for LLVM3.5 . I've taught the configure script to do the right thing for LLVM3.5 and newer and added a configure flag to force C++11 as well.
  • The introduction of LLVM's ErrorOr<T> has made the code a lot more complicated.
  • The archive linking code is a total mess. It compiles but it's very hard to read! This needs fixing

These commits could certainly do with a tidy but I think it's a good start.

Dan Liew added 24 commits August 20, 2014 18:29
For LLVM >= 3.5 explicitly force this to be enabled otherwise we can't
compile against LLVM.
``Support/Debug.h``. We need to define our own DEBUG_TYPE

The LLVM change responsible for this breakage is r206822
has been removed. The commit responsible for this is r202816.

We should check if this header file is actually being used for older
LLVM versions because I'm not convinced it is.
removed. The commit responsible for this is r202816 and r202827.
The commit responsible for this is r202816
DEBUG_TYPE.

The LLVM change responsible for this breakage is r206822
The commit responsible for this is r202816
The commit responsible for this breakage is r211033.
MemoryBuffer from the std::unique_ptr when getLazyBitcodeModule()
succesfully takes ownership.
field was added to the struct in the array of global constructors.

I'm completly ignore the extra field because I don't know what it
does and the LLVM LangRef isn't very clear at all about this.
@delcypher
Copy link
Contributor Author

LLVM3.5 has been released and I can confirm that the commits on this pull request still compile correctly.

@delcypher
Copy link
Contributor Author

Correction it doesn't compile correctly. I had the LLVM3.4 headers on my system so those were getting picked up so this doesn't compile correctly.

Dan Liew added 4 commits September 12, 2014 14:21
llvm/Linker.h -> llvm/Linker/Linker.h (r203065 responsible)
llvm/Assembly/AssemblyAnnotationWriter.h -> llvm/IR/AssemblyAnnotationWriter.h (r198688 responsible)
llvm/DebugInfo.h -> llvm/IR/DebugInfo.h (r203046 responsible)
moves it to "llvm/IR/InstIterator.h" (r202814 is responsible). It turns
out only InstructionInfoTable.cpp was actually using this include so
I just removed the include from the other files.
@delcypher
Copy link
Contributor Author

Okay compilation is fixed. These commits need a massive tidy up though.

@ddunbar
Copy link
Contributor

ddunbar commented Sep 12, 2014

I want to start landing some of these patches, so we can sort out the ones
that need work from the rest. Any objections to me cherry picking some of
your work over to master?

On Fri, Sep 12, 2014 at 6:46 AM, Dan Liew notifications@github.com wrote:

Okay compilation is fixed. These commits need a massive tidy up though.


Reply to this email directly or view it on GitHub
#141 (comment).

@delcypher
Copy link
Contributor Author

@ddunbar No problem. Just to warn you some of them are gross. The "linking" one is the worst. You're probably aware of this but support for linking archives of bitcode modules was dropped. As a temporary solution I duplicated the linking code from LLVM3.2 and implemented our own linker. I think this is the wrong way to do things. I think all the linking and (most optimisations) should be done outside of KLEE. That way

  • We use a production quality linker (ld gold) rather than the pile of crap I wrote
  • Optimizations actually reflect what clang really does (e.g. -O3)

@ddunbar
Copy link
Contributor

ddunbar commented Sep 12, 2014

I am actually the one that ripped out the linking code from LLVM. :)

Yeah, that is gross, but I am ok with it as a temporary way forward. It is
true that in general KLEE should move towards people using proper linker
support for merging bitcode files in order to get its input, but the
current solution has some benefits for now.

Given that we only use the linking code for pulling in our own archive
objects, an alternate way to do this would be to link those archive objects
into a single LLVM bitcode ourselves at build time, and then just link the
two modules (and strip unused functions, ideally).

On Fri, Sep 12, 2014 at 12:58 PM, Dan Liew notifications@github.com wrote:

@ddunbar https://github.com/ddunbar No problem. Just to warn you some
of them are gross. The "linking" one is the worst. You're probably aware of
this but support for linking archives of bitcode modules was dropped. As a
temporary solution I duplicated the linking code from LLVM3.2 and
implemented our own linker. I think this is the wrong way to do things. I
think all the linking and (most optimisations) should be done outside of
KLEE. That way

  • We use a production quality linker (ld gold) rather than the pile of
    crap I wrote
  • Optimizations actually reflect what clang really does (e.g. -O3)


Reply to this email directly or view it on GitHub
#141 (comment).

@delcypher
Copy link
Contributor Author

Given that we only use the linking code for pulling in our own archive
objects, an alternate way to do this would be to link those archive objects
into a single LLVM bitcode ourselves at build time, and then just link the
two modules (and strip unused functions, ideally).

IIRC @MartinNowack tried that and the performance was terrible because klee-uclibc as a single LLVM bitcode module is huge and linking the module you want to run with it takes quite a while.

@MartinNowack
Copy link
Contributor

The total run time for test suite is according to @delcyper's tests (#70)

  • 88.16s for current implementation (implemented by @delcyper)
  • 139.05s prelinking all modules as suggested workaround (I suggested the workaround due to the slowness)
  • 508.84s conservative linking (my first implementation)

I’m also more for removing the whole opt stuff out of it and providing KLEE with the final bc file.

On 12 Sep 2014, at 22:10, Dan Liew notifications@github.com wrote:

Given that we only use the linking code for pulling in our own archive
objects, an alternate way to do this would be to link those archive objects
into a single LLVM bitcode ourselves at build time, and then just link the
two modules (and strip unused functions, ideally).

IIRC @MartinNowack tried that and the performance was terrible because klee-uclibc as a single LLVM bitcode module is huge and linking the module you want to run with it takes quite a while.


Reply to this email directly or view it on GitHub.

@ddunbar
Copy link
Contributor

ddunbar commented Sep 12, 2014

Ah, yes, I can see how the performance might be an issue. Whether or not we
optimize separately is a separate issue though (do we have a separate bug
tracking whether or not to remove that code? I often found it useful to be
able to quickly turn optimizations on during different test runs, but I can
see the argument for ditching it).

For now, I think we should just take the approach of bringing in the legacy
code from LLVM to keep the old behavior. I'll have to think more about what
the right solution is. I think probably what we (LLVM) want here is for
there to be a method in LLVM that allows lazily linking in a module to
another one, giving the performance benefits of the archive style but
without the complexity overhead of actually having to have Archive reading
support.

On Fri, Sep 12, 2014 at 1:22 PM, MartinNowack notifications@github.com
wrote:

The total run time for test suite is according to @delcyper's tests (
#70)

  • 88.16s for current implementation (implemented by @delcyper)
  • 139.05s prelinking all modules as suggested workaround (I suggested the
    workaround due to the slowness)
  • 508.84s conservative linking (my first implementation)

I’m also more for removing the whole opt stuff out of it and providing
KLEE with the final bc file.

On 12 Sep 2014, at 22:10, Dan Liew notifications@github.com wrote:

Given that we only use the linking code for pulling in our own archive
objects, an alternate way to do this would be to link those archive
objects
into a single LLVM bitcode ourselves at build time, and then just link
the
two modules (and strip unused functions, ideally).

IIRC @MartinNowack tried that and the performance was terrible because
klee-uclibc as a single LLVM bitcode module is huge and linking the module
you want to run with it takes quite a while.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#141 (comment).

@ddunbar
Copy link
Contributor

ddunbar commented Sep 14, 2014

I just landed a bunch of the trivial ones on master. I ended up rewriting some of the commits which is going to make it hard to rebase this branch, but I plan on continuing to work through this. The main thing left is the changes to ModuleUtil, and then a pass over everything else to make sure I didn't miss any semantic changes.

@delcypher
Copy link
Contributor Author

@ddunbar We can probably just chuck this branch. It was more of test to see if I could port it to LLVM3.5. If you cherry-pick the good commits from this branch then we can try to tackle the difficult parts in a cleaner way.

I think probably what we (LLVM) want here is for
there to be a method in LLVM that allows lazily linking in a module to
another one, giving the performance benefits of the archive style but
without the complexity overhead of actually having to have Archive reading
support.

Does getLazyBitcodeModule() serve that purpose? I remember trying to use it the linking code I wrote but it didn't work (I couldn't read the symbol table correctly) so I just loaded the modules the normal way.

@ddunbar
Copy link
Contributor

ddunbar commented Sep 15, 2014

It's the underpinnings, but what we probably should have in LLVM is a
function that lazily links in a module (presumably with the lazy side being
a getLazyBitcodeModule() result). Like Linker::linkInModule, but where it
only brings in the code it needs from the other module.

On Mon, Sep 15, 2014 at 7:42 AM, Dan Liew notifications@github.com wrote:

@ddunbar https://github.com/ddunbar We can probably just chuck this
branch. It was more of test to see if I could port it to LLVM3.5. If you
cherry-pick the good commits from this branch then we can try to tackle the
difficult parts in a cleaner way.

I think probably what we (LLVM) want here is for
there to be a method in LLVM that allows lazily linking in a module to
another one, giving the performance benefits of the archive style but
without the complexity overhead of actually having to have Archive reading
support.

Does getLazyBitcodeModule() serve that purpose? I remember trying to use
it the linking code I wrote but it didn't work (I couldn't read the symbol
table correctly) so I just loaded the modules the normal way.


Reply to this email directly or view it on GitHub
#141 (comment).

@banescusebi
Copy link

Hi all, is there any news about getting KLEE to work with LLVM 3.5 in the near future? I've just tried it out with the latest commit of KLEE and I get similar errors to the ones in the following mailing list message: https://www.mail-archive.com/klee-dev@imperial.ac.uk/msg01770.html������������������������������������������������������������������

@delcypher
Copy link
Contributor Author

@banescusebi I wish I had time to work on this but right now I don't. You're quite welcome to have a go at it and I'll happily review a pull request.

@ccadar ccadar mentioned this pull request Mar 13, 2015
@ccadar
Copy link
Contributor

ccadar commented Aug 3, 2015

Closing this branch, as some commits were already pushed and the rest were incorporated as part of #253.

@ccadar ccadar closed this Aug 3, 2015
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

Successfully merging this pull request may close these issues.

None yet

5 participants