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

Calling std::thread::current() in global allocator results in non-obvious error #115209

Open
GoldsteinE opened this issue Aug 25, 2023 · 6 comments
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@GoldsteinE
Copy link
Contributor

Global allocator is a special piece of code because it runs before main(). Parts of standard library are not available at this time and it’s (as far as I know) not documented in safety contract of GlobalAlloc, or, for that matter, anywhere else.

It seems like some sort of a doc specifying which functions are “before-main-safe” (so can be used in GlobalAlloc and #[ctor]) would be really useful. On top of that, I think that identifying the current thread should be allowed in the global allocator.

I tried this code (playground):

use std::alloc::{GlobalAlloc, Layout, System};

struct MyGlobalAlloc;

unsafe impl GlobalAlloc for MyGlobalAlloc {
    unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
        std::panic::catch_unwind(|| std::thread::current()).ok();
        System.alloc(layout)
    }
    unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
        System.dealloc(ptr, layout)
    }
}

#[global_allocator]
static _ALLOCATOR: MyGlobalAlloc = MyGlobalAlloc;

fn main() {}

I expected to see this happen: nothing at all, it should just work.

Instead, this happened:

thread panicked while processing panic. aborting.

I’ve also seen:

fatal runtime error: assertion failed: thread_info.is_none()

but I can’t reliably reproduce it.

Meta

rustc --version --verbose:

rustc 1.73.0-nightly (08d00b40a 2023-08-09)
binary: rustc
commit-hash: 08d00b40aef2017fe6dba3ff7d6476efa0c10888
commit-date: 2023-08-09
host: x86_64-unknown-linux-gnu
release: 1.73.0-nightly
LLVM version: 17.0.0

but it reproduces on stable, beta and nightly.

I’m not actually sure if this is a bug, but I think one of the following must be true:

  1. Either global allocator should be able to call std::thread::current().id() without crashing the program,
  2. or its inability to do so should be documented in docs for GlobalAlloc, #[global_allocator], or std::thread::current(), preferably all of them.
@GoldsteinE GoldsteinE added the C-bug Category: This is a bug. label Aug 25, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 25, 2023
@GoldsteinE GoldsteinE changed the title Calling std::thread::current() in global allocator results in non-obvious error Calling std::thread::current() in global allocator results in non-obvious error Aug 25, 2023
@GoldsteinE GoldsteinE changed the title Calling std::thread::current() in global allocator results in non-obvious error Calling `std::thread::current() in global allocator results in non-obvious error Aug 25, 2023
@GoldsteinE GoldsteinE changed the title Calling `std::thread::current() in global allocator results in non-obvious error Calling std::thread::current() in global allocator results in non-obvious error Aug 25, 2023
@asokol123
Copy link

In macos result is always [1] 99360 illegal hardware instruction cargo run

Meta

rustc --version --verbose

rustc 1.74.0-nightly (58eefc33a 2023-08-24)
binary: rustc
commit-hash: 58eefc33adf769a1abe12ad94b3e6811185b4ce5
commit-date: 2023-08-24
host: aarch64-apple-darwin
release: 1.74.0-nightly
LLVM version: 17.0.0

@ChrisDenton
Copy link
Member

ChrisDenton commented Aug 25, 2023

Pre-main behaviour was also discussed in #110708. The feeling was that in general we should endeavour to make as much of the std work as possible and document where it doesn't. The complicating factor is that this is highly platform specific and there are likely to always be caveats and shifting behaviour as implementations evolve.

But yes, documenting it is definitely worth doing if someone wants to help with that.

@GoldsteinE
Copy link
Contributor Author

GoldsteinE commented Aug 25, 2023

I’d help with documenting, but I’m not sure where this should be documented. I’m also not sure whether it should be documented as “you can use these APIs pre-main” or “you can’t use these other APIs pre-main”.

A separate question is why std::thread::current() is not pre-main-safe. Linked issue only talks about it being non-post-main-safe.

@asokol123
Copy link

Also, this code hangs in macos, so maybe not related to pre-main:

use std::alloc::{GlobalAlloc, Layout, System};
use std::sync::atomic::AtomicBool;
use std::thread;

static IS_MAIN_STARED: AtomicBool = AtomicBool::new(false);

struct MyGlobalAlloc;

unsafe impl GlobalAlloc for MyGlobalAlloc {
    unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
        if IS_MAIN_STARED.load(std::sync::atomic::Ordering::SeqCst) {
            std::panic::catch_unwind(std::thread::current).ok();
        }
        System.alloc(layout)
    }
    unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
        System.dealloc(ptr, layout)
    }
}

#[global_allocator]
static _ALLOCATOR: MyGlobalAlloc = MyGlobalAlloc;

fn main() {
    IS_MAIN_STARED.store(true, std::sync::atomic::Ordering::SeqCst);
    let e = thread::spawn(|| {});
    e.join().unwrap();
}

If you remove #[global_allocator] or thread::spawn, it works normally

rustc --version --verbose

rustc 1.74.0-nightly (58eefc33a 2023-08-24)
binary: rustc
commit-hash: 58eefc33adf769a1abe12ad94b3e6811185b4ce5
commit-date: 2023-08-24
host: aarch64-apple-darwin
release: 1.74.0-nightly
LLVM version: 17.0.0

@saethlin saethlin added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 25, 2023
@ChrisDenton
Copy link
Member

Also, this code hangs in macos, so maybe not related to pre-main:

Oh hm, yeah it seems more likely that the issue is std::thread::current itself allocating.

@lolbinarycat
Copy link

it seems that it allocates because it uses thread_local!, it's the same underlying issue as #116390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants