Add support for importing symbols from static library #902

testhound · 2023-02-03T06:14:25Z

This pull request adds support for importing symbols from a static library into the execution engine.

gmarkall

Just had a quick skim (not a proper review) and added some comments on the diff. I haven't yet considered whether the strategy of compiling and archiving during the test run is the best path, but I've added a couple of pointers on the test code anyway.

gmarkall · 2023-02-03T10:16:08Z

llvmlite/tests/test_binding.py

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


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 ----------------------------------------------------------------------

gmarkall · 2023-02-03T10:20:21Z

llvmlite/tests/test_binding.py

+        self.assertEqual(mac_func(10, 10, 20), 120)
+        self.assertEqual(msub_func(10, 10, 20), 80)
+
+        os.unlink(objects[0])


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

gmarkall · 2023-02-03T10:26:35Z

docs/source/user-guide/binding/execution-engine.rst

+        Add the symbols from the specified static archive file to the execution
+        engine. It is a fatal error in LLVM if the *archive_file* does not exist.
+
+        * *archive* str: a path to the static object file


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

gmarkall · 2023-02-03T10:32:07Z

docs/source/user-guide/binding/execution-engine.rst

@@ -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


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

@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.

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.

…emporary directory and cleanup. Also fix typos in docs.

testhound · 2023-02-06T17:37:47Z

@gmarkall I made fixes/updates for all your comments.

…skip test if not

sklam · 2023-02-14T00:52:28Z

cc1: fatal error: /home/vsts/miniconda3/envs/cienv/lib/python3.8/site-packages/llvmlite/tests/a.c: No such file or directory

see error log at https://dev.azure.com/numba/numba/_build/results?buildId=14315&view=logs&j=fd18f1e2-1ae3-5f4d-9319-996752c1f67c&t=85c24074-1b78-5ee7-1f6d-6e0b210c5911&l=27

You'll need to add the .c files as package data. They are not being installed.

testhound · 2023-02-14T04:11:21Z

@sklam and @gmarkall all errors resolved.

gmarkall · 2023-02-14T11:37:52Z

ffi/executionengine.cpp

+    }
+
+    Expected<std::unique_ptr<object::Archive>> ArchiveOrError =
+        object::Archive::create(ArBufOrErr.get()->getMemBufferRef());


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

Suggested change

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

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

gmarkall

Just had a look at the C++ side of things, which seems mostly OK but I've added some comments on there.

gmarkall · 2023-02-14T11:38:21Z

ffi/executionengine.cpp

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


Suggested change

object::OwningBinary<object::Archive> owningBinaryArchive(std::move(Ar),

std::move(ArBuf));

OwningBinary<object::Archive> owningBinaryArchive(std::move(Ar),

std::move(ArBuf));

gmarkall · 2023-02-14T11:38:42Z

ffi/executionengine.cpp

+    object::OwningBinary<object::Archive> owningBinaryArchive(std::move(Ar),
+                                                              std::move(ArBuf));
+    engine->addArchive(std::move(owningBinaryArchive));
+    *OutError = LLVMPY_CreateString("");


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

Suggested change

*OutError = LLVMPY_CreateString("");

gmarkall · 2023-02-14T11:55:42Z

docs/source/user-guide/binding/execution-engine.rst

@@ -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


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.

gmarkall · 2023-02-14T11:57:50Z

ffi/executionengine.cpp

+    using namespace llvm::object;
+    auto engine = unwrap(EE);
+
+    ErrorOr<std::unique_ptr<MemoryBuffer>> ArBufOrErr =


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.

@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.

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

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

gmarkall · 2023-02-14T12:08:06Z

ffi/executionengine.cpp

+        std::string archiveErrStr =
+            "Unable to load archive: " + std::string(ArchiveName);
+        *OutError = LLVMPY_CreateString(archiveErrStr.c_str());


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));

testhound · 2023-02-14T21:11:52Z

@gmarkall all feedback incorporated.

testhound · 2023-02-28T02:37:18Z

@gmarkall ready for review.

apmasell · 2023-04-18T19:30:13Z

ffi/executionengine.cpp

+    using namespace llvm::object;
+    auto engine = unwrap(EE);
+
+    ErrorOr<std::unique_ptr<MemoryBuffer>> ArBufOrErr =


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

apmasell · 2023-04-18T19:32:01Z

ffi/executionengine.cpp

