Make test suite pass on macOS on aarch64 #2008

bsteinb · 2020-12-23T21:08:33Z

While working on #2007 I tried to run cargo test in libc-test, which failed, because the definition of __darwin_mcontext64 was incomplete (see #1990). This adds definitions for the exception state and the floating point state as well.

libc-test still does not pass, because I use the type [u128; 32] for the __v field of __darwin_arm_neon_state64 (in C it is __uint128_t __v[32]. ctest2 does not translate u128 to __uint128_t and the generated C code does not compile. Any ideas for working around this?

rust-highfive · 2020-12-23T21:08:36Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

JohnTitor · 2020-12-28T22:44:41Z

Thanks!

libc-test still does not pass, because I use the type [u128; 32] for the __v field of __darwin_arm_neon_state64 (in C it is __uint128_t __v[32]. ctest2 does not translate u128 to __uint128_t and the generated C code does not compile. Any ideas for working around this?

I think we could use the same workaround in #2000.

bsteinb · 2020-12-29T08:52:46Z

I think we could use the same workaround in #2000.

I am not sure which workaround in #2000 you are referring to.

JohnTitor · 2020-12-29T11:45:14Z

I am not sure which workaround in #2000 you are referring to.

This one: #2000 (comment)
I guess you could nest the array to auto-derive and not to use u128.

bsteinb · 2020-12-29T20:16:39Z

I can do that, but it does not pass the tests either. It also does not have the correct alignment.

JohnTitor · 2020-12-30T08:58:02Z

I can do that, but it does not pass the tests either. It also does not have the correct alignment.

Hmm, then could you tweak the alignment with #[repr(align)]? The problem here is I don't have the target device and cannot debug at all :(

Thomasdezeeuw · 2020-12-30T10:23:02Z

This passes on macOS 10.15.7 (Catalina), what errors are you seeing @bsteinb?

JohnTitor · 2020-12-30T10:30:21Z

@Thomasdezeeuw The error should occur on Apple Silicon.

But yeah, I'd like to see the error too.

tamird · 2020-12-30T17:52:40Z

~~I have an Apple Silicon device; how can I help? How do I reproduce the failure?~~

Oops, turns out it's Intel silicon.

bsteinb · 2020-12-30T20:09:36Z

Right now, if I run cargo test in libc-test, ctest2 generates a function as follows:

u128(*__test_field_type___darwin_arm_neon_state64___v(struct __darwin_arm_neon_state64* b))[32] {                  
  return &b->__v;                                                                                                 
}

Which the C compiler refuses to compile, saying:

libc/target/debug/build/libc-test-377e25964c16162d/out/main.c:32704:24: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
                  u128(*__test_field_type___darwin_arm_neon_state64___v(struct __darwin_arm_neon_state64* b))[32] {
                        ^

It does not recognize u128 as a type. If I change that u128 to __uint128_t by hand, I can compile the generated file just fine. Fixing this for good would require teaching ctest2 to translate Rust's u128 into C's __uint128_t, which I am not sure how good of an idea that is, given that __uint128_t is (I think) a compiler internal type.

If I change the pull request to have __v: [[u64; 2]; 32] instead of __v: [u128; 32] the generated function is:

uint64_t(*__test_field_type___darwin_arm_neon_state64___v(struct __darwin_arm_neon_state64* b))[32][2] {           
  return &b->__v;                                                                                                 
}

About which the compiler complains:

libc/target/debug/build/libc-test-377e25964c16162d/out/main.c:32705:28: error: incompatible pointer types returning '__uint128_t (*)[32]' from a function with result type 'uint64_t (*)[32][2]' [-Werror,-Wincompatible-pointer-types]
                    return &b->__v;
                           ^~~~~~~

Also, as I mentioned above, std::mem::align_of::<[u64; 2]>() == 8 whereas std::mem::align_of::<u128>() == 16.

So, two possible solutions:

use u128 and either teach ctest2 how to translate it to C, or accept that the tests do not pass,
use [u64; 2], manually set the correct #[repr(align(16))] and accept that the tests do not pass.

tamird · 2020-12-30T22:36:49Z

Translating to __uint128_t seems fine, since that's the type that is used in the apple header: https://github.com/opensource-apple/cctools/blob/fdb4825f303fd5c0751be524babd32958181b3ed/include/mach/arm/_structs.h#L61.

bsteinb · 2020-12-31T11:18:26Z

I have pushed a change to use [u64; 2] with manual alignment instead of u128, since the same compatibility concerns that applied in #2000 should apply here. Also, there still seem to be concerns about using u128 in FFI rust-lang/rust#54341 (although on this particular target, u128 and __uint128_t seem to match).

This does not match the declaration in the C headers 100%, but is closer than what exists now and probably as close as possible without using u128.

Once u128 becomes viable (w.r.t. compatibility and FFI safety), this definition can be changed and with a translation for u128 implemented in ctest2, the test suite should then pass. There is an open pull request to make the original ctest support u128 etc.: gnzlbg/ctest#89

tamird · 2020-12-31T11:55:41Z

What about #1414 ? Would that approach work?

src/unix/bsd/apple/b64/aarch64.rs

JohnTitor · 2021-01-01T00:01:56Z

So, we should make it "at least it works" in this PR as u128 issue won't be resolved right away. For libc-test failure, we could skip the check with a comment.

bsteinb · 2021-01-01T01:52:40Z

With the tests ignoring the __v field and the changes from #2016, the test suite now builds and I get more fallout:

bad HW_MAXID value at byte 0: rust: 26 (0x1a) != c 28 (0x1c)
bad SHMLBA value at byte 1: rust: 16 (0x10) != c 64 (0x40)
bad boolean_t signed: rust: 0 (0x0) != c 1 (0x1)
bad max_align_t size: rust: 16 (0x10) != c 8 (0x8)
bad max_align_t align: rust: 16 (0x10) != c 8 (0x8)
size of max_align_t is 8 in C and 16 in Rust
bad PTHREAD_STACK_MIN value at byte 1: rust: 32 (0x20) != c 64 (0x40)

Would you like the fixes for those as separate pull requests or should I include them here?

JohnTitor · 2021-01-01T02:09:46Z

Would you like the fixes for those as separate pull requests or should I include them here?

Either way is fine to me if you separate commits!

bsteinb · 2021-01-01T02:20:06Z

The test suite now fails on x86_64, because of the changes to the HW_... constants in ea2bc2c. These seem to be specific to macOS 11 (Big Sur) and the tests still run on 10.15 (Catalina).

How is this normally handled?

JohnTitor · 2021-01-01T02:29:03Z

Let's skip the test for now, you should tweak here:

libc/libc-test/build.rs

Lines 193 to 204 in 7e7452f

    
           cfg.skip_const(move |name| { 
        
               match name { 
        
                   // These OSX constants are removed in Sierra. 
        
                   // https://developer.apple.com/library/content/releasenotes/General/APIDiffsMacOS10_12/Swift/Darwin.html 
        
                   "KERN_KDENABLE_BG_TRACE" | "KERN_KDDISABLE_BG_TRACE" => true, 
        
                   // FIXME: the value has been changed since Catalina (0xffff0000 -> 0x3fff0000). 
        
                   "SF_SETTABLE" => true, 
        
                   // FIXME: the value has been changed since Catalina (VM_FLAGS_RESILIENT_MEDIA is also contained now). 
        
                   "VM_FLAGS_USER_REMAP" => true, 
        
                   _ => false, 
        
               } 
        
           });

Big Sur is available as a preview on GHA but it may cause another issue and I'd avoid it.

JohnTitor

Looks good, thanks!

JohnTitor · 2021-01-05T01:48:16Z

@bors r+

bors · 2021-01-05T01:48:18Z

📌 Commit d57347a has been approved by JohnTitor

bors · 2021-01-05T02:00:42Z

⌛ Testing commit d57347a with merge fb0e64c...

Make test suite pass on macOS on aarch64 While working on #2007 I tried to run `cargo test` in `libc-test`, which failed, because the definition of `__darwin_mcontext64` was incomplete (see #1990). This adds definitions for the exception state and the floating point state as well. `libc-test` still does not pass, because I use the type `[u128; 32]` for the `__v` field of `__darwin_arm_neon_state64` (in C it is `__uint128_t __v[32]`. `ctest2` does not translate `u128` to `__uint128_t` and the generated C code does not compile. Any ideas for working around this?

bors · 2021-01-05T02:58:34Z

💔 Test failed - checks-actions

JohnTitor · 2021-01-05T03:53:56Z

src/unix/bsd/apple/b64/mod.rs

@@ -2,7 +2,6 @@

 pub type c_long = i64;
 pub type c_ulong = u64;
-pub type boolean_t = ::c_uint;
 pub type mcontext_t = *mut __darwin_mcontext64;


I'm not sure about aarch64-apple-ios but if it's the same as macOS, we could move this to align.rs on AArch64 (and mod.rs on x86_64).

JohnTitor · 2021-01-05T11:12:08Z

Let's try again, @bors r+

bors · 2021-01-05T11:12:09Z

📌 Commit ee84dae has been approved by JohnTitor

bors · 2021-01-05T11:12:18Z

⌛ Testing commit ee84dae with merge 2ec333c...

bors · 2021-01-05T12:15:02Z

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: JohnTitor
Pushing 2ec333c to master...

rust-highfive assigned JohnTitor Dec 23, 2020

rust-highfive added the S-waiting-on-review label Dec 23, 2020

JohnTitor mentioned this pull request Dec 30, 2020

Bump to 0.2.82 #2014

Merged

JohnTitor reviewed Dec 31, 2020

View reviewed changes

src/unix/bsd/apple/b64/aarch64.rs Outdated Show resolved Hide resolved

bsteinb added 4 commits January 1, 2021 01:25

provide complete definition of __darwin_mcontext64 on aarch64

10347e8

use [u64; 2] with manual alignment instead of u128

2ec0e61

ignore __v field of __darwin_arm_neon_state64 in tests

a7fd8a7

move declaration of __darwin_mcontext64 etc. into align.rs

a15491e

bsteinb force-pushed the macos-aarch64-mcontext branch from af5faa8 to a15491e Compare January 1, 2021 00:55

bsteinb mentioned this pull request Jan 1, 2021

fix glob symbol name on macOS aarch64 #2016

Closed

bsteinb added 4 commits January 1, 2021 02:41

provide declaration of boolean_t for macOS on aarch64

42fe290

add declaration of max_align_t for macOS on aarch64

bdc755b

fix values of constants on macOS aarch64

3fc0015

add new constants for macOS Big Sur

ea2bc2c

fix glob symbol name on macOS aarch64

d377b52

bsteinb changed the title ~~Provide complete definition of __darwin_mcontext64 on aarch64~~ Make test suite pass on macOS on aarch64 Jan 1, 2021

ignore changed constants in test for now

d57347a

JohnTitor approved these changes Jan 5, 2021

View reviewed changes

bors added S-waiting-on-bors and removed S-waiting-on-review labels Jan 5, 2021

bors added S-waiting-on-review and removed S-waiting-on-bors labels Jan 5, 2021

JohnTitor reviewed Jan 5, 2021

View reviewed changes

move declarations of mcontext_t and ucontext_t

ee84dae

bors added S-waiting-on-bors and removed S-waiting-on-review labels Jan 5, 2021

bors merged commit 2ec333c into rust-lang:master Jan 5, 2021

irgstg mentioned this pull request Nov 9, 2021

user_fpsimd_struct on aarch64 #2524

Closed

Make test suite pass on macOS on aarch64 #2008

Make test suite pass on macOS on aarch64 #2008

Uh oh!

Conversation

bsteinb commented Dec 23, 2020

Uh oh!

rust-highfive commented Dec 23, 2020

Uh oh!

JohnTitor commented Dec 28, 2020

Uh oh!

bsteinb commented Dec 29, 2020

Uh oh!

JohnTitor commented Dec 29, 2020

Uh oh!

bsteinb commented Dec 29, 2020

Uh oh!

JohnTitor commented Dec 30, 2020

Uh oh!

Thomasdezeeuw commented Dec 30, 2020

Uh oh!

JohnTitor commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tamird commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsteinb commented Dec 30, 2020

Uh oh!

tamird commented Dec 30, 2020

Uh oh!

bsteinb commented Dec 31, 2020

Uh oh!

tamird commented Dec 31, 2020

Uh oh!

Uh oh!

JohnTitor commented Jan 1, 2021

Uh oh!

bsteinb commented Jan 1, 2021

Uh oh!

JohnTitor commented Jan 1, 2021

Uh oh!

bsteinb commented Jan 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohnTitor commented Jan 1, 2021

Uh oh!

JohnTitor left a comment

Choose a reason for hiding this comment

Uh oh!

JohnTitor commented Jan 5, 2021

Uh oh!

bors commented Jan 5, 2021

Uh oh!

bors commented Jan 5, 2021

Uh oh!

bors commented Jan 5, 2021

Uh oh!

JohnTitor Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

JohnTitor commented Jan 5, 2021

Uh oh!

bors commented Jan 5, 2021

Uh oh!

bors commented Jan 5, 2021

Uh oh!

bors commented Jan 5, 2021

Uh oh!

Uh oh!

JohnTitor commented Dec 30, 2020 •

edited

Loading

tamird commented Dec 30, 2020 •

edited

Loading

bsteinb commented Jan 1, 2021 •

edited

Loading