cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char` by Zalathar · Pull Request #132260 · rust-lang/rust

Zalathar · 2024-10-28T10:47:03Z

In rustc_codegen_llvm there are many uses of .as_ptr().cast() to convert a string or byte-slice to *const c_char, which then gets passed through FFI.

This works, but is fragile, because there's nothing constraining the pointer cast to actually be from u8 to c_char. If the original value changes to something else that has an as_ptr method, or the context changes to expect something other than c_char, the cast will silently do the wrong thing.

By making the cast more explicit via a helper method, we can be sure that it will either perform the intended cast, or fail at compile time.

rustbot · 2024-10-28T10:47:12Z

r? @lcnr

rustbot has assigned @lcnr.
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

compiler-errors

Yeah, I think it's always best to constrain "dangling generics" sooner than later, so this extension function seems like a nice little improvement.

compiler-errors · 2024-10-28T14:02:19Z

r? compiler-errors @bors r+

bors · 2024-10-28T14:02:21Z

📌 Commit 4bd84b2 has been approved by compiler-errors

It is now in the queue for this repository.

…r-errors cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char` In `rustc_codegen_llvm` there are many uses of `.as_ptr().cast()` to convert a string or byte-slice to `*const c_char`, which then gets passed through FFI. This works, but is fragile, because there's nothing constraining the pointer cast to actually be from `u8` to `c_char`. If the original value changes to something else that has an `as_ptr` method, or the context changes to expect something other than `c_char`, the cast will silently do the wrong thing. By making the cast more explicit via a helper method, we can be sure that it will either perform the intended cast, or fail at compile time.

…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#130259 (Lower AST node id only once) - rust-lang#131441 (Add a new trait `proc_macro::ToTokens`) - rust-lang#132247 (stable_mir: Directly use types from rustc_abi) - rust-lang#132249 (compiler: Add rustc_abi dependence to the compiler) - rust-lang#132255 (Add `LayoutS::is_uninhabited` and use it) - rust-lang#132258 ([rustdoc] Unify variant struct fields margins with struct fields) - rust-lang#132260 (cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char`) - rust-lang#132261 (refactor: cleaner check to return None) - rust-lang#132271 (Updating Fuchsia platform-support documentation) r? `@ghost` `@rustbot` modify labels: rollup

…r-errors cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char` In `rustc_codegen_llvm` there are many uses of `.as_ptr().cast()` to convert a string or byte-slice to `*const c_char`, which then gets passed through FFI. This works, but is fragile, because there's nothing constraining the pointer cast to actually be from `u8` to `c_char`. If the original value changes to something else that has an `as_ptr` method, or the context changes to expect something other than `c_char`, the cast will silently do the wrong thing. By making the cast more explicit via a helper method, we can be sure that it will either perform the intended cast, or fail at compile time.

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#130259 (Lower AST node id only once) - rust-lang#131441 (Add a new trait `proc_macro::ToTokens`) - rust-lang#132247 (stable_mir: Directly use types from rustc_abi) - rust-lang#132249 (compiler: Add rustc_abi dependence to the compiler) - rust-lang#132255 (Add `LayoutData::is_uninhabited` and use it) - rust-lang#132258 ([rustdoc] Unify variant struct fields margins with struct fields) - rust-lang#132260 (cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char`) - rust-lang#132261 (refactor: cleaner check to return None) - rust-lang#132271 (Updating Fuchsia platform-support documentation) - rust-lang#132295 (fixed wast version was released, remove randomization exemption) r? `@ghost` `@rustbot` modify labels: rollup

…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#130259 (Lower AST node id only once) - rust-lang#131441 (Add a new trait `proc_macro::ToTokens`) - rust-lang#132247 (stable_mir: Directly use types from rustc_abi) - rust-lang#132249 (compiler: Add rustc_abi dependence to the compiler) - rust-lang#132255 (Add `LayoutData::is_uninhabited` and use it) - rust-lang#132258 ([rustdoc] Unify variant struct fields margins with struct fields) - rust-lang#132260 (cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char`) - rust-lang#132261 (refactor: cleaner check to return None) - rust-lang#132271 (Updating Fuchsia platform-support documentation) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#132260 - Zalathar:type-safe-cast, r=compiler-errors cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char` In `rustc_codegen_llvm` there are many uses of `.as_ptr().cast()` to convert a string or byte-slice to `*const c_char`, which then gets passed through FFI. This works, but is fragile, because there's nothing constraining the pointer cast to actually be from `u8` to `c_char`. If the original value changes to something else that has an `as_ptr` method, or the context changes to expect something other than `c_char`, the cast will silently do the wrong thing. By making the cast more explicit via a helper method, we can be sure that it will either perform the intended cast, or fail at compile time.

