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.

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

Tests crash from inappropriate use of common linkage #113807

Merged
merged 1 commit into from
Sep 9, 2023
Merged

Tests crash from inappropriate use of common linkage #113807

merged 1 commit into from
Sep 9, 2023

Conversation

mojingran
Copy link
Contributor

@mojingran mojingran commented Jul 17, 2023 •
edited by wesleywiser
Loading

Follows up my proposal under the tracking issue for the linkage feature. Adds test for issue 109681.

@rustbot
Copy link
Collaborator

rustbot commented Jul 17, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added 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 Jul 17, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mojingran mojingran changed the title Disables unsupported linkage types in Rust. Fixes issue #109681. Disables unsupported linkage types in Rust. Jul 20, 2023
@mojingran mojingran mentioned this pull request Jul 20, 2023
Open
@cjgillot
Copy link
Contributor

I won't have time to review this before a week.
r? compiler

@rustbot rustbot assigned WaffleLapkin and unassigned cjgillot Jul 28, 2023
@WaffleLapkin
Copy link
Member

I don't really understand the implications here
r? compiler

@rustbot rustbot assigned wesleywiser and unassigned WaffleLapkin Jul 29, 2023
@bjorn3
Copy link
Member

bjorn3 commented Jul 29, 2023

The external linkage you already get for extern { ... } (as declaration) and #[no_mangle] (as definition) Setting it using #[linkage] will only break things I think. The various weak variations feel like they should get dedicated attributes which do the necessary validation. I think #[linkage] should be kept as is with all options allowed as internal attribute if we need the alternative linkage methods inside the standard library or for external crates on nightly.

@mojingran
Copy link
Contributor Author

Sure. Do you have a suggestion for what the output should be for this issue? Maybe we can mimic what clang does, or report the error in rustc? @bjorn3

@bjorn3
Copy link
Member

bjorn3 commented Jul 31, 2023

Common linkage with non-zero initializer is just invalid. For an internal feature there is no need to have diagnostics. There are many ways to crash rustc using internal features.

@wesleywiser
Copy link
Member

I think I agree with @bjorn3, since the feature is unstable (and marked perma-unstable at this time by the lang team) we should just offer the full range of LLVM options to the user.

Perhaps the best thing to do right now to close #109861 would be to just add a test that verifies we continue to hit the LLVM assertion? That would at least tell us if the behavior changes in the future.

@mojingran mojingran closed this Aug 4, 2023
@mojingran
Copy link
Contributor Author

I think I agree with @bjorn3, since the feature is unstable (and marked perma-unstable at this time by the lang team) we should just offer the full range of LLVM options to the user.

Perhaps the best thing to do right now to close #109861 would be to just add a test that verifies we continue to hit the LLVM assertion? That would at least tell us if the behavior changes in the future.

Sounds reasonable! Will do.

@mojingran mojingran reopened this Aug 8, 2023
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 8, 2023
@mojingran mojingran changed the title Disables unsupported linkage types in Rust. Tests crash from inappropriate use of common linkage Aug 9, 2023
@mojingran
Copy link
Contributor Author

mojingran commented Aug 14, 2023 •
edited
Loading

r? @wesleywiser
edit: Sorry for the accidental ping. I saw there were no reviewers, but only you as an assignee.

@rustbot
Copy link
Collaborator

rustbot commented Aug 14, 2023

Could not assign reviewer from: wesleywiser.
User(s) wesleywiser are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

wesleywiser
wesleywiser requested changes Aug 15, 2023
View reviewed changes
tests/ui/issues/issue-109681.stderr Outdated Show resolved Hide resolved
@mojingran mojingran requested a review from wesleywiser August 15, 2023 15:36
@bors
Copy link
Contributor

bors commented Aug 25, 2023

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

wesleywiser
wesleywiser approved these changes Aug 29, 2023
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

This looks good, thanks @mojingran! Happy to merge after rebase 🙂

WaffleLapkin
WaffleLapkin requested changes Aug 31, 2023
View reviewed changes
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

Please don't add issues/issue-xxxxxx.rs-named tests, and instead move the test to a more suited directory and name it accordingly.

@WaffleLapkin
Copy link
Member

@mojingran could you please also squash the commits together?

@mojingran mojingran force-pushed the master branch 5 times, most recently from cdc73c7 to 1e80c33 Compare September 1, 2023 18:17
@mojingran
Tests crash from inappropriate use of common linkage
4160337 
Normally, variables with common linkage must be zero-initialized. In Rust,
common linkage variables that are not zero-initialized causes a crash in the
compiler backend.

This commit adds a test case to confirm this behavior, which will inform us if
it changes in the future.
@bryanpkc
Copy link

bryanpkc commented Sep 8, 2023

@WaffleLapkin Can this be merged now?

@WaffleLapkin
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 8, 2023

📌 Commit 4160337 has been approved by WaffleLapkin

It is now in the queue for this repository.

@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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 8, 2023
@wesleywiser
Copy link
Member

Thanks for r+'ing @WaffleLapkin!

@mojingran just FYI for future reference, force pushing your PR doesn't generate a notification to the reviewers so feel free to leave a comment saying you force pushed 🙂

@matthiaskrgr matthiaskrgr mentioned this pull request Sep 8, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2023
@bors
Auto merge of rust-lang#115685 - matthiaskrgr:rollup-t31gowy, r=matth…
b0b8c52 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#113807 (Tests crash from inappropriate use of common linkage)
 - rust-lang#115358 (debuginfo: add compiler option to allow compressed debuginfo sections)
 - rust-lang#115630 (Dont suggest use between `use` and cfg attr)
 - rust-lang#115662 (Improve "associated type not found" diagnostics)
 - rust-lang#115673 (Fix sanitize/cfg.rs test)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6b65d37 into rust-lang:master Sep 9, 2023
@rustbot rustbot added this to the 1.74.0 milestone Sep 9, 2023
@bors
Copy link
Contributor

bors commented Sep 9, 2023

⌛ Testing commit 4160337 with merge b0b8c52...

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2023
@rust-timer
Unrolled build for rust-lang#113807
dd1f310 
Rollup merge of rust-lang#113807 - mojingran:master, r=WaffleLapkin

Tests crash from inappropriate use of common linkage

Follows up my proposal under the [tracking issue for the linkage feature](rust-lang#29603 (comment)). Adds test for [issue 109681](rust-lang#109681).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WaffleLapkin WaffleLapkin WaffleLapkin approved these changes

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.
9 participants
@mojingran @rustbot @rust-log-analyzer @cjgillot @WaffleLapkin @bjorn3 @wesleywiser @bors @bryanpkc