[m-dev.] ODBC interface

[Fergus Henderson]

Simon Taylor, you wrote:
> runtime/engine.h
> runtime/engine.mod
> 	Add wrappers around longjmp and setjmp which save and restore 
> 	some state in engine.c and the Mercury registers.

The ODBC stuff itself looks fine.
But MR_setjmp() and MR_longjmp() need quite a few changes.

> +typedef struct {
> +		jmp_buf *mercury_env;	/* 
> +					** used to save MR_engine_jmp_buf 
> +					*/
> +		jmp_buf env;		/* 
> +					** used by calls to setjmp and longjmp 
> +					*/
> +		Word *saved_succip;
> +		Word *saved_hp;
> +		Word *saved_sp;
> +		Word *saved_curfr;
> +		Word *saved_maxfr;
> +		Word regs[NUM_REAL_REGS];
> +	} MR_jmp_buf;

One minor bug: this code won't compile if NUM_REAL_REGS == 0.
The definition of the `regs' field should be inside `#if NUM_REAL_REGS > 0'.

I understand why we need to save/restore MR_engine_jmp_buf,
sp, curfr, maxfr, and succip.  These all basically relate to the
call stack, and we're jumping back to an earlier execution point,
so we need to pop of those stack frames.

But restoring hp -- is that safe?
I guess so, but you should document that any heap memory
allocated between the MR_setjmp() and the MR_longjmp()
will not be valid after the MR_longjmp() call.
[Or alternatively we could leave it up to the call to save/restore
hp -- again you should document that it is the caller's responsibility
to do this.]

In addition, with the new implementation of builtin_aggregate(),
the heap and the solutions_heap may have been swapped.
If so, then they need to be unswapped.
Perhaps the simplest way of handling this is to also save/restore
solutions_heap_pointer, heap_zone, and solutions_heap_zone.

For grades that support trailing, we also need to save/restore
MR_trail_ptr and MR_ticket_counter.

One more point: MR_setjmp/longjmp only save/restore the Mercury
special-purpose registers, the general-purpose ones (r1, r2, ...)
will, in general, be clobbered.  You need to document that.

> +	/*
> +	** MR_longjmp(MR_jmp_buf *env, int return)
> +	**
> +	** Reset MR_engine_jmp_buf to the value stored in env, restore the
> +	** Mercury registers, then call longjmp().
> +	*/
> +#define MR_longjmp(setjmp_env, ret)	longjmp((setjmp_env)->env, ret)

I think you should delete the `ret' parameter and just have this as

	#define MR_longjmp(setjmp_env)	longjmp((setjmp_env)->env, 1)

> +	if (s > nondetstack_zone->min) {
> +		printf("ptr 0x%p, offset %3ld words, procedure %s\n",
> +			(const void *) s, 
> +			(long) (Integer) (s - nondetstack_zone->min),
> +			(const char *) s[PREDNM]);
> +	}
> +	else {


Change those last two lines to

	} else {

> 	/*
> 	** Define some wrappers around setjmp and longjmp for exception 
> 	** handling. We need to use MR_setjmp and MR_longjmp because we'll 
> 	** be longjmping across C->Mercury calls, so we need to restore 
> 	** some state in runtime/engine.c.
> 	** Beware: the Mercury registers must be valid 
> 	** when odbc_catch is called.
> 	*/
> #define odbc_catch(longjmp_label)					\
> 			MR_setjmp(&odbc_trans_jmp_buf, longjmp_label)
> 
> #define odbc_throw() 	MR_longjmp(&odbc_trans_jmp_buf, 1)

You should also add "Beware: any memory allocated between a catch
and a throw will no longer be valid after the throw.
Also a throw will clobber the Mercury general-purpose registers
r1, r2, ..."

> :- pred odbc__transaction_2(odbc__connection,
> 		pred(T, odbc__state, odbc__state), T, 
> 		int, list(odbc__message), io__state, io__state).  
> :- mode odbc__transaction_2(in, pred(out, di, uo) is det, 
> 		out, out, out, di, uo) is det.
> 
> :- pragma c_code(
> 		odbc__transaction_2(Connection::in, 
> 			Closure::pred(out, di, uo) is det,
> 			Results::out, Status::out, Msgs::out,
> 			IO0::di, IO::uo),
> 		may_call_mercury,
> "
> 	/*
> 	** The Mercury registers must be valid at the call to odbc_catch
> 	** in odbc_transaction_c_code().
> 	*/
> 	save_transient_registers();
> 	odbc_transaction_c_code(TypeInfo_for_T, Connection, Closure, 
> 			&Results, &Status, &Msgs, IO0, &IO);
> 	restore_transient_registers();
> ").

Add a comment "odbc_transaction_c_code() may clobber the Mercury
general-purpose registers r1, r2, ..., but that is OK, because
this C code is declared as 'may_call_mercury', so the compiler
assumes that it is allowed to clobber those registers."

> odbc__rollback(Error) -->
> 	odbc__add_message(error(user_requested_rollback) - Error),
> 	odbc__throw.

Oh-oh, here you are allocating memory on the heap before a throw,
and expecting that data to persist after the throw.
This will only work in conservative-gc grades.
You should document that.

> :- pred test_trans(list(odbc__row)::out,
> 		odbc__state::di, odbc__state::uo) is det.
> 
> test_trans(Results) -->
> 	{ String = "select * from test" },
> 	odbc__execute(String, Results).

I think it would be clearer to just write

	test_trans(Results) -->
		odbc__execute("select * from test", Results).

> :- pred output_results(list(odbc__row)::in,
> 		io__state::di, io__state::uo) is det.
> 
> output_results(Rows) -->
> 	{ WriteRow = lambda([Row::in, IO0::di, IO::uo] is det, (
> 			io__write_list(Row, " ", output_attribute, IO0, IO)
> 		)) },
> 	io__write_list(Rows, "\n", WriteRow).

I think it would be clearer if `WriteRow' was written out-of-line
rather than using a lambda expression.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>   |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>   |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3         |     -- the last words of T. S. Garp.