Skip to content

Tracking Issue for breakpoint feature (core::arch::breakpoint) #133724

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

Open
2 of 4 tasks
joshtriplett opened this issue Dec 2, 2024 · 33 comments
Open
2 of 4 tasks

Tracking Issue for breakpoint feature (core::arch::breakpoint) #133724

joshtriplett opened this issue Dec 2, 2024 · 33 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-lang-radar Items that are on lang's radar and will need eventual work or consideration. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. T-lang Relevant to the language 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

@joshtriplett
Copy link
Member

joshtriplett commented Dec 2, 2024

Feature gate: #![feature(breakpoint)]

This is a tracking issue for the breakpoint feature, which gates the core::arch::breakpoint function. This feature was approved in ACP 491.

Public API

// core::arch

/// Compiles to a target-specific software breakpoint instruction or equivalent.
///
/// (Long description elided.)
#[inline(always)]
pub fn breakpoint() {
    unsafe {
        core::intrinsics::breakpoint();
    }
}

Steps / History

Unresolved Questions

  • None yet.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@joshtriplett joshtriplett added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 2, 2024
@clarfonthey
Copy link
Contributor

Is there a reason why this is in core::arch instead of core::hint? I feel like it would be logical to say that a perfectly acceptable implementation of this would be to do nothing, and I'd imagine this is what a lot of embedded targets will do.

Plus, core::arch is generally reserved for arch-specific stuff, and this is specifically independent of architecture.

@Scripter17
Copy link
Contributor

Does it really fit in std::hint? Everything else in there is, well, hints to the compiler. Either for lints or for optimization

It fits better in there but it doesn't feel like the obvious place to look for a breakpoint function

@joshtriplett
Copy link
Member Author

@clarfonthey I don't think "do nothing" should be a valid implementation of this. "abort" is a valid implementation. Given that, I don't think this belongs in hint.

That said, that doesn't necessarily make core::arch the best place. It seemed like a good fit at the time, in part because its behavior is arch-specific.

I don't see any existing module that this logically fits in other than arch.

We could put it in a new core::debug or similar, but I'm not sure it's worth a new module for this, and debug could be confused as being related to Debug.

@clarfonthey
Copy link
Contributor

I mean, sure, but we have other things in hint which emit special instructions, like spin_loop. Just because the definition of a "trivial" implementation changes, or that the result is architecture-specific, doesn't mean it doesn't fit into that module.

@joshtriplett
Copy link
Member Author

@clarfonthey AFAICT, every function in core::hint can be replaced by a no-op/identity for a correct (if suboptimal) implementation. I think that's a property worth not losing.

A breakpoint isn't a "hint"; it can't be replaced with a no-op.

Is there any other module in core that you think this could fit into? Or, do you think it's worth creating a new module for this and potential related items?

@kpreid
Copy link
Contributor

kpreid commented Dec 16, 2024

JavaScript has the debugger statement which causes the debugger to pause the program if a debugger is available (in current browsers, this means “if the developer tools are open”), and otherwise continues execution. This is a useful behavior (it allows executing an identical program with and without pauses for debugging, without needing to remove the breakpoint and recompile) and if Rust offered it, it would fit in core::hint.

@joshtriplett
Copy link
Member Author

@kpreid I'd love to have that operation, but unfortunately, that's a much more complex operation that isn't as simple as emitting an instruction, it'd be more error-prone (it can erroneously detect a debugger), it'd be less portable (as it's OS-specific rather than CPU-specific), and it wouldn't be available on all targets.

@BrainBacon
Copy link

@joshtriplett Maybe an ACP for debugger presence detection? C++ 26 is getting that.

I recently added debugger presence detection to Unbug using the dbg_breakpoint crate. The bulk of that crate was previously accepted as a panic hook in the Rust standard library, but then reverted later.

@joshtriplett
Copy link
Member Author

joshtriplett commented May 20, 2025

Stabilization Report

Implementation History

Approved in ACP rust-lang/libs-team#491 , implemented in #133726 , and not modified since. Includes tests and documentation.

Some discussion about what the correct module for this function should be, but no conclusion about a specific better place than core::arch.

API Summary

In core::arch (and thus also std::arch):

pub fn breakpoint();

Experience Report

The ACP was originally inspired by the unbug crate, which was promptly able to switch to this (and will be able to drop some architecture-specific code when this becomes stable, and run on stable Rust on more architectures). https://github.com/microsoft/edit is also using this.

