type-check lang items

[thestinger]

These aren't currently well-defined by the language. If you define anything with a somewhat matching LLVM signature, it will compile. Not only does it not care about the type signatures matching what it expects, there is no handling for these being defined on the wrong types of AST nodes.

[thestinger]

I don't think this is milestone-worthy, because these are already implementation details.

[emberian]

This leads to craziness like:

#[no_std];

#[main]
#[lang="start"]
fn start() { }

[2:21:07]/tmp> rustc foo.rs
task 'rustc' has overflowed its stack
zsh: illegal hardware instruction (core dumped) rustc foo.rs

or, more benignly:

#[no_std];

#[lang="start"]
fn start() { }

#[main]
fn main() { }

which just plain asserts:

rustc: /build/rust-git/src/rust/src/llvm/lib/IR/Instructions.cpp:276: void llvm::CallInst::init(llvm::Value*, llvm::ArrayRef<llvm::Value*>, const llvm::Twine&): Assertion `(Args.size() == FTy->getNumParams() || (FTy->isVarArg() && Args.size() > FTy->getNumParams())) && "Calling a function with bad signature!"' failed.

or, more crazily:

#[no_std];

#[lang="start"]
mod a { }

#[main]
fn main() { }

which "just" ICEs:

error: internal compiler error: node_id_to_type: no type for node `mod a (id=2)`
This message reflects a bug in the Rust compiler. 
We would appreciate a bug report: https://github.com/mozilla/rust/wiki/HOWTO-submit-a-Rust-bug-report
task 'rustc' failed at 'explicit failure', /build/rust-git/src/rust/src/libsyntax/diagnostic.rs:102
task '<main>' failed at 'explicit failure', /build/rust-git/src/rust/src/librustc/lib.rs:395

Language items are the interface between the compiler and the language. They need to be well-defined and verified for correctness, IMO.

[emberian]

(Nominating)

[pnkfelix]

This should not block 1.0. Assigning P-low.

[nagisa]

Still asserts with

#![no_std]
#![feature(no_std,core,lang_items)]

extern crate core;

#[lang="start"]
fn main() {
}

#[lang = "stack_exhausted"] extern fn stack_exhausted() {}
#[lang = "eh_personality"] extern fn eh_personality() {}
#[lang = "panic_fmt"] fn panic_fmt() -> ! { loop{} }

[steveklabnik]

Triage: no changes

[steveklabnik]

Triage; not aware of any changes here

[Mark-Simulacrum]

I'm closing this issue -- I don't think type checking language items is all that helpful. Anyone using the lang items feature is likely / should be aware of what they're doing and adding type checks in the compiler would likely be complicated and somewhat painful, and essentially just "wasted" work from a compiler performance perspective.

[WildCryptoFox]

@Mark-Simulacrum I strongly disagree that this is waste. The compiler knows the type it is expecting, so it should meaningfully tell the user what the type it expects is, instead of accepting anything and failing if you're lucky. Worse, what happens when a type changes? However rare, I'd rather have the types than an ICE or other obscure outputs.

[Mark-Simulacrum]

The point here is that unlike almost everything else in Rust, lang items are always pretty visible and when changing their signature it's almost always done inside libstd/libcore and as such the time investment into checking them (compile time and human time) seems to make this not really worth tracking. If a minimal PR was filed and/or we gradually started doing this, that's reasonable, but IMO we gain almost nothing from having an issue.

[WildCryptoFox]

@Mark-Simulacrum But it is an issue and is far from closed. You may not see the effort in implementing it but if it remains open, someone else may do so.

[Mark-Simulacrum]

The reason I don't see tracking it as useful is because it's essentially an internal implementation detail -- similar to having a dedicated issue for random FIXMEs in the code. In this case, it's not even clear that we want to do this -- at least to me. I'll reopen, I guess, because I don't feel strongly enough to argue about it more, but I don't think this is useful.

[WildCryptoFox]

Thank you @Mark-Simulacrum.

[asquared31415]

I've looked at this a little and I think that the existing detection that #85339 added to traits could be expanded to include most or all of the lang items. This wouldn't be a full solution but it would help to stop some of the ICEs that occur on functions and structs that are lang items.

[pnkfelix]

The point here is that unlike almost everything else in Rust, lang items are always pretty visible and when changing their signature it's almost always done inside libstd/libcore and as such the time investment into checking them (compile time and human time) seems to make this not really worth tracking. If a minimal PR was filed and/or we gradually started doing this, that's reasonable, but IMO we gain almost nothing from having an issue.

I want to push back on one part of the above message.

I understand where @Mark-Simulacrum is coming from: Why do this work, when the thing we're checking is almost always part of libcore/libstd, and thus hopefully receiving significant review for correctness, especially when it comes to something like well-formedness.

But: What about the developers who are making alternative libstds? That's a scenario where it seems like these checks would bring joy, not waste time. (Right?)

In any case, even if it turns out that some of the introduced checks have non-trivial cost, I think we could still justify adding such checks if we can either 1. detect when we're compiling an "unblessed" libcore/libstd, or 2. put them under a -Z or -C flag. That could address the important use case while mitigating the danger of making all the other compiler invocations slower.

[Mark-Simulacrum]

FWIW, I think my perspective has changed a little since 2019, in part informed by the increased presence of alternative (or at least small change fork implementations of std + alloc (and in some cases core). So I think the checks are mostly valuable these days, particularly for any new lang items.

I do think we should be cautious about making sure added checks are complete (i.e., actually verify correctness). While it is "just a bug" if we get it wrong, it is much easier in practice for folks to assume we're checking everything if we check partially, I think, especially in this area.