[WIP] Provide a cdylib with the libm C ABI #192

gnzlbg · 2019-07-02T14:18:02Z

This PR adds a new crate libm-cdylib that implements the <math.h> API and that is ABI compatible with the platforms libm implementation.

Since libm is usually dynamically linked, this allows replacing the libm implementation of any program in the platform with the Rust implementation, e.g., using LD_PRELOAD and similar APIs.

The library is tested by just linking it to C programs that use <cmath.h> and checking that the library is called and produce expected "unique" results that differ from the system libraries. The C programs are linked to the system library, and then the math library is dynamically overriden.

Right now this works properly on macos, I'll iterate on the linux part of this on CI.

gnzlbg · 2019-07-02T14:18:45Z

crates/libm-cdylib/src/lib.rs

+#[cfg(not(test))]
+#[panic_handler]
+fn panic(_info: &core::panic::PanicInfo) -> ! {
+    unsafe { core::intrinsics::abort() }


TODO: I should probably call the C abort function here.

gnzlbg · 2019-07-02T14:19:11Z

crates/libm-cdylib/src/lib.rs

+
+#[cfg(not(test))]
+#[lang = "eh_personality"]
+extern "C" fn eh_personality() {}


There has to be a better way that avoids this, but I exhausted all my google-fu trying to find it.

This has to do with libcore itself being built with panic=unwind, but why not just link to std and have it provide these?

Because libstd would pull the system's libm.

crates/libm-cdylib/src/macros.rs

m1el · 2019-07-02T14:48:56Z

crates/libm-cdylib/src/test_utils.rs

+pub(crate) fn ctype_and_printf_format_specifier(x: &str) -> (&str, &str) {
+    match x {
+        "f32" => ("float", "%f"),
+        "f64" => ("double", "%f"),


should this be %lf?

IIRC, %f prints doubles. There is no format string for printing floats AFAIK - %f converts floats into a double, and prints that.

Would it make sense when %f is a double anyway? maybe %.24f for floats %.54f for doubles makes more sense?

Or even better maybe just %e

For the linking tests it doesn't really matter. I'll add a comment about this.

So I've added a comment explaining what this does right now.

If we wanted to verify the results of this lib, I think I would prefer doing so as part of the libm-test system. We could dynamically link libm as the system library, and just verify that the results of the C ABI match the results that are produced by the Rust library, or something like that.

The problem which checking the ABI here is that, if for some reason linking failed, or the symbol has a slightly different name, etc. the functions from the system library are invoked. So if we were to just simply verify the results, we can't tell whether its our libm or the system libm that's being invoked.

Maybe there is a way to reliably prevent libm from being linked on Linux, while still being able to link, e.g., fprintf and other parts of the standard library. On MacOSX, this is IMO not worth it - everything links libSystem.dylib, which always dynamically links libm.

Schultzer · 2019-07-02T15:55:06Z

Is there anyway we can fix the linking issue?

By looking the at the online docs the following functions should be present in the Kernel

atan
cos, cosf
j0, j1, jn

There are no sincos or exp10 functions in the Kernel, they are only present in the accelerate or simd framework as vectorized version.

gnzlbg · 2019-07-02T16:13:13Z

The problem is that some symbols are missing from the cdylib, like _memcpy, _memset, etc. I don't know why, and would prefer to merge this as is, and try to fix each one of those issues in follow up PRs. This issue appears related: rust-lang/rust#44871

cc @alexcrichton

alexcrichton · 2019-07-02T18:10:29Z

crates/libm-cdylib/src/lib.rs

+
+#[cfg(not(test))]
+#[lang = "eh_personality"]
+extern "C" fn eh_personality() {}


This has to do with libcore itself being built with panic=unwind, but why not just link to std and have it provide these?

alexcrichton · 2019-07-02T18:10:38Z

crates/libm-cdylib/src/lib.rs

+#![cfg_attr(not(test), no_std)]
+
+#[path = "../../../src/math/mod.rs"]
+mod libm;


Could this do extern crate libm instead?

That would pull in std, see below.

alexcrichton · 2019-07-02T18:11:44Z

crates/libm-cdylib/src/lib.rs

+    // fn tgamma(x: f64) -> f64: (42.) -> 42.;
+    // fn tgammaf(x: f32) -> f32: (42.) -> 42.;
+    fn trunc(x: f64) -> f64: (42.) -> 42.;
+    fn truncf(x: f32) -> f32: (42.) -> 42.;


If we really want to maintain a cdylib in this repository then I would prefer that this list were automatically inferred instead of duplicated with the definitions in the main source code. If the duplication is going to happen here then I would prefer that this cdylib is provided in a different repository than this one.

I'm still going to have to generate some of these by hand though because of the API mismatches between this libm, and C's libm.

Yeah even the original test harness does that, but it's not as explicit why it does it, I had to look up the musl code to understand what was going on in the test harness.

alexcrichton · 2019-07-02T18:12:38Z

crates/libm-cdylib/src/macros.rs

+        pub extern "C" fn $id($($arg: $arg_ty),*) -> $ret_ty {
+            // This just forwards the call to the appropriate function of the
+            // libm crate.
+            #[cfg(not(link_test))] {


Adding a pretty complicated macro to test what already has a pretty complicated of tests seems like overkill? Why does the cdylib itself need to be tested when it's all already tested elsewhere?

This tests that the C ABI we provide is compatible with <math.h>, which is what allows this library to be used from C.

We currently don't test it, and in fact, libm currently doesn't provide a C compatible ABI, which is why some of the functions in the lib.rs are commented out.

Simplest example is the libm functions returning tuples by value (e.g. (i32, f32)). The libm C ABI doesn't do that, and takes pointer arguments instead that are used as "out" parameters. So the cdylib is going to have to workaround those manually.

alexcrichton · 2019-07-02T18:13:55Z

crates/libm-cdylib/src/test_utils.rs

@@ -0,0 +1,143 @@
+use std::{fs, io, path::Path, process};


I personally feel like this file may be a bit over-the-top when testing, I'm not sure it's really all that necessary to be invoking Cargo from this crate's own test suite to test various C things?

If it's really this extensive then I would prefer this not live in this repository to help keep this repository manageable and not pull in a whole set of new maintenance.

It isn't expensive on my 8 year old system - cargo is invoked on every test, but the first invocation "locks", and once the first invocation completes, all of them just return "success - the library was already built".

I can use a lazy_static! to just call cargo once, but it felt like unnecessary complexity.

gnzlbg · 2019-07-03T10:32:31Z

So I've cleaned this up, and CI is green. I've added two MacOSX build jobs, and a nightly x86_64-linux build job. For the time being, the tests are only run there.

This is sub-optimal. Ideally, we would have a libm library, that exposes an extern "C" API, and a libm-rs library, that wraps that API in the F32Ext/F64Ext traits. We would then only exhaustively test libm and only have minimal tests in libm-rs. When compiling libm as a cdylib, we would use panic=abort, while we compiling it as a rlib, we would use panic=unwind (to allow linking it against both abort/unwind code).

But these are architectural decision that have to be made depending on the goals of the library, so I've opened #195 to discuss that first.

alexcrichton · 2019-07-08T15:28:52Z

It seems like this is intended to build on #198 so I'm going to hold off on reviewing/merging until that's landed.

gnzlbg commented Jul 2, 2019

View reviewed changes

crates/libm-cdylib/src/macros.rs Show resolved Hide resolved

gnzlbg commented Jul 2, 2019

View reviewed changes

crates/libm-cdylib/src/macros.rs Outdated Show resolved Hide resolved

m1el reviewed Jul 2, 2019

View reviewed changes

gnzlbg added 5 commits July 2, 2019 18:13

Provide a cdylib with the libm C ABI

e77e85d

Fix nitpicks

69f6e56

Document formatting specifiers

4db1c4f

Run libm-cdylib tests; add OSX test jobs

a549e37

Disable musl reference tests on OSX

164560a

gnzlbg force-pushed the cffi branch from 83bab81 to 4523a35 Compare July 2, 2019 16:16

alexcrichton reviewed Jul 2, 2019

View reviewed changes

gnzlbg added 3 commits July 3, 2019 12:24

Be a bit more robust around path handling; support CARGO_TARGET_DIR

53eab6a

Properly link the library

7fa7c15

Remove debug code

b20e63e

gnzlbg force-pushed the cffi branch from 9b25dca to b20e63e Compare July 3, 2019 10:26

jacobrosenthal mentioned this pull request Jul 3, 2019

Goals and usecases for libm #195

Closed

gnzlbg closed this Jul 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Provide a cdylib with the libm C ABI #192

[WIP] Provide a cdylib with the libm C ABI #192

gnzlbg commented Jul 2, 2019

gnzlbg Jul 2, 2019

gnzlbg Jul 2, 2019

alexcrichton Jul 2, 2019

gnzlbg Jul 2, 2019

m1el Jul 2, 2019

gnzlbg Jul 2, 2019

Schultzer Jul 2, 2019

Schultzer Jul 2, 2019 •

edited

Loading

gnzlbg Jul 2, 2019

gnzlbg Jul 2, 2019

gnzlbg Jul 2, 2019

Schultzer commented Jul 2, 2019 •

edited

Loading

gnzlbg commented Jul 2, 2019

alexcrichton Jul 2, 2019

alexcrichton Jul 2, 2019

gnzlbg Jul 2, 2019

alexcrichton Jul 2, 2019

gnzlbg Jul 2, 2019

gnzlbg Jul 2, 2019

Schultzer Jul 2, 2019

alexcrichton Jul 2, 2019

gnzlbg Jul 2, 2019 •

edited

Loading

alexcrichton Jul 2, 2019

gnzlbg Jul 2, 2019 •

edited

Loading

gnzlbg commented Jul 3, 2019 •

edited

Loading

alexcrichton commented Jul 8, 2019

[WIP] Provide a cdylib with the libm C ABI #192

[WIP] Provide a cdylib with the libm C ABI #192

Conversation

gnzlbg commented Jul 2, 2019

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

Schultzer Jul 2, 2019 • 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

Schultzer commented Jul 2, 2019 • edited Loading

gnzlbg commented Jul 2, 2019

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

Choose a reason for hiding this comment

gnzlbg Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

gnzlbg commented Jul 3, 2019 • edited Loading

alexcrichton commented Jul 8, 2019

Schultzer Jul 2, 2019 •

edited

Loading

Schultzer commented Jul 2, 2019 •

edited

Loading

gnzlbg Jul 2, 2019 •

edited

Loading

gnzlbg Jul 2, 2019 •

edited

Loading

gnzlbg commented Jul 3, 2019 •

edited

Loading