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

Don't declare test_variadic_fnptr with two conflicting signatures #95088

Merged
merged 2 commits into from
Mar 23, 2022
Merged

Don't declare test_variadic_fnptr with two conflicting signatures #95088

merged 2 commits into from
Mar 23, 2022

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Mar 18, 2022 •
edited
Loading

It is UB for LLVM and results in a compile error for Cranelift.

cc https://github.com/bjorn3/rustc_codegen_cranelift/issues/806
Fixes #66690

@bjorn3 bjorn3 added the A-cranelift Things relevant to the [future] cranelift backend label Mar 18, 2022
@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 18, 2022
@rust-highfive
Copy link
Collaborator

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 18, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
dtolnay
dtolnay requested changes Mar 18, 2022
View reviewed changes
@bjorn3 bjorn3 force-pushed the fix_test_variadic_fnptr branch from ccd16da to 0467300 Compare March 19, 2022 09:19
@dtolnay dtolnay assigned dtolnay and unassigned scottmcm Mar 19, 2022
dtolnay
dtolnay approved these changes Mar 19, 2022
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Mar 19, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2022

📌 Commit 0467300 has been approved by dtolnay

@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 Mar 19, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 19, 2022
@Dylan-DPC
Rollup merge of rust-lang#95088 - bjorn3:fix_test_variadic_fnptr, r=d…
ad914ed 
…tolnay

Don't declare test_variadic_fnptr with two conflicting signatures

It is UB for LLVM and results in a compile error for Cranelift.

cc https://github.com/bjorn3/rustc_codegen_cranelift/issues/806
Fixes rust-lang#66690
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 19, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 19, 2022
@matthiaskrgr
Rollup merge of rust-lang#95088 - bjorn3:fix_test_variadic_fnptr, r=d…
d47d0ba 
…tolnay

Don't declare test_variadic_fnptr with two conflicting signatures

It is UB for LLVM and results in a compile error for Cranelift.

cc https://github.com/bjorn3/rustc_codegen_cranelift/issues/806
Fixes rust-lang#66690
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 19, 2022
Closed
@matthiaskrgr
Copy link
Member

Looks like this causes linking failures on windows:
#95123 (comment)
@bors r- rollup=iffy

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 20, 2022
@bjorn3
Copy link
Member Author

bjorn3 commented Mar 20, 2022

I hoped that printf would be available on msvc. I think I will need to look for another symbol that is.

@bjorn3
Copy link
Member Author

bjorn3 commented Mar 20, 2022

According to https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/printf-printf-l-wprintf-wprintf-l?view=msvc-170 printf should be available. @dtolnay do you have any clue what the issue could be? And if not would it be fine to disable the test on windows?

@bors
Copy link
Contributor

bors commented Mar 20, 2022

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

@bjorn3
Don't declare test_variadic_fnptr with two conflicting signatures
56939ff 
It is UB for LLVM and results in a compile error for Cranelift
@bjorn3 bjorn3 force-pushed the fix_test_variadic_fnptr branch from 0467300 to 56939ff Compare March 20, 2022 20:09
@bjorn3
Copy link
Member Author

bjorn3 commented Mar 20, 2022

(rebased, but haven't fixed it yet)

dtolnay
dtolnay requested changes Mar 22, 2022
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Don't know. I'd just exclude this test on Windows too.

@bjorn3
Copy link
Member Author

bjorn3 commented Mar 22, 2022

Done

@dtolnay
Copy link
Member

dtolnay commented Mar 22, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 22, 2022

📌 Commit 4af755b has been approved by dtolnay

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2022
@bors
Copy link
Contributor

bors commented Mar 23, 2022

⌛ Testing commit 4af755b with merge 2b50739...

@bors
Copy link
Contributor

bors commented Mar 23, 2022

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 2b50739 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 23, 2022
@bors bors merged commit 2b50739 into rust-lang:master Mar 23, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 23, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2b50739): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@bjorn3 bjorn3 mentioned this pull request Mar 25, 2022
Closed
@bjorn3 bjorn3 deleted the fix_test_variadic_fnptr branch May 11, 2022 10:21
@bjorn3 bjorn3 mentioned this pull request Oct 22, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
A-cranelift Things relevant to the [future] cranelift backend merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

Test variadic fnptr uses incompatible definition and declaration
9 participants
@bjorn3 @rust-highfive @rust-log-analyzer @dtolnay @bors @matthiaskrgr @rust-timer @scottmcm @rustbot