use #[naked] for __rust_probestack

[folkertdev]

This should work now. #[naked] takes care of adding the directives that define_rust_probestack added.

I deliberately did not format/indent the assembly code to make review easier. I could do that in a separate commit it that is desirable.

Let's see if CI is OK with this.

[tgross35]

Could you mention in the doc comment that the custom ABI is the reason this is unsafe?

[folkertdev]

Do you mean in the module doc comment? We can add some specific details (about what registers are used etc) if we add a doc comment to the function itself.

[tgross35]

I was referring to the comments above each of the functions, I guess they're not actually doc comments though they could be. Something like (hence the `unsafe`, since the ABI does not actually match `extern "C"`) in the paragraph describing ABI is sufficient, unless you want to add more. Just to have a note that it isn't supposed to be extern "C".

[tgross35]

That reminded me of a few other cases we have where naked functions are extern "C" but that doesn't really make sense. Seems like a somewhat common issue, I wrote up rust-lang/rust#140566 as a possible way to make this more clear (not for this pr of course).

[tgross35]

Thanks for putting this together, this is a nice bit of simplification. After the rustc_std_internal_symbol change I guess it's probably easiest to wait until the beta bump to merge this, since that's in only ~8 days and avoids any bootstrap problems.

I deliberately did not format/indent the assembly code to make review easier. I could do that in a separate commit it that is desirable.

Feel free to do any cleanup you see fit. Split NFC commits are great, GH's "ignore whitespace" option on the review UI works for everything else 🙂

[folkertdev]

Hmm, formatting does not really help much, because formatting in asm is undefined. So let's leave it as it is.

I did merge the definition for sdx into the normal x86_64 version (one day that can use #[cfg()] in the asm block, for now a macro will do). That removes a bunch of duplication.

[bjorn3]

After the rustc_std_internal_symbol change I guess it's probably easiest to wait until the beta bump to merge this, since that's in only ~8 days and avoids any bootstrap problems.

There will need to be a change on the rustc side anyway when updating compiler-builtins. #[rustc_std_internal_symbol] changes the exact symbol name, so the __rust_probestack reference inside cg_llvm needs to be wrapped in a mangle_internal_symbol call to match what compiler-builtins will define when it adds #[rustc_std_internal_symbol].

[folkertdev]

Well currently the bootstrap compiler rejects #[unsafe(naked)], so we're using #[cfg(bootstrap)] to make that work in some other places in compiler-builtins. Similar, bootstrap would still reject #[rustc_std_internal_symbol] in combination with #[naked].

So yeah waiting for the new beta is probably easiest (not sure how rustweek impacts the schedule there, but we'll see)

[tgross35]

The global->naked LGTM, with the optional nit that you can indent the text by two places so it's nested within the naked_asm! macro (currently asm labels are in the first column). I'm wondering about rustc_std_internal_symbol though, won't this get into a bootstrap loop?

If so, I think we could leave those unmangled for now with a FIXME to change it once this repo becomes a subtree, to make things a bit easier (unfortunately never got a chance to do that this week).

[folkertdev]

The indenting is nicer, yes (though rustfmt is still a bit wonkey sometimes).

The PR allowing rustc_std_internal_symbol on naked functions was merged on May 2nd rust-lang/rust#140552, and the bootstrap compiler was bumped after that I believe, so unless I'm missing something I think that should be OK?

[tgross35]

I think the attribute would be accepted, but from Bjorn's comment it sounds like rustc will need to be updated in order to start looking for the mangled symbols rather than unmangled. In which case, wouldn't unconditional mangling mean that when the standard library+builtins is built with the beta compiler it will emit calls to unmangled symbols, but we'll only be providing the mangled version?

We could also provide a trampoline from unmangled to mangled symbols and provide both with a FIXME to remove after the next bootstrap bump, so we don't need to worry about cfg(bootstrap) complications.

[folkertdev]

Hmm, maybe? We might also be able to "fix" the mangling on the rustc side with cfg(bootstrap) too?

Did you ever get using a git dependency for compiler-builtins in rustc to work? (I remember I tried but at least locally I never got it to work)

[bjorn3]

We might also be able to "fix" the mangling on the rustc side with cfg(bootstrap) too?

That would need to be done on the compiler-builtins side, but then the next bootstrap bump would break until a new compiler-builtins version with the cfg(bootstrap) removed lands.

[folkertdev]

Right, that's the issue we've been having. cfg(bootstrap) at least makes it possible to make changes, but it's a very tedious process. Seems like there is no real way around it though?

Bjorn mentioned earlier that using #[no_mangle] is invalid too, meaning that using cfg(bootstrap) is kind of the only way to land this change (until compiler-builtins becomes a submodule)

[tgross35]

Providing both versions is probably the easiest way forward then, unless you're able to test a better way.

Did you ever get using a git dependency for compiler-builtins in rustc to work? (I remember I tried but at least locally I never got it to work)

I do it occasionally but consistently forget exactly how. To the best of my knowledge, the below is about what is needed:

• In your git fork, make sure to change the compiler-builtins/Cargo.toml version to 0.1.999 or something, so Cargo will actually update it. I never remember this.
• Unpin the version in alloc and std's Cargo.toml
• Add [patch.crates-io] to library/Cargo.toml pointing to the git + rev
• cargo update in library/

[tgross35]

Bjorn mentioned earlier that using #[no_mangle] is invalid too, meaning that using cfg(bootstrap) is kind of the only way to land this change (until compiler-builtins becomes a submodule)

Forgot about this, #897 (comment) for my future reference. Let me just see if I can get this into a subtree within the next couple of days, which will at least avoid some of the possible back and forth.