Stabilize the breakpoint function by joshtriplett · Pull Request #142325 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation27 Commits1 Checks9 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Stabilization report and FCP in
#133724.
r? @jhpratt
rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.
labels
Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.
cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr
Relevant to the library API team, which will review and decide on the PR/issue.
and removed T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.
labels
@joshtriplett Was this discussed by T-lang as suggested here? I see that the issue was nominated until a few hours before this PR was opened, so it seems likely that that is the case.
r=me if that is the case.
From the docs:
this may compile to a trapping instruction (e.g. an undefined instruction) instead
I don't know what an "undefined instruction" is, but I hope it has nothing to do with undefined behavior?
I would assume that "illegal instruction" is the intended wording here, since such instructions always trigger interrupts on hardware regardless of whether an OS is present. Although, I think that "trapping instruction" by itself is clearer wording and adding the "clarification" here only makes it more confusing. The fact that I'm probably wrong in this explanation helps emphasise how confusing it is: to me, a trapping instruction and an undefined instruction are two separate things.
@joshtriplett Was this discussed by T-lang as suggested here?
No, it hasn't yet. I've nominated it for discussion in a meeting.
From the docs:
this may compile to a trapping instruction (e.g. an undefined instruction) instead
I don't know what an "undefined instruction" is, but I hope it has nothing to do with undefined behavior?
"trapping instruction" refers to an instruction that produces a trap of some kind, and "undefined instruction" in this context refers to instruction opcodes explicitly reserved for this purpose. See https://www.felixcloutier.com/x86/ud for example. This is entirely unrelated to undefined behavior.
Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang
Relevant to the language team
and removed T-libs-api
Relevant to the library API team, which will review and decide on the PR/issue.
labels
It's correct, I think, for us to review this, as with exposing other intrinsics, to be sure we're happy with any effects on our language definition. As it is, I think this is OK, so let's...
@rfcbot fcp merge
@rfcbot reviewed
To the point of semantics, @RalfJung, it seems to me that the API docs are "sufficiently clear" for the spec team to do its work. Based on what it says, it seems clear that breakpoint() without a debugger attached cannot cause UB since it is guaranteed not to continue execution. I presume in practice it will signal or trap on an illegal instruction. (Though I gather @joshtriplett has a plan to make them more precise, which is good.)
The exact semantics (abort if no debugger) are somewhat surprising to me. But I understand that this is the most straightforward low level operation that we can stabilize, because there is no portable way to implement "break but only if there is a debugger attached".
@rfcbot reviewed
In the final comment period and will be merged soon unless new substantive objections are raised.
label
🔔 This is now entering its final comment period, as per the review above. 🔔
I suppose in a sense, then, the semantics here is similar to
if platform_specific_volatile_read() { abort() }
and it needs to be treated as similarly immovable as volatile operations?
Given discussions on the tracking issue, I’m not actually sure if everyone’s on the same page about what the language spec would be if it said something about this intrinsic. I imagine something like (in spirit if not spec-verbiage)
Any time
breakpointis called, it non-deterministically chooses between (1) terminating the program or (2) having no observable effect.
But that would technically imply “always do nothing” is a valid implementation and many months ago @joshtriplett wrote:
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.
Also, my off the cuff proposal appears subtly different from what @scottmcm just posted while I was writing this.
I think it would be useful for there to be a warning when this is called.
@hanna-kruppe OK, fair enough. We discussed this some more in the meeting. I am aligned with your definition and I think it means that, indeed, a no-op would be a valid implementation, though not a very useful one.
(In reality, the semantics are platform dependent but must be compatible with that definition.)
Well, actually, the more I think on this, the less sure I am. Here's the thing. I can imagine architectures where it is possible for the breakpoint instruction to be intercepted. I think it is even possible, it's a signal, you can do signal handlers (how do debuggers work, after all). Which suggests that there would be programs that may indeed leverage this in twisted ways and therefore not ABORT nor be a no-op and yet not involve the user of a debugger.
It seems like the "specification" just wants to be entirely architecture dependent. Whatever spec we write would say "the defined behavior of this fn is entirely dependent on the target architecture" with a non-normative note that "it may reasonable be modeled as a conditional abort for the purposes of proving properties on programs".
Further conversation in the call had me thinking about what it means to use this. Because it returns () I think the code that calls it has to treat it like it might be a NOP no matter what the specification says. Caller code that's using it in some kind of non-unit _ => breakpoint() will have to add a panic!() or Default::default() or similar to satisfy the type checker.
So why isn't ok to just say "well this might be a NOP on some targets", since the caller has to handle that anyway?
It really sounds to me like the Specification for the intrinsic is essentially "might return, might never return" and it's a target Quality-of-Implementation issue how exactly that choice is made and how any non-return happens.
As potentially-elucidating discussions:
- would an implementation which just
loop {}s be a legal implementation? - would an implementation that uploads a dump to WER then continues executing be a legal implementation?
From a different perspective, I get the impression that people would be annoyed -- understandably so, given the intent expressed by libs-api for the function -- if a MIR optimization removed all the code after a call to this. But if the specification doesn't allow for it to return to the caller, then removing that code would be an entirely legal optimization. So if the intent is that that's not a legal optimization, then the spec must be that it might return, implying that it might have "done" nothing since there doesn't seem to be any other Observable Behaviour possible from it.
@rfcbot concern settle-on-specification
We should settle on the language definition for this before stabilizing. I debated whether or not to file the concern, but it just seems better to settle this first so we don't confuse the people who are trying to write down the meaning of our language.
In my view, the language definition for this intrinsic is that it nondeterministically aborts or falls through, and that valid implementation choices include (but are not limited to) emitting a no-op, emitting an unconditional abort, and emitting if rand() % 2 == 0 { abort() }.
Since we weren't able to settle on that unanimously, though, let's continue discussing, as above, and leave this nominated.
In particular, if a debugger is attached and one resumes execution of the program after the breakpoint, that basically makes it a NOP from the perspective of the program. (If it's not a NOP, then what did it do? The AM state is entirely unchanged. And it's not like we make any guarantees about what you can observe with a debugger anyway.) So while @joshtriplett has argued before that a NOP would not be a valid implementation, I think that's not semantically coherent.
The exact semantics (abort if no debugger) are somewhat surprising to me. But I understand that this is the most straightforward low level operation that we can stabilize, because there is no portable way to implement "break but only if there is a debugger attached".
This is more a "fun fact" than anything necessarily decisive regarding the decision here, but a while ago we actually tried to implement this specific thing in the Rust standard library for panics. The idea was that, optimistically, your debugger would stop every time it hit a panic, without having to train debuggers to recognize "oh, that's a Rust exception being thrown".
It had the embarrassing consequence that programs would abort if you used strace on them, which is a diagnostic utility that usually people think of as a debugging aid, but not a debugger. But in the eyes of the kernel, strace and gdb are doing the same thing, so when we detected "debugger attached", we detected that we were under strace. Then we tried to fling ourselves onto a ud2... the sort of thing gdb would catch us from... but our trust-fall into the waiting arms of strace was answered by it continuing to simply faithfully report to the programmer that our Rust process had decided to give up on the world.
Obviously, we reverted that, as it was well-intentioned but a bit silly.
@rustbot labels +I-libs-api-nominated
Since the libs-api call will precede the lang call next week, let's nominate this for @rust-lang/libs-api to review the feedback by lang members on how this intrinsic might be defined as a language matter in the event that affects at all (and it may not) anything on the library side.
So if I'm understanding correctly, one of the lang questions is on how this should affect optimizations (or not affect them as the case may be)? You want it to be specified as either always aborting (i.e. returning !) or maybe returning so the compiler can optimise accordingly. This is independent of any requirements libs-api may have for acceptable implementations (e.g. they may reject a PR that implements breakpoint as a literal no-op).
The other lang question is on SIGTRAP handling. I guess someone can handle SIGTRAP in twisted ways but I'm unsure how that is meaningfully different from having fun with other signals?
Let's consolidate discussion into the tracking issue for now: #133724
This comment was marked as duplicate.
Lang team prioritization drag level 3.https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang.
and removed P-lang-drag-1
Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang
Nominated for discussion during a libs-api team meeting.
labels
@rustbot labels -I-libs-api-nominated
This was discussed on the libs-api call today without any specific resolution. The nomination was to ensure awareness of the ongoing discussion, and there's that awareness now, so we can unnominate.
DEPRECATED: Use the team-based variants `S-waiting-on-t-lang`, `S-waiting-on-t-compiler`, ...
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Lang team prioritization drag level 4. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang
and removed P-lang-drag-3
Lang team prioritization drag level 3.https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang.
labels