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

Add support for importing symbols from static library #902

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
9b891d0
Add test files to be compiled for add_archive test case
testhound Feb 3, 2023
ede2555
Add support for add_archive, test case and documentation
testhound Feb 3, 2023
ca0abbf
Fix flake8 errors
testhound Feb 3, 2023
6a334a1
Fix test_add_archive to create object files and static library in a t…
testhound Feb 6, 2023
88114fc
Fix formatting according to clang-format
testhound Feb 6, 2023
77b6f89
Merge branch 'main' into testhound/addArchive
testhound Feb 10, 2023
e56a4c7
Migrate from using deprecated distutils to setuptools
testhound Feb 10, 2023
9d07856
Add code to test if calls to external compiler works and potentially …
testhound Feb 10, 2023
e77b239
Fix flake8 errors
testhound Feb 10, 2023
90043d4
Add C test files
testhound Feb 14, 2023
cb9eb40
Add *.c files to all uses of pacakge data
testhound Feb 14, 2023
0ee2687
Incorporate upstream comments
testhound Feb 14, 2023
6f2a5ef
Merge branch 'main' into testhound/addArchive
testhound Apr 18, 2023
96f2766
Write out .c .files instead of storing them in the test directory
testhound Apr 21, 2023
f9ccb45
Add use of auto varibles and simplify some code sequences
testhound Apr 21, 2023
476099a
Remove test .c files and write them to temp directory instead, use os…
testhound Apr 21, 2023
fcf38e6
Remove .c files from setup as they have are no longer needed for testing
testhound Apr 21, 2023
a159f31
Format with clang-format
testhound Apr 21, 2023
18b6aba
Revert use of 'auto' as it causes an error
testhound Apr 21, 2023
ecfe984
Remove final use of .c test files
testhound Apr 21, 2023
0db31a7
Allow test to run on Windows
testhound Apr 24, 2023
5e51921
Add debug print statement
testhound Apr 24, 2023
405b85c
Use CCompiler.library_filename to get platform specific name
testhound Apr 25, 2023
c68001b
Try instantiating unix compiler regardless of platform
testhound Apr 25, 2023
ab90410
Make sure function names are not mangled and use C calling convention
testhound Apr 25, 2023
af07004
Fix flake8 issue
testhound Apr 25, 2023
22ddfc2
Test that the jit found the function address
testhound Apr 26, 2023
f49b699
Use try .. finally block to clean up temp directory
testhound Apr 27, 2023
608b6b8
Delete jit object in case it is keeping archive open
testhound May 4, 2023
bc2023c
Delete all files created during test
testhound May 4, 2023
281a9d3
Don't use Path
testhound May 4, 2023
2e7d474
Return and use tmpdir
testhound May 7, 2023
c9a59e5
Close file to resolve Windows open file issue
testhound May 8, 2023
37182a7
Remove __ from from function names and document why the testcase does…
testhound May 10, 2023
269cd6d
Skip test on macOS and Python 3.9 due to CI issue
testhound May 11, 2023
d42498d
Add string reason to skipTest
testhound May 11, 2023
0effd26
Don't skip Windows to expose error
testhound May 24, 2023
28e5ef7
Add Windows specific compilation
testhound May 30, 2023
9f53121
Convert test case to a .c file
testhound Jun 2, 2023
674ca5f
Set debug=True
testhound Jun 2, 2023
488dd58
Fix flake8 and remove unused funtion
testhound Jun 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions docs/source/user-guide/binding/execution-engine.rst
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ The ExecutionEngine class
or a object file instance. Object file instance is not usable after this
call.

* .. method:: add_archive(archive_file)

Add the symbols from the specified static archive file to the execution
Copy link
Member

Choose a reason for hiding this comment

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

What does the word "static" mean here? My understanding is that in general, an archive contains unlinked objects, and those objects might eventually take part in a static or dynamic linking process (depending on how they were compiled, of course).

According to the LLVM docs, it looks like adding an archive uses

Suggested change
Add the symbols from the specified static archive file to the execution
Add the symbols from the specified archive file to the execution

Copy link
Author

Choose a reason for hiding this comment

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

@gmarkall static here means static archive (i.e libc.a) vs shared library (libc.so). I double checked the code on the LLVM C++ side of things and it is only expecting a static archive.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like somehow my comment got truncated, apologies - what I'm trying to suggest is that "static archive" feels like a weird term - it's an archive file of objects, which will probably be linked statically. So either "archive file" or "static library" seem like appropriate terms (but "archive file" is better here since the method name refers to archives), but "static archive" is a mashup of the two, which I can't find any reference to in common use.

