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

Tracking issue for #[ffi_pure] #58329

Open
gnzlbg opened this issue Feb 9, 2019 · 9 comments
Open

Tracking issue for #[ffi_pure] #58329

gnzlbg opened this issue Feb 9, 2019 · 9 comments
Labels
A-FFI Area: Foreign function interface (FFI) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 9, 2019

Annotates an extern C function with C pure attribute.

@jonas-schievink jonas-schievink added B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Feb 9, 2019
@nagisa
Copy link
Member

nagisa commented Feb 9, 2019

This corresponds to the readonly LLVM attribute.

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Feb 11, 2019
@jonas-schievink jonas-schievink added the A-FFI Area: Foreign function interface (FFI) label Aug 5, 2020
@joshtriplett joshtriplett added the S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. label Jun 15, 2022
@joshtriplett
Copy link
Member

Is this fully implemented and ready for potential stabilization, or is there any blocker?

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 1, 2023
…twco

Strengthen validation of FFI attributes

Previously, `codegen_attrs` validated the attributes `#[ffi_pure]`, `#[ffi_const]`, and `#[ffi_returns_twice]` to make sure that they were only used on foreign functions. However, this validation was insufficient in two ways:

1. `codegen_attrs` only sees items for which code must be generated, so it was unable to raise errors when the attribute was incorrectly applied to macros and the like.
2. the validation code only checked that the item with the attr was foreign, but not that it was a foreign function, allowing these attributes to be applied to foreign statics as well.

This PR moves the validation to `check_attr`, which sees all items. It additionally changes the validation to ensure that the attribute's target is `Target::ForeignFunction`, only allowing the attributes on foreign functions and not foreign statics. Because these attributes are unstable, there is no risk for backwards compatibility. The changes also ending up making the code much easier to read.

This PR is best reviewed commit by commit. Additionally, I was considering moving the tests to the `attribute` subdirectory, to get them out of the general UI directory. I could do that as part of this PR or a follow-up, as the reviewer prefers.

CC: rust-lang#58328, rust-lang#58329
@workingjubilee workingjubilee added the T-opsem Relevant to the opsem team label May 19, 2024
@workingjubilee
Copy link
Member

I am not sure but I believe that this and its cousin, #58328, are of interest to the opsem team, as they significantly affect what optimizations the codegen backend is allowed to perform (and declaring them is probably very unsafe!). cc @rust-lang/opsem

@RalfJung
Copy link
Member

Yes this sounds very unsafe. I assume it works a lot like asm! annotations? Are the requirements the same as what the Reference says about pure there?

@CAD97
Copy link
Contributor

CAD97 commented May 20, 2024

Interestingly, I see LLVM readonly is applied to the @llvm.type.checked.load.relative intrinsic but I don't actually see it defined for functions, only for parameters. I presume it means the same as memory(read). IIUC, GCC's pure is close to but more permissive than1 LLVM's memory(read, inaccessiblemem: readwrite) and GCC's const is close to but more permissive than2 LLVM's memory(none).

We do already expose a similar feature, in that asm! blocks can be annotated with options(pure, nomem) or options(pure, readonly). IIUC, these would correspond to GCC const and pure, respectively. Even though the purpose of the Rust attribute is to match C headers using __attribute__(), I think it'd be preferable to spell them something like #[pure(nomem)]/#[pure(readonly)] instead, to make the Rust semantic more clear.

With the removal of #[ffi_returns_twice], I don't think we want #[ffi_*] style attributes. If we want this, it should apply just as much without any FFI involved. The one advantage of applying to extern declared functions is that the point of unsafe for their signature being accurate is calling them. If allowed on fn items, either the attribute would need to similarly defer the safety of the annotation to the caller or be itself unsafe to use.

However, I can't come up with a way to define the semantics operationally in a way Miri could check. Even a brute force attempt that retags all globally accessible locations as Frozen and makes the called function use the new tag for global loads isn't sufficient, as a pointer loaded out of a global still has its existing provenance (thus can validly be used for writes).

Well, except for a shim that calls the actual symbol from an asm! block and uses those options annotations.

Footnotes

  1. GCC says that “functions declared with the pure attribute can safely [...] modify the value of objects in a way that does not affect their return value or the observable state of the program.” LLVM's inaccessiblemem: readwrite permits e.g. encapsulated usage of malloc/free, but I don't know how exactly to interpret GCC's notion of writes that exist but aren't observable.

  2. GCC says that “functions declared with the [const] attribute can safely read objects that do not change their return value, such as non-volatile constants.” LLVM doesn't expose a knob for allowing reads of runtime constant memory but not runtime mutable memory.

@workingjubilee
Copy link
Member

hmm, cc @antoyo Do you by any chance have any insight into the question about GCC's semantics for the pure and const attributes?

@RalfJung
Copy link
Member

However, I can't come up with a way to define the semantics operationally in a way Miri could check. Even a brute force attempt that retags all globally accessible locations as Frozen and makes the called function use the new tag for global loads isn't sufficient, as a pointer loaded out of a global still has its existing provenance (thus can validly be used for writes).

I think we can get most of pure, readonly with a per-thread flag indicating that this thread is currently in "pure" mode, and rejecting all sorts of operations if the current thread is in "pure" mode -- in particular, writes and fences. I guess we'd have to allow writes to the locals of the stack pure frame itself (and functions it calls), but that should be doable. Something similar could work for pure, nomem.

The part that is obviously impossible to check is the requirement that this function must terminate.

@workingjubilee
Copy link
Member

The pure attribute prohibits a function from modifying the state of the program that is observable by means other than inspecting the function’s return value. However, functions declared with the pure attribute can safely read any non-volatile objects, and modify the value of objects in a way that does not affect their return value or the observable state of the program.

Hmm, yeah, I'm also not really sure what that "modify the value of objects in a way that does not affect their return value or the observable state of the program" means, but I think what it's trying to say is "it's still pure if internally it uses something like OnceCell to memoize some computations, you're just not allowed to leak that fact".

@workingjubilee
Copy link
Member

workingjubilee commented May 21, 2024

What I just said doesn't sound like an easy-to-specify operational semantics, so yeah, "spec what we feel is appropriate and assert it has supremacy over the C attributes, then emit whatever optimization annotations qualify" sounds like the path we have to take if we don't want these to have YOLO opsem.

That does indeed lead to "we probably shouldn't try to force ourselves to reuse their names" conclusions, even if we try to get it very close. It is bad for annotations which can significantly alter program semantics to suffer from the false friend problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Development

No branches or pull requests

8 participants