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 11 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_file* str: a path to the static archive file

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

Set the object cache callbacks for this engine.
Expand Down
40 changes: 39 additions & 1 deletion ffi/executionengine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
#include "llvm/ExecutionEngine/JITEventListener.h"
#include "llvm/ExecutionEngine/ObjectCache.h"
#include "llvm/IR/Module.h"
#include "llvm/Object/Archive.h"
#include "llvm/Object/Binary.h"
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/Memory.h"

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

Expand Down Expand Up @@ -167,6 +169,42 @@ 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 =
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm a little lazy (as a C++ programmer) but it seems like we could simplify some of these declarations by using auto a bit more. I don't have strong feelings either way, but tend to prefer it when I'm writing.

Copy link
Author

@testhound testhound Feb 14, 2023

Choose a reason for hiding this comment

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

@gmarkall it's up to you, I find "auto" can obfuscate in a situation like this especially when you need to determine what to unpack such as getting the error message.

Copy link
Author

Choose a reason for hiding this comment

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

@gmarkall to your comment "... which will probably be linked statically", a static archive can only be linked statically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also prefer these to consistently be auto given some assignments in the function are auto

MemoryBuffer::getFile(ArchiveName);

std::error_code EC = ArBufOrErr.getError();
if (EC) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more idiomatically written as:

if (!ArBufOrErr) {... }

without the creation of 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());
Copy link
Member

Choose a reason for hiding this comment

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

Rather than providing a generic error message, passing along the error provided by LLVM will probably be more helpful to the user. I'm not sure of the cleanest way to do it (I often have some trouble picking and choosing between LLVM C++ and C APIs, for example, but here's one way to do it:

Suggested change
std::string archiveErrStr =
"Unable to load archive: " + std::string(ArchiveName);
*OutError = LLVMPY_CreateString(archiveErrStr.c_str());
LLVMErrorRef errorRef = wrap(std::move(takeErr));
*OutError = LLVMPY_CreateString(LLVMGetErrorMessage(errorRef));

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;
}
102 changes: 101 additions & 1 deletion llvmlite/tests/test_binding.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,58 @@
import sys
import unittest
from contextlib import contextmanager
from tempfile import mkstemp
from tempfile import mkstemp, TemporaryDirectory, mkdtemp
from pathlib import Path

from llvmlite import ir
from llvmlite import binding as llvm
from llvmlite.binding import ffi
from llvmlite.tests import TestCase
import llvmlite.tests
from setuptools._distutils.ccompiler import new_compiler
from setuptools._distutils.sysconfig import customize_compiler
import functools


@contextmanager
def _gentmpfile(suffix):
# windows locks the tempfile so use a tempdir + file, see
# https://github.com/numba/numba/issues/3304
try:
tmpdir = mkdtemp()
ntf = open(os.path.join(tmpdir, "temp%s" % suffix), 'wt')
yield ntf
finally:
try:
ntf.close()
os.remove(ntf)
except Exception:
pass
else:
os.rmdir(tmpdir)


@functools.lru_cache(maxsize=1)
def external_compiler_works():
"""
Returns True if the "external compiler" bound in setuptools._distutil
is present and working, False otherwise.
"""
compiler = new_compiler()
customize_compiler(compiler)
for suffix in ['.c', '.cxx']:
try:
with _gentmpfile(suffix) as ntf:
simple_c = "int main(void) { return 0; }"
ntf.write(simple_c)
ntf.flush()
ntf.close()
# *output_dir* is set to avoid the compiler putting temp files
# in the current directory.
compiler.compile([ntf.name], output_dir=Path(ntf.name).anchor)
except Exception: # likely CompileError or file system issue
return False
return True


# arvm7l needs extra ABI symbols to link successfully
Expand Down Expand Up @@ -1935,6 +1981,60 @@ 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):
if not external_compiler_works():
self.skipTest()

tmpdir = TemporaryDirectory()
try:
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()
customize_compiler(c)
workdir = tmpdir.name
srcdir = os.path.dirname(llvmlite.tests.__file__) + "/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use os.path.join to make this portable and same for the path manipulation below and don't add the .c files to the repository. Store the code as inline strings and write them to a temporary directory during the test.


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

objects = c.compile([file1, file2], output_dir=workdir)
library_name = "foo"

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)
finally:
# Manually invoke cleanup to remove tmpdir, objects
# and static library created during test
tmpdir.cleanup()


class TestTimePasses(BaseTest):
def test_reporting(self):
mp = llvm.create_module_pass_manager()
Expand Down
3 changes: 3 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ def run(self):
from llvmlite.utils import get_library_files
self.distribution.package_data = {
"llvmlite.binding": get_library_files(),
"llvmlite.tests": ["*.c"]
Copy link
Contributor

Choose a reason for hiding this comment

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

When you inline the .c files, revert these changes.

}


Expand All @@ -110,6 +111,7 @@ def run(self):
from llvmlite.utils import get_library_files
self.distribution.package_data = {
"llvmlite.binding": get_library_files(),
"llvmlite.tests": ["*.c"]
}
install.run(self)

Expand Down Expand Up @@ -155,6 +157,7 @@ def run(self):
build_library_files(self.dry_run)
self.distribution.package_data.update({
"llvmlite.binding": get_library_files(),
"llvmlite.tests": ["*.c"]
})
# Run wheel build command
bdist_wheel.run(self)
Expand Down