Skip to content

Don't declare test_variadic_fnptr with two conflicting signatures - #95088

Merged
bors merged 2 commits into
rust-lang:masterfrom
bjorn3:fix_test_variadic_fnptr
Mar 23, 2022
Merged

Don't declare test_variadic_fnptr with two conflicting signatures#95088
bors merged 2 commits into
rust-lang:masterfrom
bjorn3:fix_test_variadic_fnptr

Conversation

@bjorn3

@bjorn3bjorn3 commented Mar 18, 2022•
edited
Loading

Copy link
Copy Markdown
Member

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

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

rust-highfive commented Mar 18, 2022

Copy link
Copy Markdown
Contributor

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

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

rust-highfive commented Mar 18, 2022

Copy link
Copy Markdown
Contributor

r? @scottmcm

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

@rust-highfiverust-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
Comment threadlibrary/core/tests/ptr.rs Outdated
@bjorn3
bjorn3force-pushed the fix_test_variadic_fnptr branch from ccd16da to 0467300CompareMarch 19, 2022 09:19
@dtolnaydtolnay assigned dtolnay and unassigned scottmcmMar 19, 2022
dtolnay
dtolnay approved these changes Mar 19, 2022
View reviewed changes

@dtolnaydtolnay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@dtolnay

dtolnay commented Mar 19, 2022

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Mar 19, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 0467300 has been approved by dtolnay

@borsbors 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 ofrust-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/806Fixesrust-lang#66690
@Dylan-DPCDylan-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 ofrust-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/806Fixesrust-lang#66690
@matthiaskrgrmatthiaskrgr mentioned this pull request Mar 19, 2022
Closed
@matthiaskrgr

matthiaskrgr commented Mar 20, 2022

Copy link
Copy Markdown
Member

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

@borsbors 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

bjorn3 commented Mar 20, 2022

Copy link
Copy Markdown
MemberAuthor

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

@bjorn3

bjorn3 commented Mar 20, 2022

Copy link
Copy Markdown
MemberAuthor

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

bors commented Mar 20, 2022

Copy link
Copy Markdown
Collaborator

☔ 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
bjorn3force-pushed the fix_test_variadic_fnptr branch from 0467300 to 56939ffCompareMarch 20, 2022 20:09
@bjorn3

bjorn3 commented Mar 20, 2022

Copy link
Copy Markdown
MemberAuthor

(rebased, but haven't fixed it yet)

dtolnay
dtolnay requested changes Mar 22, 2022
View reviewed changes

@dtolnaydtolnay left a comment

Copy link
Copy Markdown
Member

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

bjorn3 commented Mar 22, 2022

Copy link
Copy Markdown
MemberAuthor

Done

@dtolnay

dtolnay commented Mar 22, 2022

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Mar 22, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 4af755b has been approved by dtolnay

@borsbors 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

bors commented Mar 23, 2022

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Mar 23, 2022

Copy link
Copy Markdown
Collaborator

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

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Mar 23, 2022
@bors
bors merged commit 2b50739 into rust-lang:masterMar 23, 2022
@rustbotrustbot added this to the 1.61.0 milestone Mar 23, 2022
@rust-timer

rust-timer commented Mar 23, 2022

Copy link
Copy Markdown
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

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

Reviewers

@dtolnaydtolnaydtolnay approved these changes

Assignees

@dtolnaydtolnay

Labels

A-craneliftThings relevant to the [future] cranelift backendmerged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant 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