engine. It is a fatal error in LLVM if the *archive_file* does not exist.

* *archive* str: a path to the static object file
Copy link
Member

Choose a reason for hiding this comment

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

The parameter name doesn't match that from above. Is "static object file" appropriate here, or is "archive file" more consistent?

Suggested change
* *archive* str: a path to the static object file
* *archive_file* str: path to the archive file


* .. method:: set_object_cache(notify_func=None, getbuffer_func=None)

Set the object cache callbacks for this engine.
Expand Down
38 changes: 37 additions & 1 deletion ffi/executionengine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
#include "llvm/Object/Binary.h"
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Memory.h"

#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Object/Archive.h"
#include "llvm/Support/Errc.h"
#include <cstdio>
#include <memory>

Expand Down Expand Up @@ -167,6 +169,40 @@ LLVMPY_MCJITAddObjectFile(LLVMExecutionEngineRef EE, LLVMObjectFileRef ObjF) {
{std::move(binary_tuple.first), std::move(binary_tuple.second)});
}

API_EXPORT(int)
LLVMPY_MCJITAddArchive(LLVMExecutionEngineRef EE, const char *ArchiveName,
const char **OutError) {
using namespace llvm;
using namespace llvm::object;
auto engine = unwrap(EE);

ErrorOr<std::unique_ptr<MemoryBuffer>> ArBufOrErr = MemoryBuffer::getFile(ArchiveName);

std::error_code EC = ArBufOrErr.getError();
if (EC){
*OutError = LLVMPY_CreateString(EC.message().c_str());
return 1;
}

Expected<std::unique_ptr<object::Archive>> ArchiveOrError =
object::Archive::create(ArBufOrErr.get()->getMemBufferRef());
Copy link
Member

Choose a reason for hiding this comment

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

The llvm::object namespace is already visible in this scope:

Suggested change
object::Archive::create(ArBufOrErr.get()->getMemBufferRef());
Archive::create(ArBufOrErr.get()->getMemBufferRef());


if (!ArchiveOrError) {
auto takeErr = ArchiveOrError.takeError();
std::string archiveErrStr = "Unable to load archive: " + std::string(ArchiveName);
*OutError = LLVMPY_CreateString(archiveErrStr.c_str());
return 1;
}

std::unique_ptr<MemoryBuffer> &ArBuf = ArBufOrErr.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

These can also be simplified to:

    OwningBinary<object::Archive> owningBinaryArchive(std::move(*ArchiveOrError),
                                                      std::move(*ArBufOrErr));

std::unique_ptr<object::Archive> &Ar = ArchiveOrError.get();
object::OwningBinary<object::Archive> owningBinaryArchive(std::move(Ar),
std::move(ArBuf));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
object::OwningBinary<object::Archive> owningBinaryArchive(std::move(Ar),
std::move(ArBuf));
OwningBinary<object::Archive> owningBinaryArchive(std::move(Ar),
std::move(ArBuf));

engine->addArchive(std::move(owningBinaryArchive));
*OutError = LLVMPY_CreateString("");
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to create an empty string if we're returning success.

Suggested change
*OutError = LLVMPY_CreateString("");

return 0;
}

//
// Object cache
//
Expand Down
20 changes: 20 additions & 0 deletions llvmlite/binding/executionengine.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,18 @@ def add_object_file(self, obj_file):

ffi.lib.LLVMPY_MCJITAddObjectFile(self, obj_file)

def add_archive(self, archive_file):
"""
Add archive file to the jit. archive_file is a string
representing the file system path to the archive file.
"""

with ffi.OutputString() as outerr:
res = ffi.lib.LLVMPY_MCJITAddArchive(self, archive_file.encode(),
outerr)
if res:
raise RuntimeError(str(outerr))

def set_object_cache(self, notify_func=None, getbuffer_func=None):
"""
Set the object cache "notifyObjectCompiled" and "getBuffer"
Expand Down Expand Up @@ -286,6 +298,14 @@ def _dispose(self):
ffi.LLVMObjectFileRef
]

ffi.lib.LLVMPY_MCJITAddArchive.argtypes = [
ffi.LLVMExecutionEngineRef,
c_char_p,
POINTER(c_char_p)
]

ffi.lib.LLVMPY_MCJITAddArchive.restype = c_int


class _ObjectCacheData(Structure):
_fields_ = [
Expand Down
5 changes: 5 additions & 0 deletions llvmlite/tests/a.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@

int __multiply_accumulate(int a, int b, int c)
{
return (a * b) + c;
}
4 changes: 4 additions & 0 deletions llvmlite/tests/b.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
int __multiply_subtract(int a, int b, int c)
{
return (a * b) - c;
}
50 changes: 50 additions & 0 deletions llvmlite/tests/test_binding.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
from llvmlite import binding as llvm
from llvmlite.binding import ffi
from llvmlite.tests import TestCase
import llvmlite.tests
from distutils.ccompiler import new_compiler


# arvm7l needs extra ABI symbols to link successfully
Expand Down Expand Up @@ -1935,6 +1937,54 @@ def test_get_section_content(self):
self.assertEqual(s.data().hex(), issue_632_text)


class TestArchiveFile(BaseTest):
mod_archive_asm = """
;ModuleID = <string>
target triple = "{triple}"