@joshtriplett joshtriplett added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 20, 2025
@Amanieu
Copy link
Member

Amanieu commented May 20, 2025

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 20, 2025

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 20, 2025
@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 20, 2025
@traviscross
Copy link
Contributor

traviscross commented May 20, 2025

@rustbot labels +I-lang-nominated +P-lang-drag-1

Generally we lang FCP the first stable use of an intrinsic. This is an intrinsic, and this is the first stable use. At the same time, as we discussed in the libs-api meeting today, perhaps what we're meaning to lang FCP are new capabilities of the language, and this one could be seen as equivalent to some inline assembly. So I don't know. It's worth us having a look in any case, so let's nominate.

cc @RalfJung @workingjubilee @rust-lang/lang

@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang labels May 20, 2025
@RalfJung
Copy link
Member

This doesn't really have semantics so I don't think there's much to say here from the opsem side.

@Amanieu
Copy link
Member

Amanieu commented May 20, 2025

The semantics are essentially:

if debugger is attached and chooses to skip over the breakpoint {
    continue execution
} else {
    trap
}

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented May 20, 2025

The "trap" part seems like a somewhat "new capability" for no_std programs. There's lots of ways to diverge, of course, but all the ones I can think of right now are either std-exclusive (std::process::abort), platform-specific (e.g., inline assembly or functions like core::arch::wasm32::unreachable), diverge by entering an infinite loop, or end up in the #[panic_handler] which has to use one of the aforementioned options to diverge. The kind of "portable trap" that breakpoint does is more like core::intrinsics::abort, which is unstable and has no stable equivalent as far as I know.

@clarfonthey
Copy link
Contributor

clarfonthey commented May 20, 2025

I'm still not 100% convinced on this being in core::arch, since again, it's not an architecture-specific extension. (Inline assembly is technically callable by all architectures, but still inherently architecture-specific.)

That said, the closest analogue that seems to exist is std::process, and I'm not sure whether we'd want this to be core::process::breakpoint. It would go along with std::process::abort and std::process::exit.

Perhaps std::process::breakpoint could be the advanced version of breakpoint checking that truly does nothing when a debugger is not attached, and core::process::trap could be this version which is explicitly simpler and has a more descriptive name.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented May 20, 2025

Also, regarding these arguments for core::arch over core::hint from last December:

AFAICT, every function in core::hint can be replaced by a no-op/identity for a correct (if suboptimal) implementation. I think that's a property worth not losing.

A breakpoint isn't a "hint"; it can't be replaced with a no-op.

Now that core::hint::select_unpredictable has been stabilized in beta, it's no longer true that every "hint" function is just a no-op or identity (though the correct-but-suboptimal implementation of select_unpredictable is pretty trivial). The quality of implementation lever to tune here is "whether a debugger can detect it and resume execution" and so the "suboptimal but valid" implementation would be unconditionally aborting. Indeed that's the implementation you get on targets where there is no standard mechanism that debuggers recognize as breakpoint distinct from any other program abort (e.g., I think this is the case for wasm).

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 20, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented May 20, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@clarfonthey
Copy link
Contributor

Actually, that being the case now means that it probably is best to put this in core::hint for those reasons.

@joshtriplett
Copy link
Member Author

I'm still not 100% convinced on this being in core::arch, since again, it's not an architecture-specific extension. (Inline assembly is technically callable by all architectures, but still inherently architecture-specific.)

That said, the closest analogue that seems to exist is std::process, and I'm not sure whether we'd want this to be core::process::breakpoint. It would go along with std::process::abort and std::process::exit.

We did discuss core::process in the meeting, and we weren't sure either. It would be the only thing in core::process if we did that.

Perhaps std::process::breakpoint could be the advanced version of breakpoint checking that truly does nothing when a debugger is not attached, and core::process::trap could be this version which is explicitly simpler and has a more descriptive name.

trap is a different operation; this would be debugtrap. But breakpoint seems more descriptive.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented May 20, 2025

FWIW, I think debugtrap communicates the "traps when there's no debugger" aspect better. What's called a breakpoint in interactive debuggers never aborts the program, only pauses it. So I was initially surprised to read that the intrinsic proposed here will "by default" abort the program. (Even if it's for good reasons!)

@clarfonthey
Copy link
Contributor

