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

[Jose E. Marchesi]

    Hello José.
    
    I have finished cleaning up the big patch, the only remaining task being
    writing a ChangeLog entry.
    
    I found a slightly better solution to the problem of breaking the
    include dependency cycle in pvm-val.h.  Nothing should be particularly
    controversial or difficult.
    
    Please tell me if you want something different; otherwise I will send
    you the final version tomorrow.

Hi Luca.  Thanks for doing this.
Please find my comments below.
    
     -PVM values in PVM programs
     +PVM values in PVM routines
      ~~~~~~~~~~~~~~~~~~~~~~~~~~
      
     -The PVM programs (data structures of type ``pvm_program``) are
     -allocated by Jitter, using ``malloc``.  Therefore, they are not
     -traversed by the GC.
     +PVM routines (data structures of type ``pvm_routine``) are
     +allocated by Jitter in complicated data structures, internally relying
     +on ``malloc``.  Their content is therefore not automatically visible to
     +the GC.

Thanks for improving my abhorrent English here and in so many other
places! :)

        struct pvm_cls
        {
     -    struct jitter_program *program;
     -    const void *entry_point;
     +    pvm_routine routine;
     +    pvm_program_point entry_point;
          struct pvm_env *env;
        };

Hmm, you didn't change pvm_program_point to pvm_routine_point in Jitter?
      
     diff --git a/acinclude.m4 b/acinclude.m4
     deleted file mode 100644
     index ad0a10c..0000000
     --- a/acinclude.m4
     +++ /dev/null

So aclinclude.m4 is going away.  This is ok.

     diff --git a/src/Makefile.am b/src/Makefile.am
     index 500c684..13fff4c 100644
     --- a/src/Makefile.am
     +++ b/src/Makefile.am
     @@ -76,17 +76,10 @@ poke_LDFLAGS =
      
      # Integration with jitter.
      
     -if POKE_DEBUG
     -  poke_CPPFLAGS += $(JITTER_DIRECT_THREADING_CPPFLAGS)
     -  poke_CFLAGS += $(JITTER_DIRECT_THREADING_CFLAGS)
     -  poke_LDADD += $(JITTER_DIRECT_THREADING_LDADD)
     -  poke_LDFLAGS += $(JITTER_DIRECT_THREADING_LDFLAGS)
     -else
     -  poke_CPPFLAGS += $(JITTER_CPPFLAGS)
     -  poke_CFLAGS += $(JITTER_CFLAGS)
     -  poke_LDADD += $(JITTER_LDADD)
     -  poke_LDFLAGS += $(JITTER_LDFLAGS)
     -endif
     +poke_CPPFLAGS += $(JITTER_CPPFLAGS)
     +poke_CFLAGS += $(JITTER_CFLAGS)
     +poke_LDADD += $(JITTER_LDADD)
     +poke_LDFLAGS += $(JITTER_LDFLAGS)
      
      BUILT_SOURCES += pvm-vm.h pvm-vm1.c pvm-vm2.c


