Add support for archive and single bc file linking #70

MartinNowack · 2013-12-05T00:02:00Z

With LLVM 3.3 the linker does not support reading of
archive files directly. This brings the support back
(based on llvm-mn).
Furthermore, linking single bc files or archives with
bc and object files mixed is supported as well.

@delcypher: we will need that patch to use uclibc for llvm 3.3.

delcypher · 2013-12-05T11:43:47Z

@MartinNowack Could you clarify two things

By object files you are referring to native binary object files? So if I understand you correctly now archives can contain a mix of native and LLVM bitcode modules?
Why would we need patch klee-uclibc? klee-uclibc just produces archives of LLVM bitcode modules (e.g. libc.a). The description of your patch suggests that bitcode archives are now supported (for LLVM >= 3.3) so it doesn't seem like klee-uclibc would need to be changed in anyway.

MartinNowack · 2013-12-05T18:29:52Z

@delcypher

Yes, LLVM bitcode and object files can be mixed inside an archive. Nevertheless, object files won't be loaded. But KLEE warns about that.
Let me rephrase it, I meant that we need this patch for KLEE to be able to use uclibc without patching. Before that (for LLVM >= 3.3.) we could just link in a single bc file.

delcypher · 2013-12-16T17:32:34Z

@MartinNowack I just tried this. There seem to be some performance regressions due to your approach.

For example running the following on LLVM2.9

$ cd test
$ time make TESTONE=Runtime/POSIX/DirConsistency.c check-one VERBOSE=1

takes ~2.9 seconds.

On LLVM3.3 the same command (you need the fix here to run the above in LLVM3.3) takes ~54 seconds on my machine.

At a glance running the test manually it seems that a very large amount of time is being spent linking in klee-uclibc. Any thoughts as to why that might be?

delcypher · 2013-12-17T10:42:27Z

I took another look. I'm not convinced the approach is right. IIRC when linking bitcode archives the constituent modules must be iterated over until a fixed point is reached (when the set of undefined symbols does not change). In your approach we only pass over the modules once.

I've posted some questions to the llvm-dev mailing list because there are quite a few things here I don't quite understand.

It occurs to me there might be a way to avoid these issues entirely but I do not know if it is possible. It would great if clang was setup to link against uclibc (I presume when compiling at the moment it assumes the use of glibc) and have it link in KLEE's other runtime bits. That way KLEE doesn't have to worry about linking in anything itself.

MartinNowack · 2013-12-17T12:25:35Z

Linking bitcode files is just simple kind of concatenating them (plus checking for name clashes or solving them in case different linking schemes were used). At the end, you have everything in one big module. Therefore, passing over the bitcode archive once is enough. You won't be able to solve more (but beside that, linking normal object files works differently - and yes, in that case order of object files plays a role and yes, you've to iterate it to get the fixed point).

Currently, with gcc and clang, libc implementations are integrated, one can replace them but either one re-compiles the compiler or changes build flags/system in the target application. I'm not quite sure if it makes it easier to use KLEE by modifying essential parts of the compiler. I would rather like to stick with KLEE being able to do it and using the full power of underlying LLVM tools.

delcypher · 2013-12-17T14:27:02Z

Linking bitcode files is just simple kind of concatenating them (plus checking for name clashes or solving them in case different linking schemes were used). At the end, you have everything in one big module. Therefore, passing over the bitcode archive once is enough.

Ah so the approach is valid then. But clearly we are doing much more work than we did previously (looking at the implementation of Linker::LinkInArchive() that was used in LLVM < 3.3 it seems to do the fixed point iteration which will only link in the modules that are needed), we are linking every module in, even if it isn't needed. This seems to be causing performance problems so I think we may need to re-think the approach. We could reimplement the fixed point iteration over the bitcode modules ourselves. I am hoping though that someone will reply to what I posted on llvmdev with a suggestion on what to do instead.

Currently, with gcc and clang, libc implementations are integrated, one can replace them but either one re-compiles the compiler or changes build flags/system in the target application. I'm not quite sure if it makes it easier to use KLEE by modifying essential parts of the compiler. I would rather like to stick with KLEE being able to do it and using the full power of underlying LLVM tools.