declare i32 @__multiply_accumulate(i32 %0, i32 %1, i32 %2)
declare i32 @__multiply_subtract(i32 %0, i32 %1, i32 %2)
"""

@unittest.skipUnless(sys.platform.startswith('linux'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this Linux specific? It needs to be tested on all platforms.

Copy link
Author

Choose a reason for hiding this comment

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

@apmasell thanks I modified the code to test on all platforms. I am now getting errors on all versions of Windows and a single version of macOS. Do you have any feedback you can offer?

Copy link
Member

Choose a reason for hiding this comment

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

One thing that stands out to me is that the library is called foo but you're looking for libfoo.lib on Windows. It's not usual for Windows library names to be prefixed with lib - maybe the library is called foo.lib?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps using CCompiler.library_filename() can help here.

Copy link
Author

Choose a reason for hiding this comment

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

@gmarkall thanks that got me further on Windows, still more Windows errors to debug.

Copy link
Member

Choose a reason for hiding this comment

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

On Windows you can't delete an open file. I wonder if the file is still open when you attempt to clean up the temp dir, causing the issue you see - perhaps deleting jit will help the cleanup to succeed.

"Linux-specific test")
def test_add_archive(self):
target_machine = self.target_machine(jit=False)

jit = llvm.create_mcjit_compiler(self.module(self.mod_archive_asm),
target_machine)

# Create compiler with default options
c = new_compiler()
workdir = os.path.dirname(llvmlite.tests.__file__) + "/"
Copy link
Member

Choose a reason for hiding this comment

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

It can't be assumed that the llvmlite test dir is writable (and I don't think it's good form to drop files into a package dir as part of running the tests anyway). If the test dir isn't writable then the test errors:

======================================================================
ERROR: test_add_archive (llvmlite.tests.test_binding.TestArchiveFile)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/gmarkall/numbadev/llvmlite/llvmlite/tests/test_binding.py", line 1969, in test_add_archive
    c.create_static_lib(objects, library_name, output_dir=workdir)
  File "/home/gmarkall/mambaforge/envs/numbadev/lib/python3.10/site-packages/setuptools/_distutils/unixccompiler.py", line 131, in create_static_lib
    self.spawn(self.archiver +
  File "/home/gmarkall/mambaforge/envs/numbadev/lib/python3.10/site-packages/setuptools/_distutils/ccompiler.py", line 917, in spawn
    spawn(cmd, dry_run=self.dry_run, **kwargs)
  File "/home/gmarkall/mambaforge/envs/numbadev/lib/python3.10/site-packages/setuptools/_distutils/spawn.py", line 68, in spawn
    raise DistutilsExecError(
distutils.errors.DistutilsExecError: command '/opt/binutils/2.37/bin/ar' failed with exit code 1

----------------------------------------------------------------------


# Compile into .o files
file1 = workdir + "a.c"
file2 = workdir + "b.c"

objects = c.compile([file1, file2])
library_name = "foo"
# Create static or shared library

c.create_static_lib(objects, library_name, output_dir=workdir)

static_library_name = workdir + "lib" + library_name + ".a"
jit.add_archive(static_library_name)

mac_func = CFUNCTYPE(c_int, c_int, c_int, c_int)(
jit.get_function_address("__multiply_accumulate"))

msub_func = CFUNCTYPE(c_int, c_int, c_int, c_int)(
jit.get_function_address("__multiply_subtract"))

self.assertEqual(mac_func(10, 10, 20), 120)
self.assertEqual(msub_func(10, 10, 20), 80)

os.unlink(objects[0])
Copy link
Member

Choose a reason for hiding this comment

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

If the test fails then this leaves files on disk. The tempfile module has context managers for ensuring cleanup: https://docs.python.org/3/library/tempfile.html

os.unlink(objects[1])
os.unlink(static_library_name)


class TestTimePasses(BaseTest):
def test_reporting(self):
mp = llvm.create_module_pass_manager()
Expand Down