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

CFI: Refactor: Split into modules #123521

Closed
wants to merge 6 commits into from
Closed

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Apr 5, 2024

We have almost all the implementation in a single ~1200 line file, and have had a lot of churn that leaves debris around.
I believe this to be no-functional-change, just code organization and cleanup.

This change:

  • Splits it out into three modules, instance, which has our instance generalization code, ty, which has the transform_ty logic, and itanium_cxx_abi, which has the type encoding.
  • Merges the three option types into one - they were type aliases, not newtypes, so you could already freely cast between them, and we were doing error handling for if one of them didn't accept the bit patterns of another.
  • Removes inclusion of module names in function/type names, avoiding redundancy when using the API through use rustc_symbol_mangling::typeid;
  • Minor clippy fixes
  • Adds some summary documentation about the behavior of instance::transform and ty::transform

@rustbot
Copy link
Collaborator

rustbot commented Apr 5, 2024

r? @lcnr

rustbot has assigned @lcnr.
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

@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 5, 2024

Some changes occurred in compiler/rustc_symbol_mangling/src/typeid

cc @rust-lang/project-exploit-mitigations, @rcvalle

@rcvalle
Copy link
Member

rcvalle commented Apr 5, 2024

I'll take a look at it, but it might have to wait a little bit because we're in the middle of moving all sanitizers related code into a rustc_sanitizers crate and stabilizing the core sanitizers, and we'll have to see how it fits in the planned design (@davidtwco and I).

@bors
Copy link
Contributor

bors commented Apr 6, 2024

☔ The latest upstream changes (presumably #123517) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance, these changes all seem reasonable to me, but will conflict with #123620 so you might want to wait until that lands?

@rcvalle
Copy link
Member

rcvalle commented Apr 8, 2024

Thanks a lot for your time and work on this, @maurer! Much appreciated. I took a look, and after #123620 is merged, I think we what we want from here is:

  1. Consolidating EncodeTyOptions, TransformTyOptions as TypeIdOptions.
  2. Fixing Clippy findings.
  3. Maybe the abstraction for TransformTy?

The abstraction for TransformTy would cause TransformTy to be allocated and deallocated (with a non-trivial drop) for every parameter of every function or method so I'm unsure if we want that. Using TransformTy directly doesn't seem too bad when weighted against that. What do you think?

Would you mind rebasing these on top of #123620 whenever you have time?

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2024
@Dylan-DPC
Copy link
Member

@maurer any updates on this? thanks

@Dylan-DPC
Copy link
Member

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

@Dylan-DPC Dylan-DPC closed this Aug 14, 2024
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PG-exploit-mitigations Project group: Exploit mitigations S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants