Skip to content

Commit

Permalink
Fall bad generated functions back to interpreter (#50348)
Browse files Browse the repository at this point in the history
This fixes #49715. The fix itself is pretty simple - just remove
the generator expansion that was added in #48766, but the bigger
question here is what the correct behavior should be in the first place.

# Dynamic Semantics, generally

The primary question here are of the semantics of generated functions.
Note that this is quite different to how they are implemented. In
general, the way we think about compiling Julia is that there is a
well defined set of *dynamic semantics* that specify what a particular
piece of Julia code means. Julia's dynamic semantics are generally quite
simple (at every point, call the most specific applicable method).
What happens under the hood may be quite different (e.g. lots of inference,
compiling constant folding, etc), but the compilation process should
mostly preserve the semantics (with a few well defined exceptions
around floating point arithmetic, effect assumptions, semantically
unobservable side effects, etc.).

# The dnymaic semantics of generated functions

With that diatribe out of the way, let's think about the dynamic semantics
of generated functions. We haven't always been particularly clear about
this, but I propose it's basically the following:

For a generated function:
```
@generated function f(args...)
    # = generator body =#
end
```

this is semantically equivalent to the function to basically the following:

```
const lno = LineNumberNode(@__FILE__, @__LINE__); function f(args...)
    generator = @opaque @assume_effects :foldable :generator (args...)->#= generator body =#
    body = generator(Base.get_world_counter(), lno, Core.Typeof.(args))
    execute(body, f, args...)
end
```

A couple of notes on this:

1. `@opaque` used here for the world-age capture semantics of the generator itself
2. There's an effects-assumption `:generator` that doesn't exist but is
   supposed to capture the special allowance for calling generators. This
   is discussed more below.

## Implementing `execute`

For a long time, we didn't really have a first-class implementation of `execute`.
It's almost (some liberties around the way that the arguments work, but you get
the idea)

```
execute_eval(body, f, args...) = eval((args...)->$body)(f, args....)
```

but that doesn't have the correct world age semantics (would error
as written and even if you used invokelatest, the body would run
in the wrong world).

However, with OpaqueClosure we do actually have a mechanism now and
we could write:

```
execute(body, f, args...) = OpaqueClosure(body, f)(args...)
```

Again, I'm not proposing this as an implementation, just to give us an idea
of what the dynamic semantics of generated functions are.

# The particular bug (#49715)

The issue in #49715 is that the following happens:
1. A generated function gets called and inference is attempted.
2. Inference attempts to infer the generated function and call the generator.
3. The generator throws an error.
4. Inference fails.
5. The compiler enters a generic inference-failure fallback path
6. The compiler asks for a generator expansion in the generic world (-1)
7. This gives a different error, confusing the user.

There is the additional problem that this error gets thrown at
compilation time, which is not technically legal (and there was an
existing TODO to fix that).

In addition to that, I think there is a separate question of whether it
should be semantically legal to throw an error for a different world age
than the currently running one. Given the semantics proposed above, I
would suggest that the answer should be no. This does depend on the
exact semantics of :generator, but in general, our existing
effects-related notions do not allow particularly strong assumptions on
the particular error being thrown (requiring them to be re-evaluated
at runtime), and I see no reason to depart from this practice here.

Thus, I would suggest that the current behavior should be disallowed
and the expected behavior is that the generic fallback implementation
of generated functions invoke the generator in the runtime world and
expose the appropriate error.

# Should we keep the generic world?

That does leave the question what to do about the generic world (-1).
I'm not 100% convinced that this is necessarily a useful concept to
have. It is true that most generated functions do not depend on the
world age, but they can already indicate this by returning a value
with bounded world range and no backedges (equivalently returning
a plain expression). On the other hand, keeping the generic world
does risk creating the inverse of the situation that prompted this
issue, in that there is no semantically reachable path to calling
the generator with the generic world, making it hard to debug.

As a result, I am very strongly leaning towards removing this concept,
but I am open to being convinced otherwise.

# This PR