Okay. It was just a thought. It would certainly simplify some parts of KLEE (e.g. no longer necessary to make uclibc's main the entry point) but I guess modifying clang is non-trivial.

MartinNowack · 2013-12-19T09:28:12Z

A good workaround for uclibc is ccadar/klee-uclibc#6.
For user of KLEE and own libraries, they can either use archives (with linking overhead each klee startup, for uclibc only those are ~700 files) or walk the extra mile and link files together to one archive to save that time if this is really an issue for user provided files.

Maybe, over time, LLVM infrastructure becomes better to handle that linking problem in a more sophisticated way.

Currently, I would go for that patch because the features it provides is en par with the pre-llvm 3.3 version.
We could make the linking phase more distinct for the user to help spotting it in a timely manner.

One guess why it was faster before llvm 3.3 is that the symbol index table of archives was used to speed up the process.

delcypher · 2013-12-19T11:46:12Z

@MartinNowack Thanks. I'm currently investigating how easy it would be to re-implement the fixed point approach for linking in bitcode archives. If that proves to be too difficult/complicated I'll go for your approach instead.

delcypher · 2014-01-21T14:32:46Z

@MartinNowack Sorry I've only just got round to looking at this again. I'm going to try to implementing a slightly different approach for bitcode archives today/tomorrow. Just to warn you I just rebased the branch on current master.

delcypher · 2014-01-23T20:46:26Z

My previous commit improves linking substantially for LLVM3.3 but unfortunately it's still not as fast as it was in LLVM2.9

Before 6f3973a (LLVM3.3)

********************
Testing Time: 636.49s
********************
Failing Tests (3):
    KLEE :: Feature/MemoryLimit.c
    KLEE :: Runtime/POSIX/FD_Fail2.c
    KLEE :: Runtime/POSIX/Isatty.c

For 6f3973a (LLVM3.3)

********************
Testing Time: 173.26s
********************
Failing Tests (3):
    KLEE :: Feature/MemoryLimit.c
    KLEE :: Runtime/POSIX/FD_Fail2.c
    KLEE :: Runtime/POSIX/Isatty.c

and for LLVM2.9 (unaffected by my commit)

Testing Time: 49.20s

I am also slightly worried with my implementation too because it doesn't consider trying to find a strong definition for a defined weak symbol. The old algorithm in LLVM3.2 didn't seem to do this either, so maybe it's okay??

I'd say there's lots of things about my implementation that could be improved (I quickly tried lazy loading the bitcode modules but it seems the ValueSymbolTable doesn't have the necessary symbols in it when I do this) but this is probably an okay start.

@ccadar @MartinNowack Thoughts?

delcypher · 2014-01-24T10:50:16Z

Sorry I just realise I gave invalid times for KLEE built against LLVM3.3 because I disabled optimisations (i.e. -O0) for debugging but forgot to re-enable them so I can't make a fair comparison with KLEE built against LLVM2.9 with the previous timings . So I've rerun the tests with KLEE compiled with optimisations (-O2)

Before 6f3973a

********************
Testing Time: 508.84s
********************
Failing Tests (3):
    KLEE :: Feature/MemoryLimit.c
    KLEE :: Runtime/POSIX/FD_Fail2.c
    KLEE :: Runtime/POSIX/Isatty.c

For 6f3973a

********************
Testing Time: 88.16s
********************
Failing Tests (3):
    KLEE :: Feature/MemoryLimit.c
    KLEE :: Runtime/POSIX/FD_Fail2.c
    KLEE :: Runtime/POSIX/Isatty.c

So with these new test runs performance is closer to what we have in LLVM2.9 . It's not quite there but I would so this is "good enough" for now.

The only thing I'd like to fix is that linker may include in its list of undefined symbols the KLEE special functions (e.g. klee_make_symbolic) and some LLVM intrinsics. After that I think this is probably good to merge.

delcypher · 2014-01-29T14:32:09Z

@ccadar recommended that I test @MartinNowack workaround too. The runtime on my machine when using Martin's patch to klee-uclibc with KLEE at 302b6cd
was as follows

********************
Testing Time: 139.05s
********************
Failing Tests (3):
    KLEE :: Feature/MemoryLimit.c
    KLEE :: Runtime/POSIX/FD_Fail2.c
    KLEE :: Runtime/POSIX/Isatty.c

This is certainly better performance than KLEE at 302b6cd with an unpatched version of klee-uclibc but KLEE at 6f3973a with an unpatched version of klee-uclibc looks faster.

With LLVM 3.3 the linker does not support reading of archive files directly. This brings the support back (based on llvm-mn). Furthermore, linking single bc files or archives with bc and object files mixed is supported as well.

LLVM >= 3.3 by effectively reimplementing the linking algorithm used in LLVM <= 3.2. The LLVM specific bitcode archive format has been removed from LLVM >= 3.3 . Now archives are normal system archives that can contain LLVM bitcode modules as well as regular binary object files. The previous commit implemented an approach where ALL the bitcode modules get linked in which can be terribly slow when klee-uclibc gets linked (~600 LLVM modules). Here are the options that I considered to address this: * Use LD with LLVM gold plug-in and call as an external program. I Don't really want to add another dependency to KLEE. It already has enough! * Use the upcomming LLVM linker (lld). Not really an option because at the time of writing there is no support for linking archives of bitcode modules. * Don't use archives at all and just work with modules (i.e. replace uses of llvm-ar with llvm-link and tinker with the flags a little). This isn't so great because the resulting LLVM bitcode module we execute is bigger than it should be. * Reimpelent bitcode archive linking ourselves in a slightly better way. I've gone for the last option This implementation unfortunately loads all bitcode modules into memory first so we can query the module symbols tables. I would prefer to read the archive's index and link in modules on demand but unfortunately although the new Object::Archive interface in LLVM allows iteration over symbols it doesn't provide a way of knowing if that symbol is defined/undefined. This implementation is far from perfect!

bitcode archive linker.

that clients can access HandlerInfo nicely.

KLEE intrinsics as undefined symbols

Iterators get invalidated after elements of std::vector/set are deleted. Avoid this by remembering which elements need to be deleted and do it after iterating over the data structure.

MartinNowack · 2014-02-06T23:11:11Z

I went through the code. Major issues were the iterators - strange that the test suite ran through.
Some of my "lucky" runs where endless.
But in general, it works well and a lot faster.
Thanks for your effort @delcypher

(By the way using the new llvm-lit is great!!)
And those are my lucky numbers ;)

Unoptimized - LLVM 3

********************
Testing Time: 133.83s
********************
Failing Tests (1):
    KLEE :: Runtime/POSIX/FD_Fail2.c

delcypher · 2014-02-07T14:49:11Z

@MartinNowack Thanks for taking a look. Looking at my code there is at least one bug (in my implementation) in linkBCA(object::Archive* archive, Module* composite, std::string& errorMessage)

break; // Look for symbols in next module

This breaks out of the loop over a modules symbols but then the iterator gets incremented again (so we unintentionally skip a module, this shouldn't cause any problems apart from making linking very inefficient.

Looking at GetAllUndefinedSymbols(Module *M, std::set<std::string> &UndefinedSymbols) I was under the impression that doing something like...

for (std::set<std::string>::iterator I = UndefinedSymbols.begin();
       I != UndefinedSymbols.end(); )
  {
    if (DefinedSymbols.count(*I))
    {
      UndefinedSymbols.erase(I++);  // This symbol really is defined!
      continue;
    }
    ++I;
}

was legal.? If you look at this it says...

Iterators, pointers and references referring to elements removed by the function are invalidated.
All other iterators, pointers and references keep their validity.

So I think doing erase(I++) is okay because we increment the iterator and then erase the previous value of I.

I admit this sort of code is hard to read and a little fragile so it's probably better to go for your approach.

because "RemovedSymbols" implies that the symbols have already been removed which is misleading because we don't remove until the end.

then clean up is performed.

of modules left because this information is no longer correct (we no longer shrink the vector).

delcypher · 2014-02-14T14:16:27Z

I've cleaned up a few things but it looks good. I'm going to go ahead and merge.

Add support for archive and single bc file linking

arrowd · 2019-12-10T06:37:03Z

I'd like to bring this up again.

I'm working on adding support for FreeBSD libc as KLEE libc and stumbled upon this very deficiency:

I am also slightly worried with my implementation too because it doesn't consider trying to find a strong definition for a defined weak symbol. The old algorithm in LLVM3.2 didn't seem to do this either, so maybe it's okay??

Before hacking in I want to make sure we really need current complex linker implementation.

From what I read, this "link one by one until a fixed point" approach is used for performance reasons, am I right?

MartinNowack mentioned this pull request Dec 5, 2013

Test and then announce new klee-uclibc klee/klee-uclibc#2

Closed

MartinNowack mentioned this pull request Dec 19, 2013

Link bc files together to one bc if clang is used klee/klee-uclibc#6

Closed

MartinNowack and others added 6 commits February 6, 2014 23:55

Add support for archive and single bc file linking

9e16c44

With LLVM 3.3 the linker does not support reading of archive files directly. This brings the support back (based on llvm-mn). Furthermore, linking single bc files or archives with bc and object files mixed is supported as well.

Do not consider llvm intrinsics as undefined symbols in KLEE's

d3202c9

bitcode archive linker.

Implement const_iterator interface for SpecialFunctionHandler so

ebaad17

that clients can access HandlerInfo nicely.

When using KLEE's built-in Bitcode archive linker do not consider

baa7bff

KLEE intrinsics as undefined symbols

Fix access of iterators after they have been invalidated

129a80e

Iterators get invalidated after elements of std::vector/set are deleted. Avoid this by remembering which elements need to be deleted and do it after iterating over the data structure.

MartinNowack closed this Feb 6, 2014

MartinNowack reopened this Feb 6, 2014

Dan Liew added 4 commits February 14, 2014 12:32

Correct and tidy up comments.

01fd5aa

Refactor variable name s/RemovedSymbols/SymbolsToRemove/

360f372

because "RemovedSymbols" implies that the symbols have already been removed which is misleading because we don't remove until the end.

Refactor cleaning up memory in linkBCA() so that if linking fails

3c49bee

then clean up is performed.

When running with -debug-only=klee_linker do not report the number

fd0c661

of modules left because this information is no longer correct (we no longer shrink the vector).

delcypher added a commit that referenced this pull request Feb 14, 2014

Merge pull request #70 from MartinNowack/feature_reading_archive

2ad968e

Add support for archive and single bc file linking

delcypher merged commit 2ad968e into klee:master Feb 14, 2014

MartinNowack deleted the feature_reading_archive branch December 10, 2019 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for archive and single bc file linking #70

Add support for archive and single bc file linking #70

MartinNowack commented Dec 5, 2013

delcypher commented Dec 5, 2013

MartinNowack commented Dec 5, 2013

delcypher commented Dec 16, 2013

delcypher commented Dec 17, 2013

MartinNowack commented Dec 17, 2013

delcypher commented Dec 17, 2013

MartinNowack commented Dec 19, 2013

delcypher commented Dec 19, 2013

delcypher commented Jan 21, 2014

delcypher commented Jan 23, 2014

delcypher commented Jan 24, 2014

delcypher commented Jan 29, 2014

MartinNowack commented Feb 6, 2014

delcypher commented Feb 7, 2014

delcypher commented Feb 14, 2014

arrowd commented Dec 10, 2019

Add support for archive and single bc file linking #70

Add support for archive and single bc file linking #70

Conversation

MartinNowack commented Dec 5, 2013

delcypher commented Dec 5, 2013

MartinNowack commented Dec 5, 2013

delcypher commented Dec 16, 2013

delcypher commented Dec 17, 2013

MartinNowack commented Dec 17, 2013

delcypher commented Dec 17, 2013

MartinNowack commented Dec 19, 2013

delcypher commented Dec 19, 2013

delcypher commented Jan 21, 2014

delcypher commented Jan 23, 2014

delcypher commented Jan 24, 2014

delcypher commented Jan 29, 2014

MartinNowack commented Feb 6, 2014

delcypher commented Feb 7, 2014

delcypher commented Feb 14, 2014

arrowd commented Dec 10, 2019