As I mentioned, I also think that there would be room to add std::process::breakpoint in the future that does the more advanced version that actually does nothing when a debugger isn't present, and even if this is a different module, it would make sense for the two to have different names.

@joshtriplett
Copy link
Member Author

@hanna-kruppe I disagree; it'd be just as easy to interpret "debugtrap" as "trap only when debugging". "breakpoint" emits a target-specific breakpoint instruction, and the normal behavior of such an instruction is to trap.

@scottmcm
Copy link
Member

The semantics are essentially:

if debugger is attached and chooses to skip over the breakpoint {
    continue execution
} else {
    trap
}

I think that's a lang question -- is whether a debugger is attached observable behaviour? Is it part of the Rust AM state? Are we allowed to move this around relative to other non-observable statements? It reminds me of the "non-temporal store" issue, making me think

emits a target-specific breakpoint instruction

isn't enough to describe what its Rust semantics are, just like "well it does a target-specific non-temporal store" wasn't enough.

TBH, I would also have expected hint::breakpoint() here, so that the lang questions are answered with "well it's a QoI issue whether it does anything at all, as it is spec'd to do nothing".

FWIW, fatally trapping being the default behaviour is surprising to me -- I'd pattern-matched it to https://learn.microsoft.com/en-us/dotnet/api/System.Diagnostics.Debugger.Break?view=netframework-4.8.1 which is different from https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debugger.launch?view=netframework-4.8.1. I'd have thought it does nothing if there's no debugger attached.

@joshtriplett
Copy link
Member Author

joshtriplett commented May 21, 2025

TBH, I would also have expected hint::breakpoint() here, so that the lang questions are answered with "well it's a QoI issue whether it does anything at all, as it is spec'd to do nothing".

"do nothing" isn't intended to be a valid implementation of breakpoint; the default implementation should be to abort. Being able to continue is the special-case scenario that you can possibly do with a debugger attached, not the default scenario.

At least on Linux, behaving differently if a debugger is attached is a more challenging and finicky proposition, and I wouldn't expect any debugger-dependent behavior to be something we'd be able to reliably support across a majority of targets.

@hanna-kruppe
Copy link
Contributor

Does that make any difference from a language/spec perspective? Are users expected to rely on this “by default” aborting the program, and if so, how? Basically: what makes a “do nothing” implementation of breakpoint() less valid than e.g., hint::black_box being an ordinary identity function or hint::select_unpredictable always compiling down to branches?

@traviscross
Copy link
Contributor

traviscross commented May 21, 2025

@rustbot labels +T-lang -I-lang-nominated +I-lang-radar -P-lang-drag-1

We discussed this in the lang call today. Given what @scottmcm said above, the PR to stabilize the use of this intrinsic is going to need a lang FCP. That is, it's OK for this libs-api FCP on the issue to complete as-is (or not), but please nominate the stabilization PR for lang, and please include a lang stabilization report in that PR that addresses the normal items and those raised by @scottmcm.

@rustbot rustbot added I-lang-radar Items that are on lang's radar and will need eventual work or consideration. T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang labels May 21, 2025
@Jules-Bertholet
Copy link
Contributor

We did discuss core::process in the meeting, and we weren't sure either. It would be the only thing in core::process if we did that.

No, it should not be the only thing; it should be joined by the stable version of core::intrinsics::abort(). These different methods of aborting the process belong together. core::arch is certainly not the right place for something not architecture-specific.

@traviscross
Copy link
Contributor

The argument that was made in the libs-api meeting against core::process was that processes are inherently an OS concept and so a module named "process" doesn't make sense in core.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented May 22, 2025

Yes, that is a good argument for a different name (execution?). But either way, breakpoint() should be grouped together with immediate_abort().

(A third option is to put it directly in the root of core.)

@Jules-Bertholet
Copy link
Contributor

One argument in favor of core::process is that it would be a good place for an an abort() function that is equivalent to std::process::abort() where available, and intrinsics::abort() otherwise.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented May 22, 2025

A fourth function that might be useful in this hypothetical module, is fn stop(), that does loop { std::thread::park() } where available, and loop { core::hint::spin_loop() } otherwise.

@joshtriplett joshtriplett added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 22, 2025
@joshtriplett
Copy link
Member Author

Renominating so a subsequent meeting can do further bikeshedding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-lang-radar Items that are on lang's radar and will need eventual work or consideration. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. T-lang Relevant to the language 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