klensy · 2024-10-29T14:23:05Z

compiler/rustc_codegen_llvm/src/common.rs

+impl AsCCharPtr for str {
+    fn as_c_char_ptr(&self) -> *const c_char {
+        self.as_ptr().cast()
+    }
+}


Isn't this wrong? &str can contain null bytes and casting it to c_char will lead to funny results in that case. It works for now because all used strings are well behaved and do not contain nulls.

Same with [u8].

in #132319 it broken further: c literals (for which it's ok to take *c_char), you changed to &str, which don't have guarantees of cstr.

A pointer to c_char doesn't have to be nul-terminated. We have plenty of code that passes strings over FFI as pointer/length pairs.

As long as it gets reassembled with StringRef(Ptr, Len) on the C++ side, it's fine.

And in #132319, all of the relevant FFI functions have been adjusted to take pointer/length pairs instead of nul-terminated strings, so there's no longer any need for those strings to be c"" literals.

Yes, but fragile. Can't find if it ok for StringRef to have nulls inside, if it later pass them as c strings, things will blow.

klensy · 2024-10-29T22:51:38Z

compiler/rustc_codegen_llvm/src/back/write.rs

            let section = bitcode_section_name(cgcx);
-            llvm::LLVMSetSection(llglobal, section.as_ptr().cast());
+            llvm::LLVMSetSection(llglobal, section.as_c_char_ptr());


LLVMSetSection accepts const char* without length, where section is &str.

Ahh, bitcode_section_name have fixme, so it can actually return cstr literals.

Yeah, and all of the calls to llvm::LLVMSetSection should really be calling the safe wrapper llvm::set_section (which takes &CStr), which would enforce type-safety.

…rrors cg_llvm: Consistently use safe wrapper function `set_section` Follow-up to rust-lang#131962 and rust-lang#132260 (comment). To avoid too much scope creep, I've deliberately kept the changes to `LLVMRustGetSliceFromObjectDataByName` as minimal as possible.

Rollup merge of rust-lang#132340 - Zalathar:set-section, r=compiler-errors cg_llvm: Consistently use safe wrapper function `set_section` Follow-up to rust-lang#131962 and rust-lang#132260 (comment). To avoid too much scope creep, I've deliberately kept the changes to `LLVMRustGetSliceFromObjectDataByName` as minimal as possible.

Use a type-safe helper to cast &str and &[u8] to *const c_char

4bd84b2

rustbot assigned lcnr Oct 28, 2024

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 Oct 28, 2024

compiler-errors approved these changes Oct 28, 2024

View reviewed changes

rustbot assigned compiler-errors and unassigned lcnr Oct 28, 2024

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 Oct 28, 2024

GuillaumeGomez mentioned this pull request Oct 28, 2024

Rollup of 5 pull requests #132269

Closed

This was referenced Oct 28, 2024

Rollup of 9 pull requests #132273

Closed

Rollup of 9 pull requests #132277

Merged

matthiaskrgr mentioned this pull request Oct 29, 2024

Rollup of 10 pull requests #132307

Closed

workingjubilee mentioned this pull request Oct 29, 2024

Rollup of 12 pull requests #132308

Closed

bors merged commit bd43f8e into rust-lang:master Oct 29, 2024

rustbot added this to the 1.84.0 milestone Oct 29, 2024

Zalathar deleted the type-safe-cast branch October 29, 2024 10:05

klensy reviewed Oct 29, 2024

View reviewed changes

Zalathar mentioned this pull request Oct 30, 2024

cg_llvm: Consistently use safe wrapper function set_section #132340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char`#132260

cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char`#132260
bors merged 1 commit intorust-lang:masterfrom
Zalathar:type-safe-cast

Zalathar commented Oct 28, 2024

Uh oh!

rustbot commented Oct 28, 2024

Uh oh!

compiler-errors left a comment

Uh oh!

compiler-errors commented Oct 28, 2024

Uh oh!

bors commented Oct 28, 2024

Uh oh!

klensy Oct 29, 2024 •

edited

Loading

Uh oh!

klensy Oct 29, 2024

Uh oh!

Zalathar Oct 29, 2024

Uh oh!

Zalathar Oct 29, 2024

Uh oh!

klensy Oct 29, 2024 •

edited

Loading

Uh oh!

klensy Oct 29, 2024

Uh oh!

Zalathar Oct 29, 2024

Uh oh!

Zalathar Oct 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

Zalathar commented Oct 28, 2024

Uh oh!

rustbot commented Oct 28, 2024

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Oct 28, 2024

Uh oh!

bors commented Oct 28, 2024

Uh oh!

klensy Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klensy Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

klensy Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klensy Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

klensy Oct 29, 2024 •

edited

Loading

klensy Oct 29, 2024 •

edited

Loading