This PR, which is considerably shorter than this commit message is very
simple: The attempt to invoke the generator with the generic world -1
is removed. Instead, we fall back to the interpreter, which already
has the precise semantics that I want here - invoking the generator
in the dynamic world and interpreting the result.

# The semantics of :generator

That leaves one issue to be resolved which is the semantics of `:generator`.
I don't think it's necessary to be as precise here as we are about the
other effects we expose, but I propose it be something like the following:

For functions with the :generator effects assumption, :consistent-cy is
relaxed as follows:

1. The requistive notion of equality is relaxed to a "same code and
   metadata" equality of code instances. I don't think we have any
   predicate for this (and it's not necessarily computable), but the
   idea should be that the CodeInstance is always computed in the exact
   same way, but may be mutable and such. Note that this is explicitly
   not functional extensionality, because we do analyze the structure of
   the returned code and codegen based on it.

2. The world-age semantics of :consistent sharpened to require
   our relaxed notion of consistency for any overlapping min_world:max_world
   range returned from the generator.

Co-authored-by: Oscar Smith <[email protected]>
  • Loading branch information
Keno and oscardssmith committed Jun 29, 2023
1 parent f6f3553 commit 3ddceee
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 10 deletions.
15 changes: 6 additions & 9 deletions src/jitlayers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -547,21 +547,18 @@ void jl_generate_fptr_for_unspecialized_impl(jl_code_instance_t *unspec)
jl_method_t *def = unspec->def->def.method;
if (jl_is_method(def)) {
src = (jl_code_info_t*)def->source;
if (src == NULL) {
// TODO: this is wrong
assert(def->generator);
// TODO: jl_code_for_staged can throw
src = jl_code_for_staged(unspec->def, ~(size_t)0);
}
if (src && (jl_value_t*)src != jl_nothing)
src = jl_uncompress_ir(def, NULL, (jl_value_t*)src);
}
else {
src = (jl_code_info_t*)jl_atomic_load_relaxed(&unspec->def->uninferred);
assert(src);
}
if (src) {
assert(jl_is_code_info(src));
++UnspecFPtrCount;
_jl_compile_codeinst(unspec, src, unspec->min_world, *jl_ExecutionEngine->getContext(), 0);
}
assert(src && jl_is_code_info(src));
++UnspecFPtrCount;
_jl_compile_codeinst(unspec, src, unspec->min_world, *jl_ExecutionEngine->getContext(), 0);
jl_callptr_t null = nullptr;
// if we hit a codegen bug (or ran into a broken generated function or llvmcall), fall back to the interpreter as a last resort
jl_atomic_cmpswap(&unspec->invoke, &null, jl_fptr_interpret_call_addr);
Expand Down
19 changes: 19 additions & 0 deletions test/compiler/contextual.jl
Original file line number Diff line number Diff line change
Expand Up @@ -201,3 +201,22 @@ finally
@show err
end
end

# Test that writing a bad cassette-style pass gives the expected error (#49715)
function generator49715(world, source, self, f, tt)
tt = tt.parameters[1]
sig = Tuple{f, tt.parameters...}
mi = Base._which(sig; world)

error("oh no")

stub = Core.GeneratedFunctionStub(identity, Core.svec(:methodinstance, :ctx, :x, :f), Core.svec())
stub(world, source, :(nothing))
end

@eval function doit49715(f, tt)
$(Expr(:meta, :generated, generator49715))
$(Expr(:meta, :generated_only))
end

@test_throws "oh no" doit49715(sin, Tuple{Int})
2 changes: 1 addition & 1 deletion test/staged.jl
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ let gf_err, tsk = @async nothing # create a Task for yield to try to run
Expected = ErrorException("task switch not allowed from inside staged nor pure functions")
@test_throws Expected gf_err()
@test_throws Expected gf_err()
@test gf_err_ref[] == 4
@test gf_err_ref[] < 1000
end

gf_err_ref[] = 0
Expand Down

0 comments on commit 3ddceee

Please sign in to comment.