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

Add a stable flag to enable/disable codegen UB checks #625

Closed
1 of 3 tasks
saethlin opened this issue May 1, 2023 · 8 comments
Closed
1 of 3 tasks

Add a stable flag to enable/disable codegen UB checks #625

saethlin opened this issue May 1, 2023 · 8 comments
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@saethlin
Copy link
Member

saethlin commented May 1, 2023

Proposal

We currently have one (#570), and soon possibly another (#624) MIR pass that inserts runtime checks for UB. The only way to disable these in a stable compiler is to disable debug assertions. Users who (for whatever reason, perhaps because the passes are buggy or don't support their uncommon build context: rust-lang/rust#109996) cannot disable the passes without blanket turning off debug assertions.

In addition to the above motivation, I now want this flag so that we have a way to toggle the checks if they are moved out of a MIR pass and into a codegen backend: rust-lang/rust#117473. Toggling the MIR pass was a convenient pre-existing switch that let users toggle these checks (but only on nightly!); if they are moved to codegen we need to come up with a new flag.

I am proposing to add a -C flag that can toggle these checks together, but not change whether or not debug assertions are enabled. Users are likely to wish to toggle these passes together, because if they cause a problem it will likely be because they insert unexpected panics. If unexpected panics are an issue, then leaving one form enabled but not the other is a time bomb.

The current name of this flag is -Cextra-ub-checks and it currently only supports boolean values (the yes/no/true/false family).

Implementation: rust-lang/rust#111166

Mentors or Reviewers

I do not have any in mind

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@saethlin saethlin added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels May 1, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 1, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label May 1, 2023
@tmandry
Copy link
Member

tmandry commented May 1, 2023

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label May 1, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 25, 2023
@apiraino
Copy link
Contributor

apiraino commented Aug 10, 2023

@rustbot concern needs-fcp

Registering the concern discussed on Zulip about needing a full-fledged FCP for this change

@saethlin saethlin changed the title Add a blanket flag to enable/disable codegen UB checks Add a stable flag to enable/disable codegen UB checks Oct 19, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jan 11, 2024

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 11, 2024

Team member @oli-obk 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.

@petrochenkov
Copy link

@rfcbot reviewed
With a condition that it's introduced as an unstable -Z option first, rather than a stable -C option.

@saethlin
Copy link
Member Author

I don't know how to drive this forward. All of the discussion is about what exactly to call the flag and I really don't care what it's called. So I'm not able to do anything but echo arguments made by other people, and the arguments made by others in Zulip have not moved anywhere.

If this feature is really that important, someone else can do this.

@saethlin saethlin closed this as not planned Won't fix, can't repro, duplicate, stale Feb 17, 2024
@apiraino apiraino removed the final-comment-period The FCP has started, most (if not all) team members are in agreement label Feb 21, 2024
@RalfJung
Copy link
Member

I'm trying to revive this: #725.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

8 participants