[m-dev.] For Review: Bytecode interpreter

[Fergus Henderson]

On 24-Jan-2001, Levi Cameron <l.cameron2 at ugrad.unimelb.edu.au> wrote:
> 
>  The code as below will compile & run but nondet code
>  (ie: most of the bytecodes) have  been '#if'ed out since
>  they haven't been tested to run with native code.
> 
>  Note that there still exists debugging code throughout
>  (various SAY() calls) and that originally I assumed that
>  code & data could not occupy the same address which I
>  realised later they could, so this assumption hasn't been
>  completely removed yet.
> 
> bytecode/bytecode.c
> bytecode/bytecode.h

The log message should start with a summary saying what the change is,
e.g. "Implement the bytecode interpreter".

There should be a colon (":") after each file name in the log message.

> bytecode/dict.c
> bytecode/dict.h
> bytecode/disasm.c
> bytecode/disasm.h
> bytecode/machine.c
> bytecode/machine.h
> bytecode/mbi_main.c
> bytecode/mdb.m
> bytecode/mem.c
> bytecode/mem.h
> bytecode/slist.c
> bytecode/slist.h
> bytecode/template.c
> bytecode/template.h
> bytecode/util.c
> bytecode/util.h
> 	Removed. Thesr contains all the old bytecode files from
> 	before I started. Any parts that were useful have already
> 	been salvaged and used in the new interpreter.

s/Thesr/These/

> bytecode/Mmakefile
> bytecode/Mmake.params
> 	Makefile for test bytecode program.

Unless there is some special reason for doing it differently, the
bytecode directory should use the same Mmake.params file in `..'
that all the other directories use.

You shouldn't commit the Mmake.params file.

>       Note that any library
> 	functions that are called from bytecode must be compiled
> 	with trace information. (So their entry labels can be
> 	looked up)

I'm not sure that the log message is the right place for that comment.
It should go in the source code, either in addition, or perhaps
just instead.

> ==========================================================================
> Index: Mmakefile
...
> -MERCURY_DIR=..
> +MERCURY_DIR=/home/stude/l/lpcam/mercury/src

That change should not go in the cvs repository.

> +EXTRA_MGNUCFLAGS= --no-ansi $(INC_DIRS)

`--no-ansi' should preferably not be used.

If it has to be used, then you should document exactly why it needs to
be used.

> +EXTRA_MCFLAGS	= --generate-bytecode -V -O 0 --inline-simple-threshold
> 20

Why do you set inline-simple-threshold here?

> +TAGFLAGS	= -T -d

That should not go in the CVS repository, since it is not portable.
Instead just put it in your Mmake.params file (which should not be
committed in to the CVS repository).

> +#		  keep this list in alphabetical order, please
> +HDRS		=	$(MB_HDRS)

The comment there doesn't make sense.
Also, how does HDRS differ from MB_HDRS?
Why do you need both?

> +#		  keep this list in alphabetical order, please
> +CFILES		= 	\
> +			mb_bytecode.c \
> +			mb_disasm.c \
> +			mb_interface.c \
> +			mb_machine.c \
> +			mb_machine_show.c \
> +			mb_mem.c \
> +			mb_module.c \
> +			mb_stack.c \
> +			mb_util.c \
> +			simple01_init.c
> +#			bug.c

Hmm, I'm not sure that including simple01_init.c here is a good
idea...

> +MFILES		=	\
> +			simple01.m \
> +			mb_interface_stub.m

Hmm, generally we have avoided including `.m' files and `.c' source
files in the same directory (that is why the `trace' and `browser'
directories are separate, for example).

> +MERCURY_INCLUDES	= \
> +			$(RUNTIME_DIR)/*.c $(RUNTIME_DIR)/*.h \
> +			$(RUNTIME_DIR)/machdeps/*.c $(RUNTIME_DIR)/machdeps/*.h \
> +			$(TRACE_DIR)/*.h $(TRACE_DIR)/*.c \
> +			$(BROWSER_DIR)/*.h $(BROWSER_DIR)/*.c \
> +			$(BOEHM_GC_DIR)/*.h $(BOEHM_GC_DIR)/include/*.h

Why is this called *_INCLUDES?  Why do the C files get included in this list?
INCLUDES normally refers to stuff that is #included, which is header files
only.

> +LIBS		= $(TRACE_DIR)/libmer_trace.a

You should handle libmer_trace.a in the same way that you
handle libmer_browser.a (or document clearly why they need
to be handled differently).

> +TEST_OBJS	= \
> +		mb_bytecode.o \
> +		mb_disasm.o \
> +		mb_interface.o \
> +		mb_machine.o \
> +		mb_machine_show.o \
> +		mb_mem.o \
> +		mb_module.o \
> +		mb_stack.o \
> +		mb_util.o \
> +		simple01.o \
> +		simple01_init.o \
> +		mb_interface_stub.o
> +#		bug.o

It would be better to have a separate variable MB_OBJS for mb_*.o.

> +#%.mbc: %.c
> +#	$(MC) $(ALL_GRADEFLAGS) $(ALL_MCFLAGS) --generate-bytecode
> --compile-to-c
>  
>  .PHONY: all
> +all: test
>  
> +tags: $(CFILES) $(HDRS)
> +	ctags $(TAGFLAGS) $(CFILES) $(MFILES) $(HDRS) $(MERCURY_INCLUDES)
> +	
> +test: $(TEST_OBJS)
> +	$(ML) $(ALL_GRADEFLAGS) $(ALL_MLFLAGS) $(TEST_OBJS)
> $(BROWSER_DIR)/libmer_browser.a -o test

Why do you link in libmer_browser.a manually here?
I think it would probably be better to just pass the `--trace'
option to $(ML).

In fact, it would be better to use

	MLOBJS = $(MB_OBJS)

	test: simple01

> +simple01_init.c: simple01.c
> +	$(C2INIT) $(ALL_GRADEFLAGS) $(ALL_C2INITFLAGS) simple01.c >
> simple01_init.c

You shouldn't write that rule manually; just use `mmake
simple01.depend', which will generate it for you automatically.

> +clean_local: clean_o
> +
> +realclean_local: realclean_o
> +
> +.PHONY: clean_o
> +clean_o:
> +	rm -f $(GENCFILES)
> +	rm -f $(OBJS)
> +	rm -f $(BYTECODE)
> +	rm -f simple01.c simple01_init.c
> +	rm -f mb_interface_stub.c simple01_init.c

You shouldn't need to remove simple01.c and simple01_init.c manually;
if you use `mmake simple01.depend' that should happen autoatically.
You definitely don't need to remove simple01_init.c twice ;-)

[... to be continued ...]

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
                                    |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------