Simplify dynamic bytecode loading by having the runtime append RETURN 1

[stedolan]

There is a function Meta.reify_bytecode which is used to turn a sequence of bytecode instructions into a callable closure, used by the toplevel and by Dynlink. To do this, the bytecode sequence needs to end in a RETURN 1 instruction.

Currently, this RETURN 1 instruction is added by the caller of reify_bytecode [*]. This patch makes reify_bytecode do this step instead. This seems cleaner, as it removes hardcoded binary strings from the callers of reify_bytecode, who need no longer care about endianness / opcode numbering, in particular removing a dependency from Dynlink to Opcodes. (The runtime now has to do this, but the runtime must already care about such things).

[*] actually this was done by only two of the three callers. The third now gets a redundant RETURN 1, but an extra bytecode instruction that's never executed isn't harmful.

cc @shindere

[shindere]

While working on #11996 I did encounter these bits of code. What I did there was to factorize the patching of the bytecode done in Dynlink and in the toplevel so that the patching function was shared, but the solution proposed here definitely looks better than the one proposed there so I joyfully approved this PR. @stedolan when you will be updating the Changes entry, could you please take this opportunity to also squash the two commits? After that I think this will be ready to be merged.

[shindere]

Stephen Dolan (2023/07/26 03:55 -0700):

[*] actually this was done by only two of the three callers. The third now gets a redundant `RETURN 1`, but an extra bytecode instruction that's never executed isn't harmful.

If we wanted to, I am assuming we could also give `reify_bytecode` a parameter to saywhether we want the RETURN 1 to be added or not, or some variation of that.

[lthls]

[*] actually this was done by only two of the three callers. The third now gets a redundant RETURN 1, but an extra bytecode instruction that's never executed isn't harmful.

Removing the redundant return should be a simple matter of changing [Kreturn 1] to [] in Bytegen.compile_phrase, if I read the code correctly.

[xavierleroy]

Removing the redundant return should be a simple matter of changing [Kreturn 1] to [] in Bytegen.compile_phrase, if I read the code correctly.

I think this is correct, and I would be happier with this PR if this was done, so that the "contract" for reify_bytecode is now "it takes any piece of bytecode that runs to completion and doesn't leave anything on the stack, and turns it into a function".

[stedolan]

Removing the redundant return should be a simple matter of changing [Kreturn 1] to [] in Bytegen.compile_phrase, if I read the code correctly.

I think this is correct, and I would be happier with this PR if this was done, so that the "contract" for reify_bytecode is now "it takes any piece of bytecode that runs to completion and doesn't leave anything on the stack, and turns it into a function".

I agree this sounds better, but sadly turned out not to be correct because of some trickiness with how toplevel codegen worked. I refactored the toplevel slightly until this became correct, then applied the suggested change. (In principle, the most recent commit is "just" removing the [Kreturn 1], but the actual commit is a bit longer than that)

[lthls]

If I understand correctly the new patch:

• For compilation units (which are unchanged by this patch), if there are functions in the module, the whole bytecode code looks like Kbranch lbl :: fun_code @ (Klabel lbl :: init_code). So when we need to add instructions at the beginning or end of the module initialisation code, we can do it around the whole bytecode.
• For toplevel phrases, if there are functions, the whole bytecode used to look like init_code @ (Kreturn 1 :: fun_code). This meant that the extra Kreturn 1 added by this PR was harmless, but did not always replace the already present instruction. With the new commit, the same scheme as for compilation units is used, so reify_bytecode can modify the initialisation code by prepending or appending to the whole bytecode file too.

I think this PR also includes a change to the initial stack size for toplevel phrases from 1 to 0; this looks harmless at first glance but I wouldn't mind some confirmation.

[stedolan]

That summary is correct, thanks.

I think this PR also includes a change to the initial stack size for toplevel phrases from 1 to 0; this looks harmless at first glance but I wouldn't mind some confirmation.

Well spotted! This change was accidental, I've reverted it. (I don't know why toplevel phrases specify 1 here, though)

[stedolan]

@lthls Are you happy with the current version?

[lthls]

@lthls Are you happy with the current version?

Yes, I am.