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

Add Expr(:funcinfo, ... and unsafe verifier #43747

Closed
wants to merge 3 commits into from
Closed

Conversation

vchuravy
Copy link
Sponsor Member

As discussed in #41616 we need some way of marking function as being used in an unsafe situation.
Unsafe meaning executed on a non-Julia thread, where we can not access any TLS variables. This PR mocks
up the necessary changes so that we can discuss this approach right now:

julia> function f_unsafe()
         Base.@unsafe()
       end
julia> f_unsafe()

julia> function f_unsafe()
         Base.@unsafe()
         return Ref(Int)
       end
julia> f_unsafe()
Verifying unsafe function julia_f_unsafe_43 failed

signal (6): Aborted
in expression starting at REPL[4]:1
gsignal at /usr/lib/libc.so.6 (unknown line)
abort at /usr/lib/libc.so.6 (unknown line)
runOnFunction at /home/vchuravy/src/julia/src/llvm-unsafe-verifier.cpp:66 [inlined]

Changes:

  • Add Expr(:funcinfo) for setting LLVM metadata on a function
  • Add macro unsafe
  • Add nascent unsafe verifier pass

@vchuravy vchuravy added compiler:codegen Generation of LLVM IR and native code needs compat annotation Add !!! compat "Julia x.y" to the docstring needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change kind:feature Indicates new feature / enhancement requests domain:ffi foreign function interfaces, ccall, etc. labels Jan 10, 2022
@JeffBezanson
Copy link
Sponsor Member

Yes, this is a good idea! I think we'll need some more specific names for various kinds of restrictions that people want in different cases. For example, no-runtime (doesn't call into the julia runtime), maybe no-allocation, foreign-thread (this case). The foreign-thread restriction might be the same as no-runtime, unless we carefully catalog runtime system functions that don't access TLS?

I assume this needs to be recursive as well; storing a flag on each function if it is "unsafe" and checking that unsafe functions only call other unsafe functions?

The really hard part I think is when to throw the error. Some options:

  • When the method is defined. In this case it needs to have concrete argument types, so we can compile it right away and do the checks.
  • When the function is called: probably impossible, since I guess all we could do is print a message and abort, which is no good.
  • When @cfunction is called on it. This is probably the most strictly compatible with julia's normal execution model, but may be too late to be most useful.

@vchuravy
Copy link
Sponsor Member Author

Yeah, right now it is when it is compiled (which may or may not be during @cfunction as we learned during #43748). We probably also need to propagate it upwards to the capi wrapper. Right now it terminates the program, which is also not the best error message.

I assume this needs to be recursive as well; storing a flag on each function if it is "unsafe" and checking that unsafe functions only call other unsafe functions?

Yeah this seemed challenging and very invasive. It might be useful to have a "strict" mode and a lenient mode.

We could have multiple flags? And then allow them to be set from the macro.

julia.unsafe.allow_tls = {true|false}
julia.unsafe.allow_alloc = {true|false} # false -> implies allow_tls=false
julia.unsafe.allow_rt = {true|false} # false -> implies allow_alloc=false, allow_tls=false

julia.unsafe.strict = {true|false} # all called functions must "unsafe" as well
julia.unsafe.nocalls # no function calls

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 12, 2022

Since we always support runtime function replacement (aka #265), I am not sure how this could ever be made to work.

@vchuravy
Copy link
Sponsor Member Author

I am not sure how this could ever be made to work.

Can you expand on this? There are two different properties here that I want.

  1. For Emit safepoints at function entry  #41616 I need a way to annotate methods that are going to be called on foreign threads as not eligible for safepoint insertion.
  2. I would like to have a static verification that a @cfunction compilation is not going to contain runtime interactions

Now I can see how #265 can come and rain on my parade, by invalidating the function (actually a good question what happens with a @cfunction in that case). Precisely because of this I would like to have a verification step that will error, instead of having mysterious failures down the road.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 12, 2022

@cfunction always interacts with the runtime first, to check for method updates (ala #265), and only then decides what method to call

@JeffBezanson
Copy link
Sponsor Member

That makes me think this should really be part of @cfunction then --- you tell cfunction "give me a pointer to code for this function with the following properties" and it errors if it can't fulfill that request.

@tkf
Copy link
Member

tkf commented Jan 16, 2022

(:+1: My intuition before reading all the discussion was also that it should be done at @cfunction, too.)

If the compiler checks that the given function does not need ptls etc. at the time @cfunction is invoked, it sounds strange to call this @unsafe. If we conceptualize @unsafe is like @inbounds, it means that it's the programmer who checks that given code is safe. It's similar to Rust's unsafe. But then if the compiler can throw at @cfunction, it's the compiler that verifies the ptls-freeness (and other things). In a way, the function annotated by this macro is very safe.

So, how about @runtimefree? I'm thinking "runtime" as PARTR + GC + dynamic dispatch + (other things accessed through current_task like threadid).

@adkabo
Copy link
Contributor

adkabo commented Feb 2, 2022

#43852 provides @assume_effects. @assume_* could be a more general naming convention for these.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 25, 2022

No longer needed. Though we could add a metadata/compiler option that turns off #41616

@vtjnash vtjnash deleted the vc/funcinfo branch October 25, 2022 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code domain:ffi foreign function interfaces, ccall, etc. kind:feature Indicates new feature / enhancement requests needs compat annotation Add !!! compat "Julia x.y" to the docstring needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants