Re: Preliminary: Porting Poke to the latest Jitter, with sub-package mode and the unified routine API

[Jose E. Marchesi]

    > Hmm, you didn't change pvm_program_point to pvm_routine_point in Jitter?
    
    No, and I like it this way.  "Program point" is a standard phrase,
    identifying a source location as related to the dynamic semantics of a
    program.

Ok.
    
    A routine is a compilation unit for Jitter.  Execution will always
    start from some program point in a known routine but then it may branch
    to some other program point belonging to a different routine.  Soon it
    will be possible to have multiple entry points per routine: I have an
    important use case for that, related to tail calls.
    
    The mapping between program points and routines will become even more
    complex with stepping; imagine being able to run a VM instruction and
    then returning to C, with the program point being saved to be able to
    branch back to the next instruction.  Which routine is the current one
    can be computed if needed, but after the code is compiled it is not even
    particularly important, except for debugging.
    
    Of course being very undisciplined in the mapping between closures,
    routines and program points might make routine finalization more
    difficult when routines are garbage collected, and I do not recommend
    it.  I would still say that the name is the right one.
    
    > Hm, if --enable-debug no longer controls how the Jitter VM is built,
    
    With my proposed change --enable-debug no longer does anything, in fact.
    We could remove it, but you may want to reuse the option for some other
    purpose in the future.
    
    > how can the user force building with direct threading when she wants
    > to debug something in pvm.jitter?
    
    You can pass options to the sub-package configure from the super-package
    configure: in particular you can, for example, configure *Poke* with
    --enable-dispatch-no-threading.  You can pass any combination of options
    manually enabling or disabling specific dispatches, if you want.  The
    default is meant to always remain safe and correct, even as Jitter
    evolves.
    
    Right now there is no obvious way of *computing* options for the
    sub-package configure script from the super-package configure script,
    even if it would be nice.  The internal implementation in Autoconf makes
    it difficult.

Oh, that's unfortunate.

Well, then please update the "Building with Debuggin Support" section in
HACKING documenting --enable-dispatch-no-threading.
    
    If you really like the old behavior of --enable-debug I can reinstate
    it.  Otherwise you can now force minimal-threading and no-threading to
    be disabled by configuring Poke with
      --disable-dispatch-minimal-threading --disable-dispatch-no-threading
    
    This is almost equivalent to the old --enable-debug, but a little
    better: with a compiler only supporting switch dispatching Poke would
    use switch dispatching instead of failing.
    
    At the present time even passing
      --disable-dispatch-minimal-threading --disable-dispatch-no-threading
    is pointless, however, as minimal-threading and no-threading are already
    disabled by default.  They will be enabled when I fix defective
    instructions in a reliable way.
    
    > Just a small note: the API is inconsistent there:
    > pvm_disassemble_routine vs. pvm_routine_print.  I recommend to settle in
    > one style, like verb first and then subject.  Will save us time :)
    
    You are right, that part is ugly -- I would also like to simplify the
    options for disassembling, without exposing the objdump command line at
    run time, after it is already resolved at Jitter configuration time.
    Since the API will change again because of libtextstyle I think I will
    seize that occasion to clean it up, once and for all.
    
    It will require another change in Poke, but it will be easy.  Since
    integrating libtextstyle turned out not to be completely trivial I
    decided to deal with Poke first.
    
    >       #define PKL_COMPILING_EXPRESSION 0
    >      -#define PKL_COMPILING_PROGRAM    1
    >      +#define PKL_COMPILING_ROUTINE    1
    >       #define PKL_COMPILING_STATEMENT  2
    >
    > Hm, nope.  There PKL_COMPILING_PROGRAM refers to the _Poke_ entity that
    > is being compiled.  Unrelated to PVM or jitter.  It should stay
    > PKL_COMPILING_PROGRAM.
    
    This escaped me, sorry.  I caught the same nuance in the AST but not
    here.  Undoing that change.

Ok thanks.
    
    > Please do not include GC_disable() in the patch.  The change must work
    > with the GC switched on! :)
    
    Thanks for noticing that.  Undoing it.
    
    >      +/* The C header file generated by Jitter for PVM defines 
pvm_routine as an alias
    >      +   to a VM-independent type abstract type named jitter_routine.  We 
cannot the
    >      +   header here without generating a CPP inclusion cycle, so for the 
time being
    >
    > You probably mean "We cannot include the header here"?
    
    Fixing that, and improving the comment.
    
    The next iteration is attached here.  The test suite passes.  I have not
    tried to pull the most recent changes yet, but they should not interfere
    too much.
    
    Points to be settled:
    * --enable-debug (remove it, reinstate the old behavior, leave it as a
    no-op for the future)?

Let's remove --enable-debug from Poke.  I don't like having NOP
interfaces around.  We can always reinstate it if we find we actually
need it.

    * ... ?

A little note: since this series does not include the jitter
subpackaging mode, you will also need to update HACKING to reflect a
recent enough jitter git version that supports the new routines
interface.

Other than that I'm good.  This new version LGTM.