Skip to content

add rustc-demangle assertion on mangled symbol #85534

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 29, 2021
Merged

add rustc-demangle assertion on mangled symbol #85534

merged 1 commit into from
Aug 29, 2021

Conversation

csmoe
Copy link
Contributor

@csmoe csmoe commented May 21, 2021

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2021
@michaelwoerister
Copy link
Member

Thanks, @csmoe! Let's wait and see what the crater run at #85530 yields.

@michaelwoerister
Copy link
Member

I'm also wondering if this should be a debug_assert! instead. @Mark-Simulacrum, do you have any idea how much coverage we would get if this is only a debug assertion? Does anybody use a compiler with debug assertions enabled?

@Mark-Simulacrum
Copy link
Member

Essentially only rustc developers, I'm not sure we even ship builds with debug asserts on. Do we know what performance impact we're looking at?

@csmoe
Copy link
Contributor Author

csmoe commented May 22, 2021

@michaelwoerister Or a crater_run_assert which checks if we are doing a crater_run through an env variable.

@Mark-Simulacrum
Copy link
Member

Please don't try to detect crater inside rustc; if you want to test something on crater but not land it, a try build should be sufficient with it just always on, and we can run that through crater.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2021
@michaelwoerister michaelwoerister added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2021
@michaelwoerister
Copy link
Member

I think this is blocked on value mangling support: #87194

Once we have that, we should at least make it a debug assertion. That way we cover everything that the compiler itself uses.

@michaelwoerister
Copy link
Member

@csmoe, now that #87194 is merged, would you mind turning the check into a debug assertion?

@csmoe
Copy link
Contributor Author

csmoe commented Aug 27, 2021

@michaelwoerister okay :)

michaelwoerister
michaelwoerister approved these changes Aug 27, 2021
View reviewed changes
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

Thank you, @csmoe!

@michaelwoerister
Copy link
Member

@bors r+

@michaelwoerister
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 27, 2021

📌 Commit 5eb960c has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Aug 27, 2021
@michaelwoerister
Copy link
Member

bors doesn't like PRs that have been open for a while :)

@jackh726
Copy link
Member

@bors rollup

@GuillaumeGomez GuillaumeGomez mentioned this pull request Aug 29, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 29, 2021
@bors
Auto merge of rust-lang#88461 - GuillaumeGomez:rollup-khgu0eb, r=Guil…
daa4dc9 
…laumeGomez

Rollup of 13 pull requests

Successful merges:

 - rust-lang#80543 (Notify when an `I-prioritize` issue is closed or reopened)
 - rust-lang#83251 (Suggestion for call on immutable binding of mutable type)
 - rust-lang#85534 (add rustc-demangle assertion on mangled symbol)
 - rust-lang#88173 (Refactor Markdown length-limited summary implementation)
 - rust-lang#88349 (Add const and static TAIT tests)
 - rust-lang#88357 (add unsized coercion test)
 - rust-lang#88381 (Handle stack_t.ss_sp type change for DragonFlyBSD)
 - rust-lang#88387 (Remove vestigial rustfix tests.)
 - rust-lang#88396 (Bump vulnerable crates)
 - rust-lang#88407 (Fix formatting in release notes from 52a9883)
 - rust-lang#88411 (Remove `Session.if_let_suggestions`)
 - rust-lang#88417 (RELEASES.md: fix broken link)
 - rust-lang#88419 (Fix code blocks color in Ayu theme)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 63cfbf5 into rust-lang:master Aug 29, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@michaelwoerister michaelwoerister michaelwoerister approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.
9 participants
@csmoe @michaelwoerister @Mark-Simulacrum @bors @jackh726 @crlf0710 @JohnCSimon @rust-highfive @rustbot