Fix nitpicks

Author: gnzlbg

crates/libm-cdylib/src/lib.rs

@@ -14,24 +14,22 @@ mod test_utils;
 #[cfg(not(test))]
 #[panic_handler]
 fn panic(_info: &core::panic::PanicInfo) -> ! {
+    // TODO: just call libc::abort - fails to link
+    // unsafe { libc::abort() }
     unsafe { core::intrinsics::abort() }
 }
 
+// TODO: there has to be a way to avoid this
 #[cfg(not(test))]
 #[lang = "eh_personality"]
 extern "C" fn eh_personality() {}
 
-// All functions to be exported by the C ABI.
-// Includes a test input/output pair for testing.
-// The test output will be used to override the
-// result of the function, and the test input
-// is used to call the overriden function from C.
-// This is needed to make sure that we are linking
-// against this libm during testing, and not the
-// system's libm.
+// This macro exports the functions that are part of the C ABI.
 //
-//
-// FIXME: missing symbols: _memcpy, _memset, etc.
+// It generates tests that replace the implementation of the
+// function with a specific value, that then is used to check
+// that the library is properly linked.
+
 export! {
     fn acos(x: f64) -> f64: (42.) -> 42.;
     fn acosf(x: f32) -> f32: (42.) -> 42.;
@@ -41,6 +39,7 @@ export! {
     fn asinf(x: f32) -> f32: (42.) -> 42.;
     fn asinh(x: f64) -> f64: (42.) -> 42.;
     fn asinhf(x: f32) -> f32: (42.) -> 42.;
+    // FIXME: fails to link. Missing symbol: _memcpy
     // fn atan(x: f64) -> f64: (42.) -> 42.;
     fn atanf(x: f32) -> f32: (42.) -> 42.;
     fn atanh(x: f64) -> f64: (42.) -> 42.;
@@ -51,6 +50,7 @@ export! {
     fn ceilf(x: f32) -> f32: (42.) -> 42.;
     fn copysign(x: f64, y: f64) -> f64: (42., 42.) -> 42.;
     fn copysignf(x: f32, y: f32) -> f32: (42., 42.) -> 42.;
+    // FIXME: fails to link. Missing symbols
     //fn cos(x: f64) -> f64: (42.) -> 42.;
     //fn cosf(x: f32) -> f32: (42.) -> 42.;
     fn cosh(x: f64) -> f64: (42.) -> 42.;
@@ -83,7 +83,7 @@ export! {
     fn fmod(x: f64, y: f64) -> f64: (42., 42.) -> 42.;
     fn fmodf(x: f32, y: f32) -> f32: (42., 42.) -> 42.;
 
-    // different ABI than in C
+    // TODO: different ABI than in C - need a more elaborate wrapper
     // fn frexp(x: f64) -> (f64, i32): (42.) -> (42., 42);
     // fn frexpf(x: f32) -> (f32, i32): (42.) -> (42., 42);
 
@@ -92,7 +92,7 @@ export! {
     fn ilogb(x: f64) -> i32: (42.) -> 42;
     fn ilogbf(x: f32) -> i32: (42.) -> 42;
 
-    // FIXME: fail to link:
+    // FIXME: fails to link. Missing symbols
     // fn j0(x: f64) -> f64: (42.) -> 42.;
     // fn j0f(x: f32) -> f32: (42.) -> 42.;
     // fn j1(x: f64) -> f64: (42.) -> 42.;
@@ -105,7 +105,7 @@ export! {
     fn lgamma(x: f64) -> f64: (42.) -> 42.;
     fn lgammaf(x: f32) -> f32: (42.) -> 42.;
 
-    // different ABI
+    // TODO: different ABI than in C - need a more elaborate wrapper
     // fn lgamma_r(x: f64) -> (f64, i32): (42.) -> (42., 42);
     // fn lgammaf_r(x: f32) -> (f32, i32): (42.) -> (42., 42);
 
@@ -119,10 +119,10 @@ export! {
     fn log2f(x: f32) -> f32: (42.) -> 42.;
     fn pow(x: f64, y: f64) -> f64: (42., 42.) -> 42.;
     fn powf(x: f32, y: f32) -> f32: (42., 42.) -> 42.;
+
+    // FIXME: different ABI than in C - need a more elaborate wrapper
     // fn modf(x: f64) -> (f64, f64): (42.) -> (42., 42.);
     // fn modff(x: f32) -> (f32, f32): (42.) -> (42., 42.);
-
-    // different ABI
     // remquo
     // remquof
 
@@ -131,21 +131,24 @@ export! {
     fn scalbn(x: f64, n: i32) -> f64: (42., 42) -> 42.;
     fn scalbnf(x: f32, n: i32) -> f32: (42., 42) -> 42.;
 
-    // different ABI
+    // FIXME: different ABI than in C - need a more elaborate wrapper
     // fn sincos
     // fn sincosf
 
+    // FIXME: missing symbols - fails to link
     // fn sin(x: f64) -> f64: (42.) -> 42.;
     // fn sinf(x: f32) -> f32: (42.) -> 42.;
 
     fn sinh(x: f64) -> f64: (42.) -> 42.;
     fn sinhf(x: f32) -> f32: (42.) -> 42.;
     fn sqrt(x: f64) -> f64: (42.) -> 42.;
     fn sqrtf(x: f32) -> f32: (42.) -> 42.;
+    // FIXME: missing symbols - fails to link
     // fn tan(x: f64) -> f64: (42.) -> 42.;
     // fn tanf(x: f32) -> f32: (42.) -> 42.;
     fn tanh(x: f64) -> f64: (42.) -> 42.;
     fn tanhf(x: f32) -> f32: (42.) -> 42.;
+    // FIXME: missing symbols - fails to link
     // fn tgamma(x: f64) -> f64: (42.) -> 42.;
     // fn tgammaf(x: f32) -> f32: (42.) -> 42.;
     fn trunc(x: f64) -> f64: (42.) -> 42.;

crates/libm-cdylib/src/macros.rs

@@ -3,21 +3,33 @@ macro_rules! export {
        ($($test_arg:expr),*) -> $test_ret:expr;) => {
         #[no_mangle]
         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))] {
+                libm::$id($($arg),*)
+            }
+            // When generating the linking tests, we return a specific incorrect
+            // value. This lets us tell this libm from the system's one appart:
             #[cfg(link_test)] {
+                // TODO: as part of the rountrip, we probably want to verify
+                // that the argument values are the unique ones provided.
                 let _ = libm::$id($($arg),*);
                 $test_ret as _
             }
-            #[cfg(not(link_test))] {
-                libm::$id($($arg),*)
-            }
         }
 
         #[cfg(test)]
         paste::item! {
+            // These tests check that the library links properly.
             #[test]
-            fn [<$id _test>]() {
+            fn [<$id _link_test>]() {
                 use crate::test_utils::*;
-                let (cret_t, c_format_s) = ctype_and_cformat(stringify!($ret_ty));
+
+                // Generate a small C program that calls the C API from
+                // <math.h>. This program prints the result into an appropriate
+                // type, that is then printed to stdout.
+                let (cret_t, c_format_s)
+                    = ctype_and_printf_format_specifier(stringify!($ret_ty));
                 let ctest = format!(
                     r#"
                         #include <math.h>
@@ -40,8 +52,13 @@ macro_rules! export {
                 let src_path = std::path::Path::new(src);
                 let bin_path = std::path::Path::new(bin);
                 write_to_file(&src_path, &ctest);
+
+                // We now compile the C program into an executable, make sure
+                // that the libm-cdylib has been generated (and generate it if
+                // it isn't), and then we run the program, override the libm
+                // dynamically, and verify the result.
                 compile_file(&src_path, &bin_path);
-                compile_lib();
+                compile_cdylib();
                 check(&bin_path, $test_ret as $ret_ty)
             }
         }

crates/libm-cdylib/src/test_utils.rs

@@ -1,12 +1,18 @@
 use std::{fs, io, path::Path, process};
 
+/// Writes the `content` string to a file at `path`.
 pub(crate) fn write_to_file(path: &Path, content: &str) {
     use io::Write;
     let mut file = fs::File::create(&path).unwrap();
     write!(file, "{}", content).unwrap();
 }
 
-pub(crate) fn compile_lib() {
+/// Compiles the libm-cdylib library as a C library.
+///
+/// This just compiles it once, all other times it just
+/// succeeds. We compile it with --cfg link_test to
+/// enable the tests.
+pub(crate) fn compile_cdylib() {
     let mut cmd = process::Command::new("cargo");
     cmd.arg("build");
     if cfg!(release_profile) {
@@ -16,8 +22,16 @@ pub(crate) fn compile_lib() {
     handle_err("lib_build", &cmd.output().unwrap());
 }
 
+/// Compiles the test C program with source at `src_path` into
+/// an executable at `bin_path`.
 pub(crate) fn compile_file(src_path: &Path, bin_path: &Path) {
     let mut cmd = process::Command::new("CC");
+    // We disable the usage of builtin functions, e.g., from libm.
+    // This should ideally produce a link failure if libm is not dynamically
+    // linked.
+    //
+    // On MacOSX libSystem is linked (for printf, etc.) and it links libSystem_m
+    // transitively, so this doesn't help =/
     cmd.arg("-fno-builtin")
         .arg("-o")
         .arg(bin_path)
@@ -28,15 +42,17 @@ pub(crate) fn compile_file(src_path: &Path, bin_path: &Path) {
     );
 }
 
+/// Run the program and verify that it prints the expected value.
 pub(crate) fn check<T>(path: &Path, expected: T)
 where
     T: PartialEq + std::fmt::Debug + std::str::FromStr,
     <T as std::str::FromStr>::Err: std::fmt::Debug,
 {
     let mut cmd = process::Command::new(path);
 
+    // Find the cdylib - we just support standard locations for now.
     let libm_path = format!(
-        "../../target/{}/liblibm.",
+        "../../target/{}/liblibm",
         if cfg!(release_profile) {
             "release"
         } else {
@@ -46,6 +62,7 @@ where
 
     // Replace libm at runtime
     if cfg!(target_os = "macos") {
+        // for debugging:
         // cmd.env("DYLD_PRINT_LIBRARIES", "1");
         // cmd.env("X", "1");
         cmd.env("DYLD_FORCE_FLAT_NAMESPACE", "1");
@@ -54,17 +71,20 @@ where
             format!("{}.{}", libm_path, "dylib"),
         );
     } else if cfg!(target_os = "linux") {
-        cmd.env("LD_PRELOAD", format!("{}.{}", libm_path, "so"))
+        cmd.env("LD_PRELOAD", format!("{}.{}", libm_path, "so"));
     }
+    // Run the binary:
     let output = cmd.output().unwrap();
     handle_err(&format!("run file: {}", path.display()), &output);
+    // Parse the result:
     let result = String::from_utf8(output.stdout.clone())
         .unwrap()
         .parse::<T>();
 
     if result.is_err() {
         panic!(format_output("check (parse failure)", &output));
     }
+    // Check the result:
     let result = result.unwrap();
     assert_eq!(result, expected, "{}", format_output("check", &output));
 }
@@ -98,7 +118,9 @@ pub(crate) fn format_output(
     s
 }
 
-pub(crate) fn ctype_and_cformat(x: &str) -> (&str, &str) {
+/// For a given Rust type `x`, this prints the name of the type in C,
+/// as well as the printf format specifier used to print values of that type.
+pub(crate) fn ctype_and_printf_format_specifier(x: &str) -> (&str, &str) {
     match x {
         "f32" => ("float", "%f"),
         "f64" => ("double", "%f"),

src/math/acos.rs

@@ -62,7 +62,7 @@ fn r(z: f64) -> f64 {
 /// Returns values in radians, in the range of 0 to pi.
 #[inline]
 #[cfg_attr(all(test, assert_no_panic), no_panic::no_panic)]
-extern "C" pub fn acos(x: f64) -> f64 {
+pub fn acos(x: f64) -> f64 {
     let x1p_120f = f64::from_bits(0x3870000000000000); // 0x1p-120 === 2 ^ -120
     let z: f64;
     let w: f64;