Android: Added support for prctl handling thread names

[YohDeadfall]

Addresses the first part of #3618.

[RalfJung]

Thanks for the PR!

There's currently a bit of a queue and I have fairly little time, so it may take a bit until I can review this. (Or maybe someone else will pick up the review.)

[YohDeadfall]

Sure, and meanwhile I will fix issues reported by the CI.

A side question, I noticed that the repository doesn't use git hooks like the rust repo. Is it intentional?

[saethlin]

the repository doesn't use git hooks

Repos can't have git hooks. What exactly are you referring to?

[YohDeadfall]

The main repo installs them on bootstrap: https://github.com/rust-lang/rust/blob/1a5a2240bc1b8cf0bcce7acb946c78d6493a4fd3/src/bootstrap/src/core/build_steps/setup.rs#L462.

[saethlin]

The equivalent for this repo would be a miri-script feature. I've always found git hooks more annoying than time-saving, so it's not something I would spend my own time on. But if you really like them, you're welcome to propose something.

[YohDeadfall]

A script feature would be also fine for me. The key point is to have something which can be used to run the same stuff as in the CI but locally. That thing was implemented in bevy and pgrx is considering that too.

[RalfJung]

To have CI check that your PR works, please change this line

miri/ci/ci.sh

Line 157 in 18a8ab7

TEST_TARGET=aarch64-linux-android run_tests_minimal $BASIC $UNIX pthread --skip threadname --skip pthread_cond_timedwait

to something like

... run_tests_minimal $BASIC $UNIX threadname pthread --skip pthread_cond_timedwait

[YohDeadfall]

@RalfJung, I made changes almost like you requested, but I didn't move prctl handling into a separate function since it takes not so much space now and there's not so much code around. Should it still be moved to a separate function? I checked the code around, so I guess it can be done like futex and move to the same file as where other thread handling functions reside.

Another thing I still cannot finish is the test. I tried to run it locally, but it ends up with the following message:

full stderr:
error[E0425]: cannot find value `PR_SET_NAME` in crate `libc`
  --> tests/pass-dep/libc/prctl.rs:28:40
   |
LL |             unsafe { libc::prctl(libc::PR_SET_NAME, name.as_ptr().cast::<*const c_char>()) }
   |                                        ^^^^^^^^^^^ help: a constant with a similar name exists: `PR_SET_VMA`
   |
  ::: /home/yoh/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libc-0.2.158/src/unix/linux_like/android/mod.rs:3138:1
   |
LL | pub const PR_SET_VMA: ::c_int = 0x53564d41;
   | ----------------------------- similarly named constant `PR_SET_VMA` defined here

error[E0425]: cannot find value `PR_GET_NAME` in crate `libc`
  --> tests/pass-dep/libc/prctl.rs:41:35
   |