Hm, if --enable-debug no longer controls how the Jitter VM is built, how
can the user force building with direct threading when she wants to
debug something in pvm.jitter?
      
     diff --git a/src/pk-cmd.c b/src/pk-cmd.c
     index 9b46349..980efd2 100644
     --- a/src/pk-cmd.c
     +++ b/src/pk-cmd.c
     @@ -353,17 +353,17 @@ pk_cmd_exec_1 (char *str, struct pk_trie *cmds_trie, 
char *prefix)
                      case 'e':
                        {
                          /* Compile a poke program.  */
     -                    pvm_program prog;
     +                    pvm_routine routine;

I'm ok with this general rename of `prog' to `routine'.

        if (uflags & PK_VM_DIS_F_NAT)
     -    pvm_disassemble_program (prog, true,
     +    pvm_disassemble_routine (routine, true,
                                   JITTER_OBJDUMP, NULL);
        else
     -    pvm_print_program (stdout, prog);
     +    pvm_routine_print (stdout, routine);

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 :)


     -  pvm_append_label (pasm->program, pasm->level->label1);
     +  pvm_routine_append_label (pasm->routine, pasm->level->label1);

I like the fact you normalized the name of the functions in the API.

     @@ -266,21 +266,19 @@ PKL_PHASE_BEGIN_HANDLER (pkl_gen_ps_decl)
            {
              /* At this point the code for the function specification
                 INITIAL has been assembled in the current macroassembler.
     -           Finalize the program and put it in a PVM closure, along
     +           Finalize the routine and put it in a PVM closure, along
                 with the current environment.  */
      
              void *pointers;
     -        pvm_program program = pkl_asm_finish (PKL_GEN_ASM,
     +        pvm_routine routine = pkl_asm_finish (PKL_GEN_ASM,
                                                    0 /* epilogue */,
                                                    &pointers);
              pvm_val closure;
      
              PKL_GEN_POP_ASM;
     -        pvm_specialize_program (program);
     -        closure = pvm_make_cls (program, pointers);
     -
     +        closure = pvm_make_cls (routine, pointers);
              /*XXX*/
     -        /* pvm_print_program (stdout, program); */
     +        /* pvm_routine_print (stdout, routine); */


Ok so there is no more explicit specialization in this API.  Very very
good :)

     diff --git a/src/pkl.c b/src/pkl.c
     index c5672a5..2b9cdf6 100644
     --- a/src/pkl.c
     +++ b/src/pkl.c
     @@ -47,7 +47,7 @@
      #include "poke.h"
      
      #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.

      struct pkl_compiler
     @@ -105,7 +105,7 @@ pkl_free (pkl_compiler compiler)
        free (compiler);
      }
      
     -static pvm_program
     +static pvm_routine
      rest_of_compilation (pkl_compiler compiler,
                           pkl_ast ast,
                           void **pointers)
     @@ -235,7 +235,7 @@ rest_of_compilation (pkl_compiler compiler,
      
        pkl_ast_free (ast);
        *pointers = gen_payload.pointers;
     -  return gen_payload.program;
     +  return gen_payload.routine;
      
       error:
        pkl_ast_free (ast);
     @@ -247,20 +247,20 @@ pkl_compile_buffer (pkl_compiler compiler,
                          char *buffer, char **end)
      {
        pkl_ast ast = NULL;
     -  pvm_program program;
     +  pvm_routine routine;
        int ret;
        pkl_env env = NULL;
      
        /* Note that the sole purpose of `pointers' is to serve as a root
     -     (in the stack) for the GC, to prevent the boxed values in PROGRAM
     +     (in the stack) for the GC, to prevent the boxed values in ROUTINE
           to be collected.  Ugly as shit, but conservative garbage
           collection doesn't really work.  */
        void *pointers;
      
     -  compiler->compiling = PKL_COMPILING_PROGRAM;
     +  compiler->compiling = PKL_COMPILING_ROUTINE;

Ditto here.

     @@ -397,17 +394,17 @@ pkl_compile_file (pkl_compiler compiler, const char 
*fname)
      {
        int ret;
        pkl_ast ast = NULL;
     -  pvm_program program;
     +  pvm_routine routine;
        FILE *fd;
        pkl_env env = NULL;
      
        /* Note that the sole purpose of `pointers' is to serve as a root
     -     (in the stack) for the GC, to prevent the boxed values in PROGRAM
     +     (in the stack) for the GC, to prevent the boxed values in ROUTINE
           to be collected.  Ugly as shit, but conservative garbage
           collection doesn't really work.  */
        void *pointers;
      
     -  compiler->compiling = PKL_COMPILING_PROGRAM;
     +  compiler->compiling = PKL_COMPILING_ROUTINE;

Ditto here.
      
     diff --git a/src/pvm-alloc.c b/src/pvm-alloc.c
     index b3c00a0..308d69d 100644
     --- a/src/pvm-alloc.c
     +++ b/src/pvm-alloc.c
     @@ -36,7 +36,7 @@ static void
      pvm_alloc_finalize_closure (void *object, void *client_data)
      {
        pvm_cls cls = (pvm_cls) object;
     -  pvm_destroy_program (cls->program);
     +  pvm_destroy_routine (cls->routine);
      }
      
      void *
     @@ -54,6 +54,7 @@ pvm_alloc_initialize ()
      {
        /* Initialize the Boehm Garbage Collector.  */
        GC_INIT ();
     +  GC_disable (); // FIXME: Don't.
      }

Please do not include GC_disable() in the patch.  The change must work
with the GC switched on! :)

     diff --git a/src/pvm-val.h b/src/pvm-val.h
     index 1f051ff..7677362 100644
     --- a/src/pvm-val.h
     +++ b/src/pvm-val.h
     @@ -26,6 +26,8 @@
      #include <stdint.h>
      #include <xalloc.h>
      
     +#include <jitter/jitter-routine.h> /* For Jitter routine types.  See 
below. */
     +
      /* The pvm_val opaque type implements values that are native to the
         poke virtual machine:
      
     @@ -453,23 +455,31 @@ int pvm_type_equal (pvm_val type1, pvm_val type2);
      
      #define PVM_VAL_CLS(V) (PVM_VAL_BOX_CLS (PVM_VAL_BOX ((V))))
      
     -#define PVM_VAL_CLS_PROGRAM(V) (PVM_VAL_CLS((V))->program)
     +#define PVM_VAL_CLS_ROUTINE(V) (PVM_VAL_CLS((V))->routine)
      #define PVM_VAL_CLS_ENTRY_POINT(V) (PVM_VAL_CLS((V))->entry_point)
      #define PVM_VAL_CLS_ENV(V) (PVM_VAL_CLS((V))->env)
      
     +/* 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"?

     +   we resort to using the VM-independent type rather than its VM-specific
     +   counterpart.  For all intents and purposes they are the same type. */
      struct pvm_cls
      {
     -  /* Note we have to use explicit pointers here due to the include
     -     mess induced by jitter's combined header files :/ */
     -  struct jitter_program *program;
     +  /* Here at the time we use VM-independent Jitter types rather than the
     +     corresponding PVM-specific types in order to prevent a CPP inclusion
     +     cycle. :/ */
     +  jitter_routine routine;
        void **pointers;
     -  const void *entry_point;
     +  jitter_program_point entry_point;
        struct pvm_env *env;
      };

Oh I see.  using jitter_routine instead of pvm_routine seems perfectly
ok to me.  Certainly better than the previous struct jitter_program
*program.
      
Other than the points above, the changes look good to me! :)