+        MemoryBuffer::getFile(ArchiveName);
+
+    std::error_code EC = ArBufOrErr.getError();
+    if (EC) {


This is more idiomatically written as:

if (!ArBufOrErr) {... }

without the creation of EC.

apmasell · 2023-04-18T19:34:16Z

ffi/executionengine.cpp

+
+    if (!ArchiveOrError) {
+        auto takeErr = ArchiveOrError.takeError();
+        LLVMErrorRef errorRef = wrap(std::move(takeErr));


This copy and move can be collapsed using wrap(ArchiveOrError.takeError())

apmasell · 2023-04-18T20:15:19Z

ffi/executionengine.cpp

+        return 1;
+    }
+
+    std::unique_ptr<MemoryBuffer> &ArBuf = ArBufOrErr.get();


These can also be simplified to:

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

apmasell · 2023-04-18T20:17:35Z

llvmlite/tests/test_binding.py

+            c = new_compiler()
+            customize_compiler(c)
+            workdir = tmpdir.name
+            srcdir = os.path.dirname(llvmlite.tests.__file__) + "/"


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.

apmasell · 2023-04-18T20:19:32Z

setup.py

@@ -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"]


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

… not work on Windows

testhound · 2023-05-10T22:15:19Z

@apmasell and @gmarkall I have updated the code. I am skipping the test on Windows and provided a detailed comment at the top of the test class. There is a error on macOS/Python 3.9 that appears related to a missing llvm-ar. All other versions of macOS succeed.

sklam · 2023-05-11T20:28:06Z

Note for 269cd6d
The Anaconda build of Python 3.9 is using a hardcoded path for llvm-ar from the build environment. I tested it on conda-forge build of Python 3.9 and it works without problem.

testhound · 2023-05-11T20:38:03Z

@apmasell and @gmarkall ready for review.

apmasell · 2023-05-15T14:21:36Z

If there's a way to get the binary to load on Windows using a non-distutils tool chain, that should be done rather than skipping the test.

sklam · 2023-05-15T16:09:59Z

llvmlite/tests/test_binding.py

+# Skip this test on Windows due to the fact that
+# the /GL cross-module optimization has the side-effect
+# of removing external symbols in this test case.


Maybe adding a DEF file can fix this; see https://devblogs.microsoft.com/oldnewthing/20140321-00/?p=1433

@sklam I have explored using a DEF file, however DEF files can only be provided to the "LINK" command for creating a dynamic library not the 'LIB" command that creates a static library. Additionally I have not found any way to modify the command line options to the compiler via the compile options. If I could modify the command line I would simply remove the "/GL" option.

testhound · 2023-05-15T17:12:10Z

If there's a way to get the binary to load on Windows using a non-distutils tool chain, that should be done rather than skipping the test.

Are there other

If there's a way to get the binary to load on Windows using a non-distutils tool chain, that should be done rather than skipping the test.

@apmasell unless you are aware of another solution it appears I will need to invoke the visual studio c++ compiler directly from the Python code for Windows plaltforms.

testhound · 2023-06-02T18:46:01Z

@apmasell, @sklam and @gmarkall Windows error resolved and ready for review.

testhound added 3 commits February 3, 2023 00:58

Add test files to be compiled for add_archive test case

9b891d0

Add support for add_archive, test case and documentation

ede2555

Fix flake8 errors

ca0abbf

testhound changed the title ~~Testhound/add archive~~ Add support for importing symbols from static library Feb 3, 2023

gmarkall reviewed Feb 3, 2023

View reviewed changes

gmarkall added the 2 - In Progress label Feb 6, 2023

Fix test_add_archive to create object files and static library in a t…

6a334a1

…emporary directory and cleanup. Also fix typos in docs.

testhound added 5 commits February 6, 2023 13:33

Fix formatting according to clang-format

88114fc

Merge branch 'main' into testhound/addArchive

77b6f89

Migrate from using deprecated distutils to setuptools

e56a4c7

Add code to test if calls to external compiler works and potentially …

9d07856

…skip test if not

Fix flake8 errors

e77b239

testhound added 2 commits February 13, 2023 22:38

Add C test files

90043d4

Add *.c files to all uses of pacakge data

cb9eb40

gmarkall reviewed Feb 14, 2023

View reviewed changes

Incorporate upstream comments

0ee2687

gmarkall mentioned this pull request Feb 14, 2023

LLVM ERROR: Symbol not found: __sync_fetch_and_add_4 running tests on armv7l,armv8l (armel) numba/numba#7452

Open

sklam added 3 - Ready for Review and removed 2 - In Progress labels Feb 16, 2023

gmarkall self-assigned this Mar 6, 2023

sklam added 4 - Waiting on reviewer and removed 3 - Ready for Review labels Mar 9, 2023

sklam added this to the v0.41.0 milestone Mar 9, 2023

apmasell reviewed Apr 18, 2023

View reviewed changes

testhound added 12 commits April 25, 2023 08:23

Use CCompiler.library_filename to get platform specific name

405b85c

Try instantiating unix compiler regardless of platform

c68001b

Make sure function names are not mangled and use C calling convention

ab90410

Fix flake8 issue

af07004

Test that the jit found the function address

22ddfc2

Use try .. finally block to clean up temp directory

f49b699

Delete jit object in case it is keeping archive open

608b6b8

Delete all files created during test

bc2023c

Don't use Path

281a9d3

Return and use tmpdir

2e7d474

Close file to resolve Windows open file issue

c9a59e5

Remove __ from from function names and document why the testcase does…

37182a7

… not work on Windows

testhound added 2 commits May 11, 2023 15:45

Skip test on macOS and Python 3.9 due to CI issue

269cd6d

Add string reason to skipTest

d42498d

sklam reviewed May 15, 2023

View reviewed changes

testhound added 5 commits May 24, 2023 12:53

Don't skip Windows to expose error

0effd26

Add Windows specific compilation

28e5ef7

Convert test case to a .c file

9f53121

Set debug=True

674ca5f

Fix flake8 and remove unused funtion

488dd58

apmasell approved these changes Jun 2, 2023

View reviewed changes

gmarkall modified the milestones: v0.41.0rc1, PR Backlog Jul 4, 2023

esc removed this from the PR Backlog milestone Jul 13, 2023

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

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

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

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

Add support for importing symbols from static library #902

Are you sure you want to change the base?

Add support for importing symbols from static library #902

Conversation

testhound commented Feb 3, 2023

gmarkall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testhound commented Feb 6, 2023

sklam commented Feb 14, 2023

testhound commented Feb 14, 2023

Choose a reason for hiding this comment

gmarkall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testhound Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testhound commented Feb 14, 2023

testhound commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testhound commented May 10, 2023

sklam commented May 11, 2023

testhound commented May 11, 2023

apmasell commented May 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testhound commented May 15, 2023

testhound commented Jun 2, 2023

testhound Feb 14, 2023 •

edited

Loading