LL |                 libc::prctl(libc::PR_GET_NAME, name.as_mut_ptr().cast::<*mut c_char>())
   |                                   ^^^^^^^^^^^ not found in `libc`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0425`.

Meanwhile these constants exist and I definitely can compile the code.

[tiif]

From what I understand from the CI test error,

  Error: the compiler panicked
  Error: you likely need to bless the tests with `./miri test --bless`
  Error: 
     0: ui tests in tests/pass for aarch64-linux-android failed
     1: tests failed
  
  Location:
     /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ui_test-0.26.5/src/lib.rs:357
  
  Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
  Run with RUST_BACKTRACE=full to include source snippets.
  error: test failed, to rerun pass `--test ui`
  
  Caused by:
    process didn't exit successfully: `/Users/runner/work/miri/miri/target/debug/deps/ui-0ba442cf755d06fa empty_main integer vec string btreemap hello hashmap heap_alloc align panic/panic panic/unwind concurrency/simple atomic libc-mem libc-misc libc-random env num_cpus pthread threadname --skip pthread_cond_timedwait` (exit status: 1)
  Error: command exited with non-zero code `cargo +miri test --locked --all-features --manifest-path /Users/runner/work/miri/miri/./Cargo.toml -- empty_main integer vec string btreemap hello hashmap heap_alloc align panic/panic panic/unwind concurrency/simple atomic libc-mem libc-misc libc-random env num_cpus pthread threadname --skip pthread_cond_timedwait`: 1
  error: test got exit status: 101, but expected 0
   = note: the compiler panicked
  
  error: actual output differed from expected
  Execute `./miri test --bless` to update `tests/pass/concurrency/threadname.stderr` to the actual output
  --- tests/pass/concurrency/threadname.stderr
  +++ <stderr output>
  +thread 'rustc' panicked at src/helpers.rs:LL:CC:
  +failed to find required Rust item: ["libc", "PR_SET_NAME"]

I guess it is because aarch64-linux-android platform in libc indeed doesn't have PR_SET_NAME. There is no result when I try to search it in libc https://docs.rs/libc/latest/aarch64-linux-android/libc/?search=PR_SET_NAME.

[YohDeadfall]

So, libc hasn't these constants for Android, but as soon as rust-lang/libc#3941 merged the CI will be unblocked. Surely it requires a new version of libc released.

[tiif]

Are there any more android target that doesn't support these two flags?

[YohDeadfall]

According to the git history these constants were added in 2011, so I guess it's supported everywhere now.

[RalfJung]

So, libc hasn't these constants for Android, but as soon as rust-lang/libc#3941 merged the CI will be unblocked. Surely it requires a new version of libc released.

That could take a while, so we shouldn't block on it. You can hard-code the numbers for now and leave a FIXME pointing to the libc PR.

[bors]

☔ The latest upstream changes (presumably #3913) made this pull request unmergeable. Please resolve the merge conflicts.

[YohDeadfall]

@RalfJung, I need a bit of help to finish this pull request. I run the threadname test and got this error:

error: Undefined Behavior: scalar size mismatch: expected 8 bytes but got 4 bytes instead
  --> /home/yoh/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/pal/unix/thread.rs:120:13
   |
LL | /             libc::prctl(
LL | |                 PR_SET_NAME,
LL | |                 name.as_ptr(),
LL | |                 0 as libc::c_ulong,
LL | |                 0 as libc::c_ulong,
LL | |                 0 as libc::c_ulong,
LL | |             );
   | |_____________^ scalar size mismatch: expected 8 bytes but got 4 bytes instead
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: BACKTRACE on thread `unnamed-2`:
   = note: inside `std::sys::pal::unix::thread::Thread::set_name` at /home/yoh/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/pal/unix/thread.rs:120:13: 126:14
   = note: inside closure at /home/yoh/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/mod.rs:514:17: 514:44
   = note: inside `<{closure@std::thread::Builder::spawn_unchecked_<'_, {closure@tests/pass/concurrency/threadname.rs:12:16: 12:23}, ()>::{closure#1}} as std::ops::FnOnce<()>>::call_once - shim(vtable)` at /home/yoh/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71
   = note: inside `<std::boxed::Box<dyn std::ops::FnOnce()> as std::ops::FnOnce<()>>::call_once` at /home/yoh/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/boxed.rs:2453:9: 2453:52
   = note: inside `<std::boxed::Box<std::boxed::Box<dyn std::ops::FnOnce()>> as std::ops::FnOnce<()>>::call_once` at /home/yoh/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/boxed.rs:2453:9: 2453:52
   = note: inside `std::sys::pal::unix::thread::Thread::new::thread_start` at /home/yoh/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/pal/unix/thread.rs:105:17: 105:64

The return types both for pthread_setname_np returns c_int, and prctl does that as well. The first argument of prctl is c_int and it's passed as c_int in std:

#[cfg(target_os = "android")]
pub fn set_name(name: &CStr) {
    const PR_SET_NAME: libc::c_int = 15;
    unsafe {
        libc::prctl(
            PR_SET_NAME,
            name.as_ptr(),
            0 as libc::c_ulong,
            0 as libc::c_ulong,
            0 as libc::c_ulong,
        );
    }
}

So, I'm a bit lost. What the compiler expects to be 8-bytes long?

[bors]

☔ The latest upstream changes (presumably #3918) made this pull request unmergeable. Please resolve the merge conflicts.

[tiif]

The maintainers look busy, and I am waiting for review too, so I might as well help with this ;)

The problem seems to be in this line, it works with let tid = this.pthread_self()?;.

The test report UB because of the size mismatch in this line:

miri/src/shims/unix/thread.rs

Line 75 in bf0801d

let thread = thread.to_int(this.libc_ty_layout("pthread_t").size)?;

[RalfJung]

@rustbot author

[bors]

☔ The latest upstream changes (presumably #3953) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

All right I think all the other cleanup is done. :D So after a rebase + squash this PR should be only adding the new shims, and extend the threadname test accordingly.

[bors]

☔ The latest upstream changes (presumably #3978) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

@rustbot author

[YohDeadfall]

Just a few hundred comments more and that pull request can be a new "legend" in the rustc contribution docs. Unfortunately not because there's something interesting going on, but because I constantly forget something. So, thank you for your patience (:

[RalfJung]

I think this is the last round. :)
Please squash (with get rebase --keep-base) after resolving the last two comments.

[RalfJung]

Awesome, now please just squash (with git rebase --keep-base) the commits :)

[RalfJung]

Thanks!

@bors r+

[bors]

📌 Commit 4f79c0e has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit 4f79c0e with merge 9a228e5...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 9a228e5 to master...