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

Refactor testing framework into a libm-test #198

Closed
wants to merge 43 commits into from

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jul 4, 2019

cc @alexcrichton

This PR removes the old testing framework, and instead adds two new crates, libm-analyze, and libm-test.

libm-analize provides a proc macro, that expands its argument for each of the public functions in the libm crate, providing it its signature:

macro_rules! user_defined_macro {
    (
        id: $id:ident;      // The function name
        api_kind: $api_kind:ident;  // The variant name in ApiKind enum for this fn
        arg_tys: $($arg_tys:ty),*;  // The types of function arguments
        arg_ids: $($arg_ids:ident),*;  // The names of function arguments
        ret_ty: $ret_ty:ty;  // The return type
    ) => {};
}
libm_analyze::for_each_api!(user_defined_macro);

This is then re-used by the libm-test crate, to implement the random tests for each of the libm APIs that were previously provided by the build-script. It uses the same proc macro to implement exhaustiveness tests ala #186 , and it replaces with it most of libm-bench as well.

The libm_analyize::for_each_api macro does a sanity check of the libm API when the libm-analyze tests run. It marks dozens and dozens of issues (missing inline, missing no_panic, no extern "C", returning types that are not repr(C), mismatches with musl API, etc.). The list is in this gist. Fixing all of those takes time, so the errors are silenced by default, except when building the libm-analyze crate. At some point we should turn these into hard errors. EDIT: I started doing that here, but I've left hrad API breaking changes to subsequent PRs.

gnzlbg added 10 commits July 4, 2019 11:39
This proc macro takes a macro of the form:

```
macro_rules! nop {
    (
        id: $id:ident;
        arg_tys: $($arg_tys:ty),*;
        arg_ids: $($arg_ids:ident),*;
        ret: $ty:ty;
    ) => {};
}
```

as input and expands it on every public libm API. This facilitates generating
tests, benchmarks, etc. for all APIs.
Currently, only the randomly generated tests from the old build.rs
are generated.
}
}

// Some interesting random values
Copy link
Contributor

Choose a reason for hiding this comment

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

We should properly think about adding positive and negative NAN, MIN_POSITIVE, MAX and, subnorm which are 1.401298464324817070923730e-45f32 and 4.9406564584124654e-324f64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to keep the tests testing exactly the same thing as in the old build.rs, to keep the test from breaking on CI due to too many new issues - there were already enough simpler to fix issues being raised by libm-analyze. I think we should move this to do something cleverer here as we fix the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some of these values, haven't added the subnormal yet, I thought MIN_POSITIVE and -MIN_POSITIVE where the two subnormal values. Is that incorrect ?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 5, 2019

The FMA implementation is quiet broken. Instead of using wrapping_.. appropriately, it uses the non-wrapping operators everywhere. I've disabled the system tests for it, even though I landed a couple of bug fixes.

I've removed nexttoward/nexttowardf for similar reasons. I don't think we can implement these correctly in Rust without a f80 floating point type with 64-bit significand.

@alexcrichton
Copy link
Member

I'm sorry I just can't keep reviewing multi-thousand line complete reorganizations of crates. Can this be split up? Some changes I would prefer to not happen after a skim are:

  • The main sources of the crate should continue to live in /src/*.rs, not in /crates/libm/src/*.rs
  • Rust-defined functions should not have extern ABIs, only if we add a C ABI crate should they be added.

I again do not want to lose the current functionality of tests which I'm afraid a refactoring like this could possibly lose. Can all these changes be disentangled from one another to avoid having a massive blocking PR?

@alexcrichton
Copy link
Member

Also if things like the fma implementation are broken can a separate PR be used to track that? It gets very difficult to manage this when so many fixes are piled in one place.

Additionally I do not want to add clippy to CI here, it seems like it will cause endless headaches of maintenance over time as clippy lints change.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 9, 2019

I again do not want to lose the current functionality of tests which I'm afraid a refactoring like this could possibly lose.

This change does not lose any tests AFAICT, all APIs are automatically tested, and compilation fails if one is not. One can ignore the results of tests of some APIs, if these happen to be broken, but all original tests pass here.

Rust-defined functions should not have extern ABIs, only if we add a C ABI crate should they be added.

That's being discussed in #195 so I'd rather wait to see how that resolves there.

Can this be split up?

Sure.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 10, 2019

@alexcrichton so PRs #203, #204, #205, #206, #207, #208, and #209 are the ground work for this. After those get resolved. I'll rebase here and see what the next steps are.

The first step would be to just submit a PR adding the libm-analyze crate, and migrating the existing tests to use it. Once that is resolved, further PRs would then add in parallel the remaining tests, and migrate libm-bench to use it as well.

@alexcrichton
Copy link
Member

Ok, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants