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.

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

Drop "readonly" LLVM attribute from references to opaque C++ types #1370

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

dtolnay
Copy link
Owner

@dtolnay dtolnay commented Aug 30, 2024

@capickett noticed that passing a cxx-generated opaque C++ type by reference gets annotated with an unwanted, incorrect readonly attribute. We have observed miscompiles that arise from this in real-world code when using cross-language ThinLTO.

According to https://llvm.org/docs/LangRef.html:

readonly

This attribute indicates that the function does not write through this pointer argument, even though it may write to the memory that the pointer points to.

If a function writes to a readonly pointer argument, the behavior is undefined.

For the following repro:

#[no_mangle]
pub fn repro(opaque: &cxx::private::Opaque) {
    extern "C" {
        fn do_repro(opaque: *const cxx::private::Opaque);
    }
    unsafe { do_repro(opaque) }
}

this is the LLVM IR generated before this PR:

; Function Attrs: nounwind nonlazybind uwtable
define void @repro(ptr noalias noundef nonnull readonly align 1 %opaque) unnamed_addr #0 {
start:
  tail call void @do_repro(ptr noundef nonnull %opaque) #1
  ret void
}

and after:

; Function Attrs: nounwind nonlazybind uwtable
define void @repro(ptr noundef nonnull align 1 %opaque) unnamed_addr #0 {
start:
  tail call void @do_repro(ptr noundef nonnull %opaque) #1
  ret void
}
< define void @repro(ptr noalias noundef nonnull readonly align 1 %opaque) unnamed_addr #0 {
---
> define void @repro(ptr noundef nonnull align 1 %opaque) unnamed_addr #0 {

Old compilers didn't used to consider `()` acceptable for FFI.

    error: `extern` block uses type `()`, which is not FFI-safe
      --> demo/src/main.rs:20:14
       |
    20 |           type BlobstoreClient;
       |  ______________^
    21 | |
    22 | |         fn new_blobstore_client() -> UniquePtr<BlobstoreClient>;
    23 | |         fn put(&self, parts: &mut MultiBuf) -> u64;
       | |________________^ not FFI-safe
       |
       = help: consider using a struct instead
       = note: tuples have unspecified layout
    note: the lint level is defined here
      --> demo/src/main.rs:1:1
       |
    1  | #[cxx::bridge(namespace = "org::blobstore")]
       | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       = note: this error originates in the attribute macro `cxx::bridge` (in Nightly builds, run with -Z macro-backtrace for more info)
@dtolnay dtolnay merged commit 54233f7 into master Aug 30, 2024
30 checks passed
@dtolnay dtolnay deleted the unsafecell branch August 30, 2024 20:16
facebook-github-bot pushed a commit to facebookexperimental/rust-shed that referenced this pull request Aug 31, 2024
Summary:
This update includes dtolnay/cxx#1370 to solve S443733.

The other intervening release notes:

- https://github.com/dtolnay/cxx/releases/tag/1.0.125
- https://github.com/dtolnay/cxx/releases/tag/1.0.126
- https://github.com/dtolnay/cxx/releases/tag/1.0.127
- https://github.com/dtolnay/cxx/releases/tag/1.0.128

Reviewed By: kuecks

Differential Revision: D62048225

fbshipit-source-id: 716eae04d2731616b1f88a72ca7b9071649619ca
facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request Aug 31, 2024
Summary:
This update includes dtolnay/cxx#1370 to solve S443733.

The other intervening release notes:

- https://github.com/dtolnay/cxx/releases/tag/1.0.125
- https://github.com/dtolnay/cxx/releases/tag/1.0.126
- https://github.com/dtolnay/cxx/releases/tag/1.0.127
- https://github.com/dtolnay/cxx/releases/tag/1.0.128

Reviewed By: kuecks

Differential Revision: D62048225

fbshipit-source-id: 716eae04d2731616b1f88a72ca7b9071649619ca
facebook-github-bot pushed a commit to facebook/hhvm that referenced this pull request Sep 11, 2024
Summary:
This update includes dtolnay/cxx#1370 to solve S443733.

The other intervening release notes:

- https://github.com/dtolnay/cxx/releases/tag/1.0.125
- https://github.com/dtolnay/cxx/releases/tag/1.0.126
- https://github.com/dtolnay/cxx/releases/tag/1.0.127
- https://github.com/dtolnay/cxx/releases/tag/1.0.128

Reviewed By: kuecks

Differential Revision: D62048225

fbshipit-source-id: 716eae04d2731616b1f88a72ca7b9071649619ca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant