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

Make RustString an extern type to avoid improper_ctypes warnings #132549

Merged
merged 2 commits into from
Nov 9, 2024
Merged

Make RustString an extern type to avoid improper_ctypes warnings #132549

merged 2 commits into from
Nov 9, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Nov 3, 2024

Currently, any FFI function that uses &RustString needs to also add #[ignore(improper_ctypes)] to silence a warning.

The warning is not completely bogus, because RustString contains Vec<u8> and therefore does not have a guaranteed layout. But we have no way of telling the lint that this doesn't matter, because the C++ code only uses that pointer opaquely and never relies on its underlying layout.

Ideally there would be some way to silence improper_ctypes at the type-definition site. But because there isn't, casting to and from a separate extern type is better than having to annotate every single use site.

@rustbot
Copy link
Collaborator

rustbot commented Nov 3, 2024

r? @cuviper

rustbot has assigned @cuviper.
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 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 Nov 3, 2024
@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 3, 2024

I also want to rename RustString to something else, because it's a raw byte buffer rather than a UTF-8 sequence, but that would be too much scope creep for this PR.

@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 3, 2024

See also #116963 for general dissatisfaction with improper_ctypes.

@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 3, 2024

I'm not 100% confident that casting between normal references and extern type references like this is OK, but it seems reasonable?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 3, 2024

See 47e932b for why RustString is in rustc_llvm.

@Zalathar Zalathar force-pushed the rust-string branch 4 times, most recently from a1dcb02 to 730d088 Compare November 3, 2024 22:31
@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 3, 2024

Rebased to remove another ignore added by #132514 (diff).

@bors
Copy link
Contributor

bors commented Nov 8, 2024

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

@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 8, 2024

Rebased to resolve conflicts and remove more ignore/expect added by (my) other PRs.

Zalathar
Zalathar commented Nov 8, 2024
View reviewed changes
compiler/rustc_llvm/src/lib.rs
#![feature(rustdoc_internals)]
#![warn(unreachable_pub)]
// tidy-alphabetical-end

// NOTE: This crate only exists to allow linking on mingw targets.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what this was originally referring to, but it's pretty clearly no longer true of the present crate.

cuviper
cuviper requested changes Nov 8, 2024
View reviewed changes
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Overall it looks like a nice improvement! I just have a couple quibbles.

compiler/rustc_llvm/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_llvm/src/lib.rs Outdated Show resolved Hide resolved
@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 Nov 8, 2024
@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 9, 2024

Addressed review feedback (diff).

@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 9, 2024

@rustbot ready

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

cuviper commented Nov 9, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2024

📌 Commit 89d7efa has been approved by cuviper

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 Nov 9, 2024
@bors
Copy link
Contributor

bors commented Nov 9, 2024

⌛ Testing commit 89d7efa with merge 012ae13...

@bors
Copy link
Contributor

bors commented Nov 9, 2024

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 012ae13 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 9, 2024
@bors bors merged commit 012ae13 into rust-lang:master Nov 9, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 9, 2024
@Zalathar Zalathar deleted the rust-string branch November 9, 2024 07:10
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (012ae13): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary 3.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 3.1% [2.5%, 4.3%] 4
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 780.614s -> 780.969s (0.05%)
Artifact size: 335.33 MiB -> 335.32 MiB (-0.00%)

mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
@bors @mati865
Auto merge of rust-lang#132549 - Zalathar:rust-string, r=cuviper
242ca99 
Make `RustString` an extern type to avoid `improper_ctypes` warnings

Currently, any FFI function that uses `&RustString` needs to also add `#[ignore(improper_ctypes)]` to silence a warning.

The warning is not _completely_ bogus, because `RustString` contains `Vec<u8>` and therefore does not have a guaranteed layout. But we have no way of telling the lint that this doesn't matter, because the C++ code only uses that pointer opaquely and never relies on its underlying layout.

Ideally there would be some way to silence `improper_ctypes` at the type-definition site. But because there isn't, casting to and from a separate extern type is better than having to annotate every single use site.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper requested changes

Assignees

@cuviper cuviper

Labels
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.84.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Zalathar @rustbot @rust-log-analyzer @bors @cuviper